Fix `vec_deque::Drain` FIXME #106276

Sp00ph · 2022-12-29T22:52:03Z

In my original VecDeque rewrite, I didn't use VecDeque::slice_ranges in Drain::as_slices, even though that's basically the exact use case for slice_ranges. The reason for this was that a VecDeque wrapped in a Drain actually has its length set to drain_start, so that there's no potential use after free if you mem::forget the Drain. I modified slice_ranges to accept an explicit len parameter instead, which it now uses to bounds check the given range. This way, Drain::as_slices can use slice_ranges internally instead of having to basically just copy paste the slice_ranges code. Since slice_ranges is just an internal helper function, this shouldn't change the user facing behavior in any way.

rustbot · 2022-12-29T22:52:10Z

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2022-12-29T22:52:13Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

the8472

One comment inside the method still talks about self.len

the8472 · 2023-01-18T20:42:07Z

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

-    /// the given range.
-    fn slice_ranges<R>(&self, range: R) -> (Range<usize>, Range<usize>)
+    /// 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


there's an extra whitespace here

the8472 · 2023-01-18T20:48:52Z

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

+            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;


The variable names here could be clearer.

What do you propose? Maybe something like let remaining_range = self.idx..self.idx+self.remaining;?

maybe. and also that it's a logical index, not offsets into the allocation.

the8472 · 2023-01-18T20:52:20Z

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

+            // SAFETY: the range `start..end` lies strictly inside
+            // the range `0..deque.original_len`. Because of this, and because


Hrm, this means that end may be smaller than original_len. That happens because one is draining from the middle of the deque, right? But then end isn't deque.len anymore which one is supposed to pass to slice_ranges (according to the new documentation).

In this case it doesn't really matter which length you pass, as long as it's not smaller than end. Since the range is trivially in bounds, and we don't pass a RangeFrom to slice_ranges (which would then use the len parameter as the end index), the bounds checking in slice_ranges is technically redundant anyways. Explicitly calculating original_len here to pass as a dead bounds check would just result in slightly more unnecessary computations with no difference in behavior.

Well, the documentation should say so? Also, could passing in the wrong length cause any unsafety?

The function itself can't cause any unsafety, it doesn't access any memory and only computes index ranges into the physical buffer. Using it with the wrong inputs and then using the return values to index into the buffer could obviously lead to buffer overruns, for example:

let (a, b) = self.slice_ranges(.., usize::MAX); // unsafety is introduced here since `b` doesn't satisfy the `buffer_range` preconditions. let b = unsafe { &*self.buffer_range(b) };

I'm not 100% sure how to word the safety docs for this, as you can't just require the first len items to be initialized (because Drain may have already destroyed elements at the beginning of the deque). Would something like
"The caller has to ensure that for all possible inputs, the result of calling slice::range(range, ..len) is a valid range into the logical array, and all of its elements must be initialized"
be okay? It's kind of a mouthful, but I'm not sure how else to phrase it.

the8472 · 2023-02-20T21:03:20Z

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

+    /// 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


That is indeed a bit of a mouthful and requires one to look up with slice::range does (I wasn't familiar with it). Maybe it's better to say "up to range's end or len, whichever is lower"?

Something like "all elements up to ..." wouldn't cover all our use cases, as Drain::as_slices calls this function on a deque whose elements in the logical range drain.start..drain.idx aren't initialized anymore.

I suppose we could say "all elements in the logical range range.start..cmp::min(range.end, len) must be initialized", but I'm not sure if that makes it more readable than it currently is.

Also, since there's quite a few types that implement RangeBounds, I opted for a more precise and perhaps more convoluted comment over one that's simpler but might not cover all use cases or might introduce some ambiguities.

Ok, but even if we keep the slice::range phrasing I'm still having trouble with the whole paragraph.

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

What does the "for all possible values" add? Does that refer to the covered elements? Or does it just say that every time you pass in garbage you get garbage out?

Eh, the "for all possible values" can just be left out. I'm not sure why I put it there in the first place so I guess I'll just remove it.

the8472 · 2023-02-20T21:32:14Z

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

+    /// 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<R>(&self, range: R, len: usize) -> (Range<usize>, Range<usize>)


Maybe it would be a bit clearer if this were a free function that takes range, capacity, len as arguments instead of the awkward mix where it takes the capacity from self but the len is fed in externally.

I'm not sure that'd actually simplify things. It would always be incorrect to call slice_ranges with a capacity that's not just self.capacity(), whereas len doesn't have the same restriction. Making it a free function would also make both the implementation and the call sites more lengthy and probably less readable (e.g. it couldn't use self.to_physical_idx) and might introduce more chances to accidentally mix up two arguments of the same type.

the8472 · 2023-03-10T20:53:24Z

@bors r+ rollup

bors · 2023-03-10T20:53:27Z

📌 Commit acc876e has been approved by the8472

It is now in the queue for this repository.

Fix `vec_deque::Drain` FIXME In my original `VecDeque` rewrite, I didn't use `VecDeque::slice_ranges` in `Drain::as_slices`, even though that's basically the exact use case for `slice_ranges`. The reason for this was that a `VecDeque` wrapped in a `Drain` actually has its length set to `drain_start`, so that there's no potential use after free if you `mem::forget` the `Drain`. I modified `slice_ranges` to accept an explicit `len` parameter instead, which it now uses to bounds check the given range. This way, `Drain::as_slices` can use `slice_ranges` internally instead of having to basically just copy paste the `slice_ranges` code. Since `slice_ranges` is just an internal helper function, this shouldn't change the user facing behavior in any way.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#106276 (Fix `vec_deque::Drain` FIXME) - rust-lang#107629 (rustdoc: sort deprecated items lower in search) - rust-lang#108711 (Add note when matching token with nonterminal) - rust-lang#108757 (rustdoc: Migrate `document_item_info` to Askama) - rust-lang#108784 (rustdoc: Migrate sidebar rendering to Askama) - rust-lang#108927 (Move __thread_local_inner to sys) - rust-lang#108949 (Honor current target when checking conditional compilation values) - rust-lang#108950 (Directly construct Inherited in typeck.) - rust-lang#108988 (rustdoc: Don't crash on `crate` references in blocks) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned joshtriplett Dec 29, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 29, 2022

the8472 assigned the8472 and unassigned joshtriplett Jan 18, 2023

the8472 reviewed Jan 18, 2023

View reviewed changes

Sp00ph added 2 commits January 31, 2023 15:04

Fix vec_deque::Drain FIXME

8ca25b8

Add slice_ranges safety comment

1e114a8

Sp00ph force-pushed the unify_slice_ranges branch from a62348c to 1e114a8 Compare February 5, 2023 01:17

the8472 reviewed Feb 20, 2023

View reviewed changes

Changes according to review

acc876e

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2023

matthiaskrgr mentioned this pull request Mar 11, 2023

Rollup of 10 pull requests #109012

Closed

matthiaskrgr mentioned this pull request Mar 11, 2023

Rollup of 9 pull requests #109015

Merged

bors merged commit 790d9f3 into rust-lang:master Mar 11, 2023

rustbot added this to the 1.70.0 milestone Mar 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `vec_deque::Drain` FIXME #106276

Fix `vec_deque::Drain` FIXME #106276

Sp00ph commented Dec 29, 2022

rustbot commented Dec 29, 2022

rustbot commented Dec 29, 2022

the8472 left a comment

the8472 Jan 18, 2023

the8472 Jan 18, 2023

Sp00ph Jan 18, 2023

the8472 Feb 20, 2023

the8472 Jan 18, 2023

Sp00ph Jan 18, 2023

the8472 Jan 18, 2023

Sp00ph Jan 18, 2023

the8472 Feb 20, 2023

Sp00ph Feb 20, 2023

Sp00ph Feb 20, 2023 •

edited

Loading

Sp00ph Feb 20, 2023

the8472 Feb 20, 2023

Sp00ph Feb 20, 2023

the8472 Feb 20, 2023

Sp00ph Feb 20, 2023 •

edited

Loading

the8472 commented Mar 10, 2023

bors commented Mar 10, 2023

		// SAFETY: the range `start..end` lies strictly inside
		// the range `0..deque.original_len`. Because of this, and because

		/// 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

Fix vec_deque::Drain FIXME #106276

Fix vec_deque::Drain FIXME #106276

Conversation

Sp00ph commented Dec 29, 2022

rustbot commented Dec 29, 2022

rustbot commented Dec 29, 2022

the8472 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sp00ph Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sp00ph Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

the8472 commented Mar 10, 2023

bors commented Mar 10, 2023

Fix `vec_deque::Drain` FIXME #106276

Fix `vec_deque::Drain` FIXME #106276

Sp00ph Feb 20, 2023 •

edited

Loading

Sp00ph Feb 20, 2023 •

edited

Loading