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.

Sign up for GitHub

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

Jump to bottom

IndirectlyMutableLocals analysis is unsound in the presence of unsafe code #65006

Closed
ecstatic-morse opened this issue Oct 2, 2019 · 1 comment
Closed

IndirectlyMutableLocals analysis is unsound in the presence of unsafe code #65006

ecstatic-morse opened this issue Oct 2, 2019 · 1 comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 2, 2019 •
edited
Loading

#64470 added an IndirectlyMutableLocals analysis to track whether a local could possibly be mutated through a pointer at a given point in the program. However, this analysis overlooked the fact that a shared reference to a Freeze field of a struct could be converted to a shared reference to a !Freeze field of that same struct by offsetting a pointer.

This does not currently cause any unsoundness in the language, since this analysis is only used in const contexts, where the required operations are forbidden. However, we need to fix this before it becomes possible to take a mutable reference or mutate an UnsafeCell or other !Freeze type in a const context.

#64980 added a test that demonstrates the incorrect behavior.
@jonas-schievink jonas-schievink added A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2019
@ecstatic-morse ecstatic-morse mentioned this issue Oct 2, 2019
Closed
@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Oct 5, 2019 •
edited
Loading

Using ptr::offset to convert a reference to one field into a pointer to another is UB according to the current model of stacked borrows (see rust-lang/unsafe-code-guidelines#134). As a result, the behavior of the test is undefined, and this does not (currently) represent any unsoundness in IndirectlyMutableLocals.

@ecstatic-morse ecstatic-morse mentioned this issue Feb 12, 2020
Merged
bors added a commit that referenced this issue Feb 19, 2020
@bors
Auto merge of #69113 - ecstatic-morse:unified-dataflow-borrowed, r=we…
3a8108d 
…sleywiser

Combine `HaveBeenBorrowedLocals` and `IndirectlyMutableLocals` into one dataflow analysis

This PR began as an attempt to port `HaveBeenBorrowedLocals` to the new dataflow framework (see #68241 for prior art). Along the way, I noticed that it could share most of its code with `IndirectlyMutableLocals` and then found a few bugs in the two analyses:
- Neither one marked locals as borrowed after an `Rvalue::AddressOf`.
- `IndirectlyMutableLocals` was missing a minor fix that `HaveBeenBorrowedLocals` got in #61069. This is not a problem today since it is only used during const-checking, where custom drop glue is forbidden. However, this may change some day.

I decided to combine the two analyses so that they wouldn't diverge in the future while ensuring that they remain distinct types (called `MaybeBorrowedLocals` and `MaybeMutBorrowedLocals` to be consistent with the `Maybe{Un,}InitializedPlaces` naming scheme). I fixed the bugs and switched to exhaustive matching where possible to make them less likely in the future. Finally, I added comments explaining some of the finer points of the transfer function for these analyses (see #61069 and #65006).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Check type of place base (not projection) for Freeze in IndirectlyMutableLocals ecstatic-morse/rust
2 participants
@jonas-schievink @ecstatic-morse