From 8ca25b8e49ca3442a56029f59677dfaab5b6eaf5 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Thu, 29 Dec 2022 23:43:34 +0100 Subject: [PATCH 1/3] Fix `vec_deque::Drain` FIXME --- .../alloc/src/collections/vec_deque/drain.rs | 28 +++++-------------- .../alloc/src/collections/vec_deque/mod.rs | 18 ++++++------ 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/drain.rs b/library/alloc/src/collections/vec_deque/drain.rs index 89feb361ddc..a102aaad452 100644 --- a/library/alloc/src/collections/vec_deque/drain.rs +++ b/library/alloc/src/collections/vec_deque/drain.rs @@ -57,31 +57,17 @@ impl<'a, T, A: Allocator> Drain<'a, T, A> { unsafe fn as_slices(&self) -> (*mut [T], *mut [T]) { unsafe { let deque = self.deque.as_ref(); - // FIXME: This is doing almost exactly the same thing as the else branch in `VecDeque::slice_ranges`. - // Unfortunately, we can't just call `slice_ranges` here, as the deque's `len` is currently - // just `drain_start`, so the range check would (almost) always panic. Between temporarily - // adjusting the deques `len` to call `slice_ranges`, and just copy pasting the `slice_ranges` - // implementation, this seemed like the less hacky solution, though it might be good to - // find a better one in the future. - // because `self.remaining != 0`, we know that `self.idx < deque.original_len`, so it's a valid - // logical index. - let wrapped_start = deque.to_physical_idx(self.idx); + let start = self.idx; + // We know that `self.idx + self.remaining <= deque.len <= usize::MAX`, so this won't overflow. + let end = start + self.remaining; - let head_len = deque.capacity() - wrapped_start; - - let (a_range, b_range) = if head_len >= self.remaining { - (wrapped_start..wrapped_start + self.remaining, 0..0) - } else { - let tail_len = self.remaining - head_len; - (wrapped_start..deque.capacity(), 0..tail_len) - }; - - // SAFETY: the range `self.idx..self.idx+self.remaining` lies strictly inside - // the range `0..deque.original_len`. because of this, and because of the fact - // that we acquire `a_range` and `b_range` exactly like `slice_ranges` would, + // SAFETY: the range `start..end` lies strictly inside + // the range `0..deque.original_len`. Because of this, and because + // we haven't touched the elements inside this range yet, // it's guaranteed that `a_range` and `b_range` represent valid ranges into // the deques buffer. + let (a_range, b_range) = deque.slice_ranges(start..end, end); (deque.buffer_range(a_range), deque.buffer_range(b_range)) } } diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index c955db46d29..6d3e784c8b7 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -1147,7 +1147,7 @@ impl VecDeque { #[inline] #[stable(feature = "deque_extras_15", since = "1.5.0")] pub fn as_slices(&self) -> (&[T], &[T]) { - let (a_range, b_range) = self.slice_ranges(..); + let (a_range, b_range) = self.slice_ranges(.., self.len); // SAFETY: `slice_ranges` always returns valid ranges into // the physical buffer. unsafe { (&*self.buffer_range(a_range), &*self.buffer_range(b_range)) } @@ -1181,7 +1181,7 @@ impl VecDeque { #[inline] #[stable(feature = "deque_extras_15", since = "1.5.0")] pub fn as_mut_slices(&mut self) -> (&mut [T], &mut [T]) { - let (a_range, b_range) = self.slice_ranges(..); + let (a_range, b_range) = self.slice_ranges(.., self.len); // SAFETY: `slice_ranges` always returns valid ranges into // the physical buffer. unsafe { (&mut *self.buffer_range(a_range), &mut *self.buffer_range(b_range)) } @@ -1223,19 +1223,21 @@ impl VecDeque { /// Given a range into the logical buffer of the deque, this function /// return two ranges into the physical buffer that correspond to - /// the given range. - fn slice_ranges(&self, range: R) -> (Range, Range) + /// the given range. The `len` parameter should usually just be `self.len`; + /// the reason it's passed explicitly is that if the deque is wrapped in + /// a `Drain`, then `self.len` is not actually the length of the deque. + fn slice_ranges(&self, range: R, len: usize) -> (Range, Range) where R: RangeBounds, { - let Range { start, end } = slice::range(range, ..self.len); + let Range { start, end } = slice::range(range, ..len); let len = end - start; if len == 0 { (0..0, 0..0) } else { // `slice::range` guarantees that `start <= end <= self.len`. - // because `len != 0`, we know that `start < end`, so `start < self.len` + // because `len != 0`, we know that `start < end`, so `start < len` // and the indexing is valid. let wrapped_start = self.to_physical_idx(start); @@ -1281,7 +1283,7 @@ impl VecDeque { where R: RangeBounds, { - let (a_range, b_range) = self.slice_ranges(range); + let (a_range, b_range) = self.slice_ranges(range, self.len); // SAFETY: The ranges returned by `slice_ranges` // are valid ranges into the physical buffer, so // it's ok to pass them to `buffer_range` and @@ -1321,7 +1323,7 @@ impl VecDeque { where R: RangeBounds, { - let (a_range, b_range) = self.slice_ranges(range); + let (a_range, b_range) = self.slice_ranges(range, self.len); // SAFETY: The ranges returned by `slice_ranges` // are valid ranges into the physical buffer, so // it's ok to pass them to `buffer_range` and From 1e114a88bde098d1c057161aa252fa75d5739592 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Sun, 5 Feb 2023 02:16:43 +0100 Subject: [PATCH 2/3] Add `slice_ranges` safety comment --- library/alloc/src/collections/vec_deque/drain.rs | 9 ++++----- library/alloc/src/collections/vec_deque/mod.rs | 8 ++++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/drain.rs b/library/alloc/src/collections/vec_deque/drain.rs index a102aaad452..99bd7902e69 100644 --- a/library/alloc/src/collections/vec_deque/drain.rs +++ b/library/alloc/src/collections/vec_deque/drain.rs @@ -62,11 +62,10 @@ impl<'a, T, A: Allocator> Drain<'a, T, A> { // We know that `self.idx + self.remaining <= deque.len <= usize::MAX`, so this won't overflow. let end = start + self.remaining; - // SAFETY: the range `start..end` lies strictly inside - // the range `0..deque.original_len`. Because of this, and because - // we haven't touched the elements inside this range yet, - // it's guaranteed that `a_range` and `b_range` represent valid ranges into - // the deques buffer. + // SAFETY: `start..end` represents the range of elements that + // haven't been drained yet, so they're all initialized, + // and `slice::range(start..end, end) == start..end`, + // so the preconditions for `slice_ranges` are met. let (a_range, b_range) = deque.slice_ranges(start..end, end); (deque.buffer_range(a_range), deque.buffer_range(b_range)) } diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 6d3e784c8b7..813430ae615 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -1226,6 +1226,14 @@ impl VecDeque { /// the given range. The `len` parameter should usually just be `self.len`; /// the reason it's passed explicitly is that if the deque is wrapped in /// a `Drain`, then `self.len` is not actually the length of the deque. + /// + /// # Safety + /// + /// This function is always safe to call. For the resulting ranges to be valid + /// ranges into the physical buffer, the caller must ensure that for all possible + /// values of `range` and `len`, the result of calling `slice::range(range, ..len)` + /// represents a valid range into the logical buffer, and that all elements + /// in that range are initialized. fn slice_ranges(&self, range: R, len: usize) -> (Range, Range) where R: RangeBounds, From acc876e42fce0dc3d6596f754dcfbc0ce07cabd6 Mon Sep 17 00:00:00 2001 From: Markus Everling Date: Mon, 20 Feb 2023 23:25:26 +0100 Subject: [PATCH 3/3] Changes according to review --- library/alloc/src/collections/vec_deque/drain.rs | 13 +++++++------ library/alloc/src/collections/vec_deque/mod.rs | 9 ++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/drain.rs b/library/alloc/src/collections/vec_deque/drain.rs index 99bd7902e69..0be274a3822 100644 --- a/library/alloc/src/collections/vec_deque/drain.rs +++ b/library/alloc/src/collections/vec_deque/drain.rs @@ -52,21 +52,22 @@ impl<'a, T, A: Allocator> Drain<'a, T, A> { } } - // Only returns pointers to the slices, as that's - // all we need to drop them. May only be called if `self.remaining != 0`. + // Only returns pointers to the slices, as that's all we need + // to drop them. May only be called if `self.remaining != 0`. unsafe fn as_slices(&self) -> (*mut [T], *mut [T]) { unsafe { let deque = self.deque.as_ref(); - let start = self.idx; // We know that `self.idx + self.remaining <= deque.len <= usize::MAX`, so this won't overflow. - let end = start + self.remaining; + let logical_remaining_range = self.idx..self.idx + self.remaining; - // SAFETY: `start..end` represents the range of elements that + // SAFETY: `logical_remaining_range` represents the + // range into the logical buffer of elements that // haven't been drained yet, so they're all initialized, // and `slice::range(start..end, end) == start..end`, // so the preconditions for `slice_ranges` are met. - let (a_range, b_range) = deque.slice_ranges(start..end, end); + let (a_range, b_range) = + deque.slice_ranges(logical_remaining_range.clone(), logical_remaining_range.end); (deque.buffer_range(a_range), deque.buffer_range(b_range)) } } diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs index 813430ae615..40f31f1a194 100644 --- a/library/alloc/src/collections/vec_deque/mod.rs +++ b/library/alloc/src/collections/vec_deque/mod.rs @@ -1230,10 +1230,9 @@ impl VecDeque { /// # Safety /// /// This function is always safe to call. For the resulting ranges to be valid - /// ranges into the physical buffer, the caller must ensure that for all possible - /// values of `range` and `len`, the result of calling `slice::range(range, ..len)` - /// represents a valid range into the logical buffer, and that all elements - /// in that range are initialized. + /// ranges into the physical buffer, the caller must ensure that the result of + /// calling `slice::range(range, ..len)` represents a valid range into the + /// logical buffer, and that all elements in that range are initialized. fn slice_ranges(&self, range: R, len: usize) -> (Range, Range) where R: RangeBounds, @@ -1244,7 +1243,7 @@ impl VecDeque { if len == 0 { (0..0, 0..0) } else { - // `slice::range` guarantees that `start <= end <= self.len`. + // `slice::range` guarantees that `start <= end <= len`. // because `len != 0`, we know that `start < end`, so `start < len` // and the indexing is valid. let wrapped_start = self.to_physical_idx(start);