From 9e22516877a2853c71456ef792fcbd9087fb905a Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sun, 26 Feb 2023 01:27:18 +0100 Subject: [PATCH 1/2] Fix `VecDeque::shrink_to` and add tests. This adds both a test specific to #108453 as well as an exhaustive test that goes through all possible combinations of head index, length and target capacity for a deque with capacity 16. --- .../alloc/src/collections/vec_deque/mod.rs | 119 +++++++++--------- .../alloc/src/collections/vec_deque/tests.rs | 42 +++++++ 2 files changed, 105 insertions(+), 56 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 1573b3d77dc..b30449ed06a 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -944,65 +944,72 @@ pub fn shrink_to(&mut self, min_capacity: usize) { return; } - if target_cap < self.capacity() { - // There are three cases of interest: - // All elements are out of desired bounds - // Elements are contiguous, and head is out of desired bounds - // Elements are discontiguous, and tail is out of desired bounds - // - // At all other times, element positions are unaffected. - // - // Indicates that elements at the head should be moved. + // There are three cases of interest: + // All elements are out of desired bounds + // Elements are contiguous, and tail is out of desired bounds + // Elements are discontiguous, and head is out of desired bounds + // + // At all other times, element positions are unaffected. - let tail_outside = (target_cap + 1..=self.capacity()).contains(&(self.head + self.len)); - // Move elements from out of desired bounds (positions after target_cap) - if self.len == 0 { - self.head = 0; - } else if self.head >= target_cap && tail_outside { - // H := head - // L := last element - // H L - // [. . . . . . . . o o o o o o o . ] - // H L - // [o o o o o o o . ] - unsafe { - // nonoverlapping because self.head >= target_cap >= self.len - self.copy_nonoverlapping(self.head, 0, self.len); - } - self.head = 0; - } else if self.head < target_cap && tail_outside { - // H := head - // L := last element - // H L - // [. . . o o o o o o o . . . . . . ] - // L H - // [o o . o o o o o ] - let len = self.head + self.len - target_cap; - unsafe { - self.copy_nonoverlapping(target_cap, 0, len); - } - } else if self.head >= target_cap { - // H := head - // L := last element - // L H - // [o o o o o . . . . . . . . . o o ] - // L H - // [o o o o o . o o ] - let len = self.capacity() - self.head; - let new_head = target_cap - len; - unsafe { - // can't use copy_nonoverlapping here for the same reason - // as in `handle_capacity_increase()` - self.copy(self.head, new_head, len); - } - self.head = new_head; + // `head` and `len` are at most `isize::MAX` and `target_cap < self.capacity()`, so nothing can + // overflow. + let tail_outside = (target_cap + 1..=self.capacity()).contains(&(self.head + self.len)); + + if self.len == 0 { + self.head = 0; + } else if self.head >= target_cap && tail_outside { + // Head and tail are both out of bounds, so copy all of them to the front. + // + // H := head + // L := last element + // H L + // [. . . . . . . . o o o o o o o . ] + // H L + // [o o o o o o o . ] + unsafe { + // nonoverlapping because `self.head >= target_cap >= self.len`. + self.copy_nonoverlapping(self.head, 0, self.len); } - - self.buf.shrink_to_fit(target_cap); - - debug_assert!(self.head < self.capacity() || self.capacity() == 0); - debug_assert!(self.len <= self.capacity()); + self.head = 0; + } else if self.head < target_cap && tail_outside { + // Head is in bounds, tail is out of bounds. + // Copy the overflowing part to the beginning of the + // buffer. This won't overlap because `target_cap >= self.len`. + // + // H := head + // L := last element + // H L + // [. . . o o o o o o o . . . . . . ] + // L H + // [o o . o o o o o ] + let len = self.head + self.len - target_cap; + unsafe { + self.copy_nonoverlapping(target_cap, 0, len); + } + } else if !self.is_contiguous() { + // Head is out of bounds, tail is in bounds. + // Copy the head backwards so it lines up with the target capacity. + // This won't overlap because `target_cap >= self.len`. + // + // H := head + // L := last element + // L H + // [o o o o o . . . . . . . . . o o ] + // L H + // [o o o o o . o o ] + let head_len = self.capacity() - self.head; + let new_head = target_cap - head_len; + unsafe { + // can't use `copy_nonoverlapping()` here because the new and old + // regions for the head might overlap. + self.copy(self.head, new_head, head_len); + } + self.head = new_head; } + self.buf.shrink_to_fit(target_cap); + + debug_assert!(self.head < self.capacity() || self.capacity() == 0); + debug_assert!(self.len <= self.capacity()); } /// Shortens the deque, keeping the first `len` elements and dropping diff --git a/library/alloc/src/collections/vec_deque/tests.rs b/library/alloc/src/collections/vec_deque/tests.rs index 220ad71beab..205a8ff3c19 100644 --- a/library/alloc/src/collections/vec_deque/tests.rs +++ b/library/alloc/src/collections/vec_deque/tests.rs @@ -748,6 +748,48 @@ fn test_drain() { } } +#[test] +fn issue_108453() { + let mut deque = VecDeque::with_capacity(10); + + deque.push_back(1u8); + deque.push_back(2); + deque.push_back(3); + + deque.push_front(10); + deque.push_front(9); + + deque.shrink_to(9); + + assert_eq!(deque.into_iter().collect::>(), vec![9, 10, 1, 2, 3]); +} + +#[test] +fn test_shrink_to() { + // test deques with capacity 16 with all possible head positions, lengths and target capacities. + let cap = 16; + + for len in 0..cap { + for head in 0..cap { + let expected = (1..=len).collect::>(); + + for target_cap in len..cap { + let mut deque = VecDeque::with_capacity(cap); + // currently, `with_capacity` always allocates the exact capacity if it's greater than 8. + assert_eq!(deque.capacity(), cap); + + // we can let the head point anywhere in the buffer since the deque is empty. + deque.head = head; + deque.extend(1..=len); + + deque.shrink_to(target_cap); + + assert_eq!(deque, expected); + } + } + } +} + #[test] fn test_shrink_to_fit() { // This test checks that every single combination of head and tail position, From 4a4f43e4e9b994c729a0506d921fe9734673a20a Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sun, 26 Feb 2023 02:33:34 +0100 Subject: [PATCH 2/2] Disambiguate comments --- library/alloc/src/collections/vec_deque/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index b30449ed06a..d4a12509b1c 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -947,7 +947,7 @@ pub fn shrink_to(&mut self, min_capacity: usize) { // There are three cases of interest: // All elements are out of desired bounds // Elements are contiguous, and tail is out of desired bounds - // Elements are discontiguous, and head is out of desired bounds + // Elements are discontiguous // // At all other times, element positions are unaffected. @@ -987,7 +987,7 @@ pub fn shrink_to(&mut self, min_capacity: usize) { self.copy_nonoverlapping(target_cap, 0, len); } } else if !self.is_contiguous() { - // Head is out of bounds, tail is in bounds. + // The head slice is at least partially out of bounds, tail is in bounds. // Copy the head backwards so it lines up with the target capacity. // This won't overlap because `target_cap >= self.len`. //