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

Inliner use of storage statements is unsound #119366

Closed
tmiasko opened this issue Dec 27, 2023 · 2 comments · Fixed by #126154
Closed

Inliner use of storage statements is unsound #119366

tmiasko opened this issue Dec 27, 2023 · 2 comments · Fixed by #126154
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining A-miri Area: The miri tool 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-opsem Relevant to the opsem team

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Dec 27, 2023

The inliner strategy for dealing with storage statements is simple. If a callee local already has some storage statements, they are preserved as is when integrating callee into the caller. There are no new storage statements for such locals.

Turns out this approach is unsound due to peculiar semantics of MIR. It is well defined to return from a function while there are still some live locals. At the same time it is undefined behaviour to execute StorageLive for already live local. Effectively inliner is obliged to end the storage for locals that are still live when callee returns, which it doesn't do at the moment.

Arguably this is more of a bug in MIR semantics, then one in the inliner rust-lang/unsafe-code-guidelines#129 (comment).
@tmiasko tmiasko added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining T-opsem Relevant to the opsem team labels Dec 27, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 27, 2023
@tmiasko tmiasko removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 27, 2023
@fmease fmease added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Dec 28, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 28, 2023
@apiraino
Copy link
Contributor

hey @tmiasko would you like to first T-opsem discuss this? Maybe have this topic on a meeting and figure out a plan to fix this?

@tmiasko tmiasko added the A-miri Area: The miri tool label Dec 28, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 14, 2024
@RalfJung
Copy link
Member

Oh, interesting catch. Cc @rust-lang/opsem

Maybe we should just allow StorageLive on an already live local (and declare it to first implicitly free the local, and then make it live again)...

@RalfJung RalfJung mentioned this issue May 29, 2024
Closed
@RalfJung RalfJung removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 8, 2024
@RalfJung RalfJung mentioned this issue Jun 8, 2024
Merged
jieyouxu added a commit to jieyouxu/rust that referenced this issue Jun 19, 2024
@jieyouxu
Rollup merge of rust-lang#126154 - RalfJung:storage-live, r=compiler-…
1381b0c 
…errors

StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang#99160 (comment)), which also contains the motivation.

Fixes rust-lang#99160
Fixes rust-lang#98896 (by declaring it not-a-bug)
Fixes rust-lang#119366
Fixes rust-lang/unsafe-code-guidelines#129
@bors bors closed this as completed in 035285b Jun 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2024
@rust-timer
Unrolled build for rust-lang#126154
84ece53 
Rollup merge of rust-lang#126154 - RalfJung:storage-live, r=compiler-errors

StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang#99160 (comment)), which also contains the motivation.

Fixes rust-lang#99160
Fixes rust-lang#98896 (by declaring it not-a-bug)
Fixes rust-lang#119366
Fixes rust-lang/unsafe-code-guidelines#129
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 21, 2024
@fmease
Rollup merge of #126154 - RalfJung:storage-live, r=compiler-errors
161309b 
StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang/rust#99160 (comment)), which also contains the motivation.

Fixes rust-lang/rust#99160
Fixes rust-lang/rust#98896 (by declaring it not-a-bug)
Fixes rust-lang/rust#119366
Fixes rust-lang/unsafe-code-guidelines#129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining A-miri Area: The miri tool 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-opsem Relevant to the opsem team
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

StorageLive: refresh storage (instead of UB) when local is already live RalfJung/rust
5 participants
@RalfJung @apiraino @fmease @rustbot @tmiasko