Rollup merge of #162462 - maxdexh:issue-162452-vec-into-iter-into-deque, r=Darksonn

Fix unsoundness in `VecDeque::from_iter(vec::IntoIter)`

Adds a safety requirement to `VecDeque::from_contiguous_raw_parts_in` to ensure that the `VecDeque` it creates upholds the safety invariants. In particular, either `head < capacity` or `head == capacity == 0`.
Refactors `vec::IntoIter::into_vecdeque` to uphold that invariant (and splits the unsafe blocks into multiple parts, because the safety comment was messy & incomplete). Actual fix is the addition of `|| len == 0`.

I would add a regression test, but the specialization is behind `cfg(not(test))`. I cannot see why this was done, as there are no comments documenting this...

Fixes #162452

r? libs

Author: JonathanBrouwer

library/alloc/src/collections/vec_deque/mod.rs

@@ -949,6 +949,7 @@ impl<T, A: Allocator> VecDeque<T, A> {
     /// `Vec::from_raw_parts_in`, but takes a *range* of elements that are
     /// initialized rather than only supporting `0..len`.  Requires that
     /// `initialized.start` ≤ `initialized.end` ≤ `capacity`.
+    /// Also, `initialized.start` < `capacity`, unless both are 0.
     #[inline]
     #[cfg(not(test))]
     pub(crate) unsafe fn from_contiguous_raw_parts_in(
@@ -959,9 +960,13 @@ impl<T, A: Allocator> VecDeque<T, A> {
     ) -> Self {
         debug_assert!(initialized.start <= initialized.end);
         debug_assert!(initialized.end <= capacity);
+        debug_assert!(initialized.start == 0 && capacity == 0 || initialized.start < capacity);
 
         // SAFETY: Our safety precondition guarantees the range length won't wrap,
-        // and that the allocation is valid for use in `RawVec`.
+        // that the allocation is valid for use in `RawVec` with `alloc`,
+        // and that the range contains valid elements.
+        // We have `head`, `len` ≤ `cap`, since `start`, `end` ≤ `cap`.
+        // Also, `head` < `cap` unless `head` = `cap` = `0`.
         unsafe {
             VecDeque {
                 head: WrappedIndex::from_arbitrary_number(initialized.start),

library/alloc/src/vec/into_iter.rs

@@ -213,25 +213,39 @@ impl<T, A: Allocator> IntoIter<T, A> {
         // Keep our `Drop` impl from dropping the elements and the allocator
         let mut this = ManuallyDrop::new(self);
 
-        // SAFETY: This allocation originally came from a `Vec`, so it passes
-        // all those checks. We have `this.buf` ≤ `this.ptr` ≤ `this.end`,
-        // so the `offset_from_unsigned`s below cannot wrap, and will produce a well-formed
-        // range. `end` ≤ `buf + cap`, so the range will be in-bounds.
-        // Taking `alloc` is ok because nothing else is going to look at it,
-        // since our `Drop` impl isn't going to run so there's no more code.
-        unsafe {
-            let buf = this.buf.as_ptr();
-            let initialized = if T::IS_ZST {
-                // All the pointers are the same for ZSTs, so it's fine to
-                // say that they're all at the beginning of the "allocation".
-                0..this.len()
-            } else {
-                this.ptr.offset_from_unsigned(this.buf)..this.end.offset_from_unsigned(buf)
-            };
-            let cap = this.cap;
-            let alloc = ManuallyDrop::take(&mut this.alloc);
-            VecDeque::from_contiguous_raw_parts_in(buf, initialized, cap, alloc)
-        }
+        let buf = this.buf.as_ptr();
+        let initialized = if T::IS_ZST || this.len() == 0 {
+            // All the pointers are the same for ZSTs, so it's fine to
+            // say that they're all at the beginning of the "allocation".
+            // For non-ZSTs, we have length 0, so we can choose the (empty)
+            // range to be at the start of the buffer.
+            //
+            // Due to `0` ≤ `this.len()` ≤ `this.cap`, the range is well-formed,
+            // and due to the argument above it spans exactly the elements of
+            // this iterator. Because `init.start` = `0`, it follows that either
+            // `init.start` < `cap` or `cap` = `init.start` = `0`; thus the range
+            // satisfies the requirements of `from_contiguous_raw_parts_in`.
+            0..this.len()
+        } else {
+            // SAFETY: `this.ptr` and `this.end` are created via offsets of `this.buf`,
+            // so they point to the same allocation. We have `this.buf` ≤ `this.ptr` ≤ `this.end`,
+            // so this cannot wrap, and will produce a well-formed range that spans exactly
+            // the elements of this iterator.
+            //
+            // Additionally, due to `end ≤ buf + cap`, we have `init.start` ≤ `init.end` ≤ `cap`.
+            // Due to the length check above, `init.start < cap`, so the range satisfies the
+            // requirements of `from_contiguous_raw_parts_in`.
+            unsafe { this.ptr.offset_from_unsigned(this.buf)..this.end.offset_from_unsigned(buf) }
+        };
+
+        let cap = this.cap;
+        // SAFETY: `this` is forgotten afterwards, so we can move out the allocator.
+        let alloc = unsafe { ManuallyDrop::take(&mut this.alloc) };
+
+        // SAFETY: This allocation originally came from a `Vec`, so it satisfies all
+        // requirements for the `buf` pointer with capacity `cap` allocated in `alloc`.
+        // Correctness of `initialized` was shown above.
+        unsafe { VecDeque::from_contiguous_raw_parts_in(buf, initialized, cap, alloc) }
     }
 }
 

library/alloctests/tests/vec_deque.rs

@@ -2495,3 +2495,18 @@ fn truncate_to_range_inclusive_end_overflow() {
     let mut v: VecDeque<_> = (0..6).collect();
     v.truncate_to_range(0..=usize::MAX);
 }
+
+#[test]
+fn issue_162452_vec_deque_from_empty_vec_into_iter() {
+    for n in 1..20 {
+        let v = Vec::from_iter(0..n);
+
+        let mut it = v.into_iter();
+        for _ in &mut it {}
+
+        let mut d: VecDeque<_> = it.collect();
+
+        d.push_back(n);
+        assert_eq!(Some(n), d.pop_front());
+    }
+}