Github Fix invalid slice access in String::retain by SkiFire13 · Pull Request #8...
source link: https://github.com/rust-lang/rust/pull/82554
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
As noted in #78499, the previous fix was technically still unsound because it accessed elements of a slice outside its bounds (even though they were still inside the same allocation). This PR addresses that concern by switching to a dropguard approach.
r? @m-ou-se
(rust-highfive has picked a reviewer for you, use r? to override)
somewhat suspicious - it's calling get_unchecked on a &str obtained via auto-deref which is zero-length now. So we more or less expect this needs to go through a raw pointer directly or something similar.
Yeah, that seems potentially problematic. Until we have a better idea of what the aliasing model will be long-term, it's better to be conservative and always remain in-bounds even for dynamically sized references.
I haven't carefully checked the code here, but a set-len-on-drop guard is a common approach elsewhere in the stdlib. Is this function covered by a liballoc test case? If yes, any idea why Miri is not complaining about this? If it is possible to cause a Miri error with the current implementation of that function, it'd be good to add that as a testcase.
The function was covered by this test, but it contains a catch_unwind (also added by #78499) that makes it not runnable in miri.
However even when run without the catch_unwind part it doesn't trigger miri. Heck, even the following program, which seems obviously wrong, doesn't trigger miri:
fn main() {
unsafe {
let mut v = vec![1, 2, 3];
v.set_len(0);
assert_eq!(*v.get_unchecked(1), 2);
let a = [1, 2, 3];
let s = &a[0..0];
assert_eq!(s.len(), 0);
assert_eq!(*s.get_unchecked(1), 2);
}
}I guess miri doesn't correctly track slices bounds but only the allocation ones?
but it contains a catch_unwind (also added by #78499) that makes it not runnable in miri.
Miri has supported unwinding for more than a year at this point. ;)
I guess miri doesn't correctly track slices bounds but only the allocation ones?
Interesting... I first thought this was just a missing -Zmiri-track-raw-pointers, but that's not it. The following does error though (with -Zmiri-track-raw-pointers):
fn main() {
unsafe {
let a = [1, 2, 3];
let s = &a[0..1];
assert_eq!(s.len(), 1);
assert_eq!(*s.get_unchecked(1), 2);
}
}Something strange is going on with zero-sized references here.
I'm a bit worried that the resulting code here is worse with a drop guard. Did you check?
It might be better to keep the set_len(0) and use std::slice::from_raw_parts and std::str::from_utf8_unchecked to create the slice.
Alternatively, you could keep use Vec::spare_capacity_mut right after set_len(0) together with slice_assume_init_mut() to get the full slice while temporarily having the Vec's len at zero. (You'd have to change the ptr::copy call though.)
I do already have most of an impl of the raw_parts and from_utf8_unchecked version in https://github.com/DJMcNab/retain_more/blob/c87ddd74b8b0c6660781dc9b5bb4b8f8a3187cc5/src/string.rs#L141-L223
I think the safety comments are correct, although the actual reasons that I think alloc::String::retain is broken are wrong. Obviously that impl can be simplified to not expose the additional power that it provides.
I'm a bit worried that the resulting code here is worse with a drop guard. Did you check?
The difference looks pretty minimal, it adds just a couple of methods and some instructions outside the main loop. https://godbolt.org/z/4rqM9P
I forgot to update the labels
@rustbot label -S-waiting-on-author +S-waiting-on-review
Also, I correct my previous statement, it doesn't add code to the function, it just moves the return label.
@bors r+
Commit c89e643 has been approved by m-ou-se
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK