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

Improve soundness of rustc_data_structures #97707

Merged
merged 2 commits into from
Jun 5, 2022

Conversation

Noratrieb
Copy link
Member

Make it runnable in miri by adding some ignores and changing N in miri. Also fix a stacked borrows issue in sip128.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 3, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2022
@cjgillot
Copy link
Contributor

cjgillot commented Jun 4, 2022

Thanks @Nilstrieb. I'm not sure the title accurately represents the PR. I'd go for "Make rustc_data_structures testable through miri."
r=me after that.

@Noratrieb
Copy link
Member Author

It also makes a soundness fix in the second commit, so I think it's more accurate. But I can change it it if you want.

@cjgillot
Copy link
Contributor

cjgillot commented Jun 4, 2022

Ok. The commit messages are clear enough anyway.
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit e869dce8230e6779a0305fd3f393611b7a263fd9 has been approved by cjgillot

@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 Jun 4, 2022
@Noratrieb
Copy link
Member Author

@cjgillot I was able to run an additional test in miri by reducing the number thanks to Ralf, can you approve it again?

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2022

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 9fef644e7e5a5ded13ffefc35e80500c5769badd has been approved by cjgillot

Some tests took too long and owning_ref is fundamentally flawed,
so don't run these tests or run them with a shorter N. This makes
miri with `-Zmiri-strict-provenance` usable to find UB.
It creates the src pointer first, which is then invalidated by a
unique borrow of the destination pointer. Swap the borrows around
to fix this. Found with miri.
@Noratrieb
Copy link
Member Author

You have to approve that again, I fucked up my formatting :)

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2022

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 7e3bee6 has been approved by cjgillot

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2022
…askrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#97609 (Iterate over `maybe_unused_trait_imports` when checking dead trait imports)
 - rust-lang#97688 (test const_copy to make sure bytewise pointer copies are working)
 - rust-lang#97707 (Improve soundness of rustc_data_structures)
 - rust-lang#97731 (Add regresion test for rust-lang#87142)
 - rust-lang#97735 (Don't generate "Impls on Foreign Types" for std)
 - rust-lang#97737 (Fix pretty printing named bound regions under -Zverbose)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0145321 into rust-lang:master Jun 5, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 5, 2022
@Noratrieb Noratrieb deleted the data-structures-ub branch June 5, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants