Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix OOB pointer formed in Vec::index #122761

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

jwong101
Copy link
Contributor

Move the length check to before using index with ptr::add to prevent an out of bounds pointer from being formed.

Fixes #122760

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
@rustbot
Copy link
Collaborator

rustbot commented Mar 20, 2024

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot 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 Mar 20, 2024
@workingjubilee
Copy link
Member

what the f
Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 20, 2024

📌 Commit 37718f9 has been approved by workingjubilee

It is now in the queue for this repository.

@bors 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 20, 2024
@Noratrieb
Copy link
Member

this is not a recent regression, so not that critical to merge immediately
@bors r-

Could you add a test for this such that miri-test-libstd can catch this in the future?

@bors bors 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 Mar 20, 2024
@workingjubilee
Copy link
Member

@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 #[should_panic] tests is all we need to do, then we should definitely do that.

@Noratrieb
Copy link
Member

Noratrieb commented Mar 20, 2024

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

@workingjubilee
Copy link
Member

Fair enough! Posted #122765

@workingjubilee
Copy link
Member

@bors r=workingjubilee,Nilstrieb

@bors
Copy link
Contributor

bors commented Mar 20, 2024

📌 Commit 37718f9 has been approved by workingjubilee,Nilstrieb

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 20, 2024
@jwong101
Copy link
Contributor Author

Could you add a test for this such that miri-test-libstd can catch this in the future?

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 Vec::with_capacity(8).insert(9, true);.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 20, 2024
…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
@Amanieu
Copy link
Member

Amanieu commented Mar 20, 2024

This is likely to be perf-sensitive since it was originally introduced in smallvec as an optimization.

@bors rollup=never

@jwong101
Copy link
Contributor Author

This is likely to be perf-sensitive since it was originally introduced in smallvec as an servo/rust-smallvec#282.

Looking at the pr where this optimization was introduced, it seems the main optimization was avoiding the copy when index == len. This change preserves that optimization, however, it now checks for index > len before reserving extra space. I can update the PR to unconditionally reserve extra space to preserve those semantics if that has any impact on perf.

Also, could someone with the necessary permissions trigger the perf run for this PR? I don't have permission to do it.

@cuviper
Copy link
Member

cuviper commented Mar 20, 2024

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 r- and then do perf runs on try builds.

@jwong101
Copy link
Contributor Author

jwong101 commented Mar 20, 2024

Ah my bad. I thought Amanieu's comment meant that it needed a perf run before being approved.

Given that it's already approved, it will get an automatic perf run after it merges.

In that case, I'll just rely on the automatic perf run.

@workingjubilee
Copy link
Member

cc @nnethercote Is the optimization introduced in SmallVec sound in SmallVec and not here?

@workingjubilee
Copy link
Member

Answer: no, it is not. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9b3d536d340a9c4e42e487a4ad3d0cac

@pnkfelix
Copy link
Member

Ah my bad. I thought Amanieu's comment meant that it needed a perf run before being approved.

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 rollup-) was such PR's to land on their own, rather than being added to a rollup, to make it easier to identify regressions from such "perf risky" PR's when they do pop up.

@bors
Copy link
Contributor

bors commented Mar 20, 2024

⌛ Testing commit 37718f9 with merge 1388d7a...

@@ -1541,6 +1541,9 @@ impl<T, A: Allocator> Vec<T, A> {
}

let len = self.len();
if index > len {
Copy link
Contributor

@nnethercote nnethercote Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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);                                                
        }          

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also avoid the unnecessary extra block :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@workingjubilee
Copy link
Member

@jwong101 If perf comes back lookin' kinda funky on this, can you post a followup? Thanks in advance!

@jwong101
Copy link
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).

@workingjubilee
Copy link
Member

workingjubilee commented Mar 20, 2024

@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.

@bors
Copy link
Contributor

bors commented Mar 20, 2024

☀️ Test successful - checks-actions
Approved by: workingjubilee,Nilstrieb
Pushing 1388d7a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 20, 2024
@bors bors merged commit 1388d7a into rust-lang:master Mar 20, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 20, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1388d7a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
2.3% [2.0%, 2.5%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-2.8%, -2.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [-2.8%, 2.5%] 3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.218s -> 669.712s (0.07%)
Artifact size: 312.87 MiB -> 312.76 MiB (-0.04%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined Behavior in Vec::insert if passed an index outside of its capacity
10 participants