Rollup merge of #106084 - RalfJung:into-iter, r=thomcc

fix vec::IntoIter::drop on high-alignment ZST

This fixes a soundness bug: IntoIter would call `drop_in_place` on an insufficiently aligned pointer. So if a ZST with alignment greater 1 had drop glue, that would be called with an unaligned reference. Since #103957, Miri checks alignment even if the type does not have drop glue, which is how this bug was found.

r? ``@thomcc``

Author: matthiaskrgr

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<A>,
     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<T, A: Allocator> IntoIter<T, A> {
 
     /// 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<T, A: Allocator> Iterator for IntoIter<T, A> {
         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<T, A: Allocator> Iterator for IntoIter<T, A> {
         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) };
@@ -250,7 +251,7 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {
                 return Err(unsafe { array::IntoIter::new_unchecked(raw_ary, 0..len) });
             }
 
-            self.ptr = self.ptr.wrapping_byte_add(N);
+            self.end = self.end.wrapping_byte_sub(N);
             // Safety: ditto
             return Ok(unsafe { raw_ary.transpose().assume_init() });
         }

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<u8>` 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):
 

src/tools/miri/tests/pass/vec.rs

@@ -1,4 +1,6 @@
 //@compile-flags: -Zmiri-strict-provenance
+#![feature(iter_advance_by, iter_next_chunk)]
+
 // Gather all references from a mutable iterator and make sure Miri notices if
 // using them is dangerous.
 fn test_all_refs<'a, T: 'a>(dummy: &mut T, iter: impl Iterator<Item = &'a mut T>) {
@@ -37,15 +39,31 @@ 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::<usize>();
+    assert_eq!(v, 0);
+
+    let mut it = vec![[0u64; 0], [0u64; 0]].into_iter();
+    it.advance_by(1);
+    drop(it);
+
+    let mut it = vec![[0u64; 0], [0u64; 0]].into_iter();
+    it.next_chunk::<1>().unwrap();
+    drop(it);
+
+    let mut it = vec![[0u64; 0], [0u64; 0]].into_iter();
+    it.next_chunk::<4>().unwrap_err();
+    drop(it);
 }
 
-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::<usize>();
+    assert_eq!(v, 0);
 }
 
 fn vec_iter_and_mut() {
@@ -150,8 +168,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);