fix OOB pointer formed in Vec::index by jwong101 · Pull Request #122761 · rust-l...
source link: https://github.com/rust-lang/rust/pull/122761
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.
fix OOB pointer formed in Vec::index #122761
Conversation
Contributor
Move the length check to before using index with ptr::add to prevent an out of bounds pointer from being formed.
Fixes #122760
Collaborator
|
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 (
|
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
Contributor
|
what the f @bors r+ rollup |
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
Member
|
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? |
added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
Contributor
|
@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 |
Member
|
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)) |
Contributor
|
Fair enough! Posted #122765 |
Contributor
|
@bors r=workingjubilee,Nilstrieb |
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-author Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Contributor
Author
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 |
Member
|
This is likely to be perf-sensitive since it was originally introduced in smallvec as an optimization. @bors rollup=never |
Contributor
Author
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. |
Member
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 |
Contributor
Author
|
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. |
Contributor
|
cc @nnethercote Is the optimization introduced in SmallVec sound in SmallVec and not here? |
Member
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 { |
Contributor
#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);
} That would also avoid the unnecessary extra block :)
Contributor
Author
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?
Contributor
Author
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.
Contributor
|
@jwong101 If perf comes back lookin' kinda funky on this, can you post a followup? Thanks in advance! |
Contributor
Author
|
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). |
Contributor
|
@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. |
Contributor
|
☀️ Test successful - checks-actions |
Collaborator
|
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)Results 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%) |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
Undefined Behavior in Vec::insert if passed an index outside of its capacity
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK