From d0f404d77a4efefa132346c507738d9a5c6e69b4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 23 Dec 2022 15:08:56 +0100 Subject: [PATCH] fix IntoIter::drop on high-alignment ZST --- library/alloc/src/vec/into_iter.rs | 21 +++++++++++---------- src/tools/miri/README.md | 1 + src/tools/miri/tests/pass/vec.rs | 18 +++++++++++------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index 6bcde6d899ce8..cd4ea829e3782 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -40,7 +40,9 @@ pub struct IntoIter< // to avoid dropping the allocator twice we need to wrap it into ManuallyDrop pub(super) alloc: ManuallyDrop, pub(super) ptr: *const T, - pub(super) end: *const T, + pub(super) end: *const T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that + // ptr == end is a quick test for the Iterator being empty, that works + // for both ZST and non-ZST. } #[stable(feature = "vec_intoiter_debug", since = "1.13.0")] @@ -132,7 +134,9 @@ impl IntoIter { /// Forgets to Drop the remaining elements while still allowing the backing allocation to be freed. pub(crate) fn forget_remaining_elements(&mut self) { - self.ptr = self.end; + // For th ZST case, it is crucial that we mutate `end` here, not `ptr`. + // `ptr` must stay aligned, while `end` may be unaligned. + self.end = self.ptr; } #[cfg(not(no_global_oom_handling))] @@ -184,10 +188,9 @@ impl Iterator for IntoIter { if self.ptr == self.end { None } else if T::IS_ZST { - // purposefully don't use 'ptr.offset' because for - // vectors with 0-size elements this would return the - // same pointer. - self.ptr = self.ptr.wrapping_byte_add(1); + // `ptr` has to stay where it is to remain aligned, so we reduce the length by 1 by + // reducing the `end`. + self.end = self.end.wrapping_byte_sub(1); // Make up a value of this ZST. Some(unsafe { mem::zeroed() }) @@ -214,10 +217,8 @@ impl Iterator for IntoIter { let step_size = self.len().min(n); let to_drop = ptr::slice_from_raw_parts_mut(self.ptr as *mut T, step_size); if T::IS_ZST { - // SAFETY: due to unchecked casts of unsigned amounts to signed offsets the wraparound - // effectively results in unsigned pointers representing positions 0..usize::MAX, - // which is valid for ZSTs. - self.ptr = self.ptr.wrapping_byte_add(step_size); + // See `next` for why we sub `end` here. + self.end = self.end.wrapping_byte_sub(step_size); } else { // SAFETY: the min() above ensures that step_size is in bounds self.ptr = unsafe { self.ptr.add(step_size) }; diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index dac0a9820b90e..989a196b700c3 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -639,6 +639,7 @@ Definite bugs found: * [Data race in `thread::scope`](https://github.com/rust-lang/rust/issues/98498) * [`regex` incorrectly handling unaligned `Vec` buffers](https://www.reddit.com/r/rust/comments/vq3mmu/comment/ienc7t0?context=3) * [Incorrect use of `compare_exchange_weak` in `once_cell`](https://github.com/matklad/once_cell/issues/186) +* [Dropping with unaligned pointers in `vec::IntoIter`](https://github.com/rust-lang/rust/pull/106084) Violations of [Stacked Borrows] found that are likely bugs (but Stacked Borrows is currently just an experiment): diff --git a/src/tools/miri/tests/pass/vec.rs b/src/tools/miri/tests/pass/vec.rs index 26732cec5eb9a..a165c7c2fe79b 100644 --- a/src/tools/miri/tests/pass/vec.rs +++ b/src/tools/miri/tests/pass/vec.rs @@ -37,15 +37,19 @@ fn vec_into_iter() -> u8 { } fn vec_into_iter_rev() -> u8 { - vec![1, 2, 3, 4].into_iter().map(|x| x * x).fold(0, |x, y| x + y) + vec![1, 2, 3, 4].into_iter().rev().map(|x| x * x).fold(0, |x, y| x + y) } -fn vec_into_iter_zst() -> usize { - vec![[0u64; 0], [0u64; 0]].into_iter().rev().map(|x| x.len()).sum() +fn vec_into_iter_zst() { + for _ in vec![[0u64; 0]].into_iter() {} + let v = vec![[0u64; 0], [0u64; 0]].into_iter().map(|x| x.len()).sum::(); + assert_eq!(v, 0); } -fn vec_into_iter_rev_zst() -> usize { - vec![[0u64; 0], [0u64; 0]].into_iter().rev().map(|x| x.len()).sum() +fn vec_into_iter_rev_zst() { + for _ in vec![[0u64; 0]; 5].into_iter().rev() {} + let v = vec![[0u64; 0], [0u64; 0]].into_iter().rev().map(|x| x.len()).sum::(); + assert_eq!(v, 0); } fn vec_iter_and_mut() { @@ -150,8 +154,8 @@ fn main() { assert_eq!(vec_into_iter(), 30); assert_eq!(vec_into_iter_rev(), 30); vec_iter_and_mut(); - assert_eq!(vec_into_iter_zst(), 0); - assert_eq!(vec_into_iter_rev_zst(), 0); + vec_into_iter_zst(); + vec_into_iter_rev_zst(); vec_iter_and_mut_rev(); assert_eq!(make_vec().capacity(), 4);