fix OOB pointer formed in Vec::index #122761
Conversation
Move the length check to before using `index` with `ptr::add` to prevent an out of bounds pointer from being formed. Fixes rust-lang#122760
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
what the f @bors r rollup |
bors
added
S-waiting-on-bors
|
this is not a recent regression, so not that critical to merge immediately Could you add a test for this such that miri-test-libstd can catch this in the future? |
bors
added
S-waiting-on-author
|
@Nilstrieb I was actually thinking about opening an issue for that as a followup, as I wasn't sure what we need to do to make sure miri's daily testing triggers, but if adding |
|
That should be all that's needed, yes. I did think about just asking for a follow up but there's always the risk that it will be forgotten, and I thought merging this isn't urgent enough for that. But if you add the tests soon (not just opening an issue :3) you can approve it. (also FWIW, Miris daily tests will soon be bors-ly (https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Running.20miri-test-libstd.20in.20merge.20CI)) |
|
Fair enough! Posted #122765 |
|
@bors r=workingjubilee,Nilstrieb |
bors
added
S-waiting-on-bors
Yeah, I added two test cases locally in https://github.com/rust-lang/rust/blob/c86f3ac24f6b62b438c4bdc34ae73e8a1db60234/library/alloc/tests/vec.rs that I ran with miri-test-libstd to verify if the patch fixed the issue. The reason that I didn't add it in this PR was because I didn't know what the policy was for adding test cases just for MIRI. Should I still add them in this PR or is #122765 enough? The only additional test that's different from #122765 is |
…jubilee,Nilstrieb fix OOB pointer formed in Vec::index Move the length check to before using `index` with `ptr::add` to prevent an out of bounds pointer from being formed. Fixes rust-lang#122760
|
This is likely to be perf-sensitive since it was originally introduced in smallvec as an optimization. @bors rollup=never |
Looking at the pr where this optimization was introduced, it seems the main optimization was avoiding the copy when Also, could someone with the necessary permissions trigger the perf run for this PR? I don't have permission to do it. |
Given that it's already approved, it will get an automatic perf run after it merges. If you want to delay that and experiment some more, then we should |
|
Ah my bad. I thought Amanieu's comment meant that it needed a perf run before being approved.
In that case, I'll just rely on the automatic perf run. |
|
cc @nnethercote Is the optimization introduced in SmallVec sound in SmallVec and not here? |
For clarity: No, we don't require a perf run before such a PR is approved. But we do like to see (and what Amanieu did by marking this with |
| @@ -1541,6 1541,9 @@ impl<T, A: Allocator> Vec<T, A> { | |||
| } | |||
|
|
|||
| let len = self.len(); | |||
| if index > len { | |||
There was a problem hiding this comment.
#98755 explains the reasons for the original change. This PR will increase the number of tests in the common index < len case from one to two, and reinstate the zero-length copy in the index == len case.
If the add is the problem, it could be moved so it's only computed after the index < len and index == len tests. Something like this:
// Space for the new element.
if len == self.buf.capacity() {
self.reserve(1);
}
unsafe {
// Do not call `add` until the index is checked, to avoid UB. Also,
// minimize the number of conditional tests, and avoid a
// zero-length copy in the `index == len` case. (All this requires
// the duplicate computation of `p`.)
if index < len {
// Shift everything over to make space. (Duplicating the
// `index`th element into two consecutive places.) Then
// overwrite the first copy of the `index`th element.
let p = self.as_mut_ptr().add(index);
ptr::copy(p, p.add(1), len - index);
ptr::write(p, element);
} else if index == len {
// No elements need shifting. Overwrite the first copy of the
// `index`th element.
let p = self.as_mut_ptr().add(index);
ptr::write(p, element);
} else {
assert_failed(index, len);
}
self.set_len(len 1);
} There was a problem hiding this comment.
That would also avoid the unnecessary extra block :)
There was a problem hiding this comment.
This PR will increase the number of tests in the common index < len case from one to two, and reinstate the zero-length copy in the index == len case.
I believe I kept the original change where the ptr::copy is only called when index < len. I thought that was the main reason for the perf speedup from the original PR, however, I can see the extra check in the common path having an impact on perf.
Should I make a new commit to fold the index > len check back to the original comparison now or wait for a perf run?
There was a problem hiding this comment.
Also, we might get a small speedup in the sense that we no longer potentially resize the Vec if the index is OOB, though obviously it would be silly to try to optimize that at the expense of the non-panicking in-bounds case.
|
@jwong101 If perf comes back lookin' kinda funky on this, can you post a followup? Thanks in advance! |
|
Yeah, I can make the changes that @nnethercote suggested now, as that shouldn't cause any perf regressions(unless LLVM decides to optimize worse for fun if we don't unconditionally emit a gepi). |
|
@jwong101 Best to pursue it in a new PR building atop, as this commit is already queued to merge, and we'll want to see both perf runs anyways. It's possible this has stopped being a relevant optimization as-of LLVM 18. |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1388d7a): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 669.218s -> 669.712s (0.07%) |
Move the length check to before using
indexwithptr::addto prevent an out of bounds pointer from being formed.Fixes #122760