From c07955c6b6a8901fc59d9c22004127b30a965407 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Wed, 17 Mar 2021 16:02:07 -0700 Subject: [PATCH] Fix overflowing length in Vec to VecDeque `Vec` can hold up to `usize::MAX` ZST items, but `VecDeque` has a lower limit to keep its raw capacity as a power of two, so we should check that in `From> for VecDeque`. We can also simplify the capacity check for the remaining non-ZST case. Before this fix, the new test would fail on the length: ``` thread 'collections::vec_deque::tests::test_from_vec_zst_overflow' panicked at 'assertion failed: `(left == right)` left: `0`, right: `9223372036854775808`', library/alloc/src/collections/vec_deque/tests.rs:474:5 note: panic did not contain expected string panic message: `"assertion failed: `(left == right)`\n left: `0`,\n right: `9223372036854775808`"`, expected substring: `"capacity overflow"` ``` That was a result of `len()` using a mask `& (size - 1)` with the improper length. Now we do get a "capacity overflow" panic as soon as that `VecDeque::from(vec)` is attempted. --- .../alloc/src/collections/vec_deque/mod.rs | 37 +++++++++---------- .../alloc/src/collections/vec_deque/tests.rs | 15 ++++++++ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index f7cefdce27856..7a0de74eb239d 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -2783,27 +2783,26 @@ impl From> for VecDeque { /// This avoids reallocating where possible, but the conditions for that are /// strict, and subject to change, and so shouldn't be relied upon unless the /// `Vec` came from `From>` and hasn't been reallocated. - fn from(other: Vec) -> Self { - unsafe { - let mut other = ManuallyDrop::new(other); - let other_buf = other.as_mut_ptr(); - let mut buf = RawVec::from_raw_parts(other_buf, other.capacity()); - let len = other.len(); - - // We need to extend the buf if it's not a power of two, too small - // or doesn't have at least one free space. - // We check if `T` is a ZST in the first condition, - // because `usize::MAX` (the capacity returned by `capacity()` for ZST) - // is not a power of two and thus it'll always try - // to reserve more memory which will panic for ZST (rust-lang/rust#78532) - if (!buf.capacity().is_power_of_two() && mem::size_of::() != 0) - || (buf.capacity() < (MINIMUM_CAPACITY + 1)) - || (buf.capacity() == len) - { - let cap = cmp::max(buf.capacity() + 1, MINIMUM_CAPACITY + 1).next_power_of_two(); - buf.reserve_exact(len, cap - len); + fn from(mut other: Vec) -> Self { + let len = other.len(); + if mem::size_of::() == 0 { + // There's no actual allocation for ZSTs to worry about capacity, + // but `VecDeque` can't handle as much length as `Vec`. + assert!(len < MAXIMUM_ZST_CAPACITY, "capacity overflow"); + } else { + // We need to resize if the capacity is not a power of two, too small or + // doesn't have at least one free space. We do this while it's still in + // the `Vec` so the items will drop on panic. + let min_cap = cmp::max(MINIMUM_CAPACITY, len) + 1; + let cap = cmp::max(min_cap, other.capacity()).next_power_of_two(); + if other.capacity() != cap { + other.reserve_exact(cap - len); } + } + unsafe { + let (other_buf, len, capacity) = other.into_raw_parts(); + let buf = RawVec::from_raw_parts(other_buf, capacity); VecDeque { tail: 0, head: len, buf } } } diff --git a/library/alloc/src/collections/vec_deque/tests.rs b/library/alloc/src/collections/vec_deque/tests.rs index 87e06fa394d38..6116cfe1d0110 100644 --- a/library/alloc/src/collections/vec_deque/tests.rs +++ b/library/alloc/src/collections/vec_deque/tests.rs @@ -457,6 +457,21 @@ fn test_from_vec() { assert!(vd.into_iter().eq(vec)); } } + + let vec = Vec::from([(); MAXIMUM_ZST_CAPACITY - 1]); + let vd = VecDeque::from(vec.clone()); + assert!(vd.cap().is_power_of_two()); + assert_eq!(vd.len(), vec.len()); +} + +#[test] +#[should_panic = "capacity overflow"] +fn test_from_vec_zst_overflow() { + use crate::vec::Vec; + let vec = Vec::from([(); MAXIMUM_ZST_CAPACITY]); + let vd = VecDeque::from(vec.clone()); // no room for +1 + assert!(vd.cap().is_power_of_two()); + assert_eq!(vd.len(), vec.len()); } #[test]