Add `slice::check_range` #75207

dylni · 2020-08-06T02:39:06Z

This method is useful for RangeBounds parameters. It's even been rewritten many times in the standard library, sometimes assuming that the bounds won't be usize::MAX.

For example, Vec::drain creates an empty iterator when usize::MAX is used as an inclusive end bound:

assert!(vec![1].drain(..=usize::max_value()).eq(iter::empty()));

If this PR is merged, I'll create another to use it for those methods.

rust-highfive · 2020-08-06T02:39:08Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

leonardo-m · 2020-08-06T07:28:44Z

library/core/src/slice/mod.rs

+    /// [`Index::index`]: ../ops/trait.Index.html#tymethod.index
+    #[track_caller]
+    #[unstable(feature = "slice_check_range", issue = "none")]
+    pub fn check_range<R: RangeBounds<usize>>(&self, range: R) -> Range<usize> {


We can't unfortunately return a CheckedRange<usize> because there's no association with a specific range, so it's useless.

Can you explain more? I don't see how this would be useless, since it converts RangeBounds to usizes. It wasn't meant to reduce unsafe code.

It could return a CheckedRange that has a reference to the slice and a safe get method. However, that would require check_range_mut and CheckedRangeMut too, which would make this simple method much more complex.

Your code is useful. I meant to say that returning a CheckedRange is not so useful. As you say it could be done with the reference to the slice, but it could make it over engineered, so all is OK.

Oh, that makes sense. Thanks for the clarification.

KodrAus · 2020-08-12T22:41:34Z

If this PR is merged, I'll create another to use it for those methods.

Would you be happy to roll replacing the ad-hoc implementations with this in into the same PR here? Just so we can land it all together and know everything is done 🙂

dylni · 2020-08-13T00:07:28Z

Sure, I can update the PR this week. I was initially trying to reduce the scope of the change, but adding it now is not a problem

dylni · 2020-08-17T01:49:43Z

library/alloc/src/collections/vec_deque.rs

-        assert!(end <= len, "upper bound was too large");
-        (start, end)
+        // SAFETY: This buffer is only used to check the range.
+        let buffer = unsafe { slice::from_raw_parts(self.ptr(), self.len()) };


This is a hack, but it's not too bad.

Could this use buffer_as_slice?

I used that method at first, but I decided to use from_raw_parts instead, since it's a hack either way. The issue is that I need a contiguous slice for check_range, so I use the first self.len() elements even if they're not initialized.

I could change it to unsafe { self.buffer_as_slice()[..self.len()] }, but that adds an unnecessary bounds check and looks more valid than it is.

I also added more of this explanation to the safety comment.

dylni · 2020-08-17T02:16:46Z

There are 3 more ad-hoc implementations:

rust/library/alloc/src/collections/btree/map.rs

Lines 2117 to 2125 in 4bb4b96

    
           match (range.start_bound(), range.end_bound()) { 
        
               (Excluded(s), Excluded(e)) if s == e => { 
        
                   panic!("range start and end are equal and excluded in BTreeMap") 
        
               } 
        
               (Included(s) | Excluded(s), Included(e) | Excluded(e)) if s > e => { 
        
                   panic!("range start is greater than range end in BTreeMap") 
        
               } 
        
               _ => {} 
        
           };

The bounds aren't integers, and they're not used for a slice.

rust/src/librustc_expand/proc_macro_server.rs

Lines 579 to 589 in 4bb4b96

    
           let start = match start { 
        
               Bound::Included(lo) => lo, 
        
               Bound::Excluded(lo) => lo + 1, 
        
               Bound::Unbounded => 0, 
        
           }; 
        
           let end = match end { 
        
               Bound::Included(hi) => hi + 1, 
        
               Bound::Excluded(hi) => hi, 
        
               Bound::Unbounded => length, 
        
           };

I don't think this method is allowed to panic, but it also doesn't use a slice.

rust/src/librustc_data_structures/sorted_map.rs

Lines 207 to 227 in 4bb4b96

    
           let start = match range.start_bound() { 
        
               Bound::Included(ref k) => match self.lookup_index_for(k) { 
        
                   Ok(index) | Err(index) => index, 
        
               }, 
        
               Bound::Excluded(ref k) => match self.lookup_index_for(k) { 
        
                   Ok(index) => index + 1, 
        
                   Err(index) => index, 
        
               }, 
        
               Bound::Unbounded => 0, 
        
           }; 
        
           let end = match range.end_bound() { 
        
               Bound::Included(ref k) => match self.lookup_index_for(k) { 
        
                   Ok(index) => index + 1, 
        
                   Err(index) => index, 
        
               }, 
        
               Bound::Excluded(ref k) => match self.lookup_index_for(k) { 
        
                   Ok(index) | Err(index) => index, 
        
               }, 
        
               Bound::Unbounded => self.data.len(), 
        
           };

I don't see any way check_range could be defined that would help here.

KodrAus

Thanks for taking on the extra scope creep and running through all the places we could use this! I've just left a few little comments

KodrAus · 2020-08-24T03:42:57Z

library/alloc/src/collections/vec_deque.rs

-        assert!(end <= len, "upper bound was too large");
-        (start, end)
+        // SAFETY: This buffer is only used to check the range.
+        let buffer = unsafe { slice::from_raw_parts(self.ptr(), self.len()) };


Could this use buffer_as_slice?

KodrAus · 2020-08-24T03:45:34Z

library/alloc/src/vec.rs

-        if end > len {
-            end_assert_failed(end, len);
-        }
+        let Range { start, end } = self.check_range(range);


It looks like this method was pretty finely tuned, do we have any existing benchmarks we can check to see whether there's any impact?

I don't think Vec::drain has any benches. These are the benches for Vec:
https://github.com/rust-lang/rust/blob/master/library/alloc/benches/vec.rs

However, I wouldn't expect a regression. check_range calls functions with the same attributes for this reason. That's part of why I think it's better to package it with the standard library.

If there aren't any benches then I'd consider it a bit of a speculative implementation anyways, so am happy for this to be made simpler 👍

dylni · 2020-08-24T14:58:43Z

No problem! It's probably better to include the simplifications in this PR anyway

KodrAus · 2020-09-04T02:35:22Z

Thanks for your patience @dylni! This looks great to me.

@bors r+

bors · 2020-09-04T02:35:23Z

📌 Commit d9e877f has been approved by KodrAus

dylni · 2020-09-04T03:22:38Z

Thanks @KodrAus! Don't worry, I've seen you active on a lot of RFCs.

Should I create a tracking issue for this method?

Edit: #76393

bors · 2020-09-04T04:57:25Z

⌛ Testing commit d9e877f with merge 82877c725f6510e8514b197af2e06e1e3f586a97...

bors · 2020-09-04T09:12:25Z

💥 Test timed out

Dylan-DPC-zz · 2020-09-04T12:21:31Z

@bors retry

bors · 2020-09-04T12:21:49Z

⌛ Testing commit d9e877f with merge ef55a0a...

bors · 2020-09-04T14:32:16Z

☀️ Test successful - checks-actions, checks-azure
Approved by: KodrAus
Pushing ef55a0a to master...

RalfJung · 2020-09-13T09:44:45Z

Something about this PR is breaking Rust's aliasing rules. The test_stable_pointers test started failing when the PR landed: calling drain on only a part of the vector creates a reference that overlaps with unaffected elements, just invalidating those pointers. I am still investigating the details.

RalfJung · 2020-09-13T09:46:50Z

library/alloc/src/vec.rs

-        if end > len {
-            end_assert_failed(end, len);
-        }
+        let Range { start, end } = self.check_range(range);


This call here is the problem: this implicitly creates a slice covering the entire vector, which is a problem because that slice aliases with other pointers that existed before and that should remain valid.

I proposed a fix in #76662.

Interesting. Wouldn't this use the safe Deref implementation for Vec? Even if not, why doesn't the mutable reference to self on this method prevent aliasing?

It prevents aliasing from safe clients, but Vec explicitly wants to support some unsafe clients that keep pointers into the buffer. That is okay as long as all the operations involved are guaranteed not to reallocate the buffer.

See here for the testcase that shows how unsafe code can rely on this property.

Wow, I had no idea Vec makes those guarantees. Thanks for the explanation!

Many of these guarantees are documented here. I might have gone overboard in the test and check more than is guaranteed, but then it doesn't seem unlikely that people will take this guarantee as far as they can -- so I added a test basically for every operation that I could imagine preserving the buffer. Someone else double-checking this certainly would not hurt. :)

Indeed, I read that passage but not thoroughly enough to notice these subtleties.

but then it doesn't seem unlikely that people will take this guarantee as far as they can

IIUC, there is still room for some of those calls to be removed. From the guarantees:

Bulk insertion methods may reallocate, even when not necessary.

Thus, append, extend, extend_from_slice, and sometimes splice shouldn't need to be restricted. Of course, keeping them in the test doesn't hurt. :)

Yeah... maybe we should add a comment though that this test does not constitute a guarantee. I'll prepare a PR.

RalfJung · 2020-09-13T09:48:37Z

library/alloc/src/collections/vec_deque.rs

-        (start, end)
+        // SAFETY: This buffer is only used to check the range. It might be partially
+        // uninitialized, but `check_range` needs a contiguous slice.
+        // https://github.com/rust-lang/rust/pull/75207#discussion_r471193682


Creating a partially uninitialized slice is UB. So I don't think the safety comment here is accurate.

I thought this was something the standard library could assume. Wouldn't that make buffer_as_slice UB too?

If this was meant to rely on "the standard library is special and can bend the rules", the comment should explicitly say so. The standard library can't just implicitly do this, the risk is too high that someone will copy this code and use it in their own crate.

However, even then it is better to avoid; usually we only do that for layout assumptions (Cc rust-lang/unsafe-code-guidelines#90). For other kinds of UB, it is typically considered a bug.

Wouldn't that make buffer_as_slice UB too?

Maybe? I am not familiar with the code I am afraid, I just read the safety comment.

Yeah, looking at the code, buffer_as_slice should likely return a raw slice instead of a reference.

If this was meant to rely on "the standard library is special and can bend the rules", the comment should explicitly say so.

You're right. My mistake.

However, even then it is better to avoid; usually we only do that for layout assumptions

That makes sense. I was mostly using buffer_as_slice as a reference, but I agree that avoiding that assumption is better.

Yeah, there's lots of old code here that was written before we had clear rules, and so does not follow today's rules. This case here is not a big deal because the discussion is still open whether we want this to be UB or not (that's rust-lang/unsafe-code-guidelines#77), but while we discuss it's better to follow the rules, even in the standard library, also to learn how hard/annoying it is to work with these rules.

Oh also the standard library currently does a poor job at documenting when it relies on extra assumptions, but I am pushing for new code that we add to do better. :)

That all sounds great, :)

RalfJung · 2020-09-13T09:51:20Z

library/core/src/slice/mod.rs

+    /// [`Index::index`]: ops::Index::index
+    #[track_caller]
+    #[unstable(feature = "slice_check_range", issue = "none")]
+    pub fn check_range<R: RangeBounds<usize>>(&self, range: R) -> Range<usize> {


Given that we only need the length of the slice, I don't think this method should take a reference to the full slice. References are very strong types, they assert the aliasing rules and they assert that the memory they point to is initialized. As my other messages here show, that is a problem for this method.

For future readers, this conversation was continued here.

@dylni

…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.

@dylni

…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.

@dylni

…lacrum Fix liballoc test suite for Miri Mostly, fix the regression introduced by rust-lang#75207 that caused slices (i.e., references) to be created to invalid memory or memory that has aliasing pointers that we want to keep valid. @dylni this changes the type of `check_range` to only require the length, not the full reference to the slice, which indeed is all the information this function requires. Also reduce the size of a test introduced in rust-lang#70793 to make it not take 3 minutes in Miri. This makes https://github.com/RalfJung/miri-test-libstd work again.

Add slice::check_range

bb70e52

rust-highfive assigned KodrAus Aug 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2020

Fix links

202b242

leonardo-m reviewed Aug 6, 2020

View reviewed changes

Fix links again

ed02b90

Replace ad hoc implementations with slice::check_range

d04e6b8

dylni commented Aug 17, 2020

View reviewed changes

KodrAus reviewed Aug 24, 2020

View reviewed changes

Add more information to safety comment

d9e877f

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 Sep 4, 2020

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

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 Sep 4, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 4, 2020

bors merged commit ef55a0a into rust-lang:master Sep 4, 2020

This was referenced Sep 4, 2020

Add escape_ascii method to u8 and [u8] #73111

Closed

Detect overflow in proc_macro_server subspan #75146

Merged

Split core::slice to smaller mods #76311

Merged

dylni deleted the add-slice-check-range branch September 4, 2020 16:05

dylni mentioned this pull request Sep 5, 2020

Tracking Issue for slice_range #76393

Open

4 tasks

RalfJung reviewed Sep 13, 2020

View reviewed changes

RalfJung mentioned this pull request Sep 13, 2020

Fix liballoc test suite for Miri #76662

Merged

dylni mentioned this pull request Oct 5, 2020

Move slice::check_range to RangeBounds #76885

Merged

tmiasko mentioned this pull request Oct 30, 2020

Integer overflow on String::drain() with an inclusive range #72237

Closed

cuviper added this to the 1.48.0 milestone May 2, 2024

Add slice::check_range #75207

Add slice::check_range #75207

Conversation

dylni commented Aug 6, 2020

rust-highfive commented Aug 6, 2020

leonardo-m Aug 6, 2020 • 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

KodrAus commented Aug 12, 2020

dylni commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylni commented Aug 17, 2020

KodrAus 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

dylni commented Aug 24, 2020 • edited Loading

KodrAus commented Sep 4, 2020

bors commented Sep 4, 2020

dylni commented Sep 4, 2020 • edited Loading

bors commented Sep 4, 2020

bors commented Sep 4, 2020

Dylan-DPC-zz commented Sep 4, 2020

bors commented Sep 4, 2020

bors commented Sep 4, 2020

RalfJung commented Sep 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 13, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Sep 13, 2020 • 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

Add `slice::check_range` #75207

Add `slice::check_range` #75207

leonardo-m Aug 6, 2020 •

edited

Loading

dylni commented Aug 24, 2020 •

edited

Loading

dylni commented Sep 4, 2020 •

edited

Loading

RalfJung Sep 13, 2020 •

edited

Loading

RalfJung Sep 13, 2020 •

edited

Loading

RalfJung Sep 13, 2020 •

edited

Loading

RalfJung Sep 13, 2020 •

edited

Loading