ref_prop: do not collapse a mutable reborrow onto a multi-borrowed place#156896
Closed
invictustitan2 wants to merge 1 commit into
Closed
ref_prop: do not collapse a mutable reborrow onto a multi-borrowed place#156896invictustitan2 wants to merge 1 commit into
invictustitan2 wants to merge 1 commit into
Conversation
Collaborator
|
The Miri subtree was changed cc @rust-lang/miri Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixes rust-lang#132898. ReferencePropagation set `targets[_3] = Pointer(_1.0, true)` for a mutable reference `_3 = &mut _1.0`, so the Replacer would rewrite every `*_3` to `_1.0` — including inside the reborrow `_2 = &raw mut (*_3)`, yielding `_2 = &raw mut _1.0`. That shortens `_2`'s provenance from `_1 → _3 → _2` to `_1 → _2`, which races an independent borrow of the root local (e.g. the 2-phase `&mut _1` for a method call), and the pass introduced Stacked Borrows UB into code that was sound without the optimisation. The "uniqueness" guard (`fully_replaceable_locals`) verifies only that each individual mutable reference is fully replaced; it does not see other borrows of the referent's memory. As the issue notes, an independent borrow of the root with overlapping lifetime breaks the required invariant. Fix: count direct borrows of each local — borrows whose place is rooted at the local with no Deref in its projection (_1, _1.0; not *_1). When propagating a mutable reference whose target is a direct place rooted at a local with more than one such borrow, leave the target as Unknown. The pass still performs its core sound transform (collapsing a raw reborrow of a &mut through the &mut); only the further collapse onto a direct place that aliases another borrow is blocked. This follows the same conservative-bail pattern as the already-merged GVN fix (rust-lang#132527) and CopyProp fix (rust-lang#143509). Regression test: tests/mir-opt/reference_prop_mutable_alias.rs asserts that the &mut survives, the write goes through (*_3), and the unsound (_1.0) = const 42 never appears. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b6b0ad3 to
ff4eee4
Compare
Collaborator
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot)For more information how to resolve CI failures of this job, visit this link. |
Member
|
This looks like vibecoding. I refuse to accept a vibecoded MIR opt change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #132898.
The bug
ReferencePropagationrewrites the mutable two-phase repro from the issue as follows (current master, MIR opt-level 2):That partial collapse is fine, but the pass goes further when
_2's direct uses are absent — collapsing*_3onto the direct place_1.0, so the raw reborrow becomes_2 = &raw mut _1.0(or, equivalently after replacement of all*_3, the write becomes(_1.0) = const 42). This shortens_2's provenance from_1 → _3 → _2to_1 → _2, which races the independent_5 = &mut _1(a 2-phase borrow created for theaddcall), and introduces Stacked Borrows UB into code that was sound without the optimisation. miri confirms this on current nightly.@RalfJung's analysis in the issue identified the cause: the pass's "uniqueness" guard (
fully_replaceable_locals) verifies only that each individual mutable reference is fully replaced, but does not see other pointers derived from the same place. Per his framing:The fix
A small counting sub-analysis (
count_direct_borrows) records, for each local, the number of borrows that directly borrow it — borrows whose place is rooted at the local with noDerefin its projection (_1,_1.0; not*_1). Two such borrows of a local mean a mutable reference to one of its (sub-)places is not the unique path to that memory.In
compute_replacement, when about to settargets[ref] = Pointer(P, needs_unique=true)for a mutable reference whose targetPis a direct place (first projection is notDeref), bail ifdirect_borrow_count[P.local] > 1.The pass still performs its core sound transform (collapsing a raw reborrow through the
&mutit was derived from); only the further collapse onto a direct place that aliases another borrow is blocked. This is the same conservative-bail pattern as the already-mergedgvn: Invalid dereferences for all non-local mutationsDo not unify borrowed locals in CopyProp.Reason about borrowed classes in CopyProp.Production diff is ~19 lines (one helper + a three-line gate).
Why an SB-only fix is still must-fix
On current nightly the pass-introduced UB is detectable under Stacked Borrows but not under Tree Borrows (TB has evolved since 2024 and now accepts the post-pass code on the raw-pointer variant). Per
rust-lang/miri's own README, "the eventual final aliasing model of Rust will be stricter than Tree Borrows ... if you use Tree Borrows, even if your code is accepted today, it might be declared UB in the future. This is much less likely with Stacked Borrows." TB-acceptance is therefore not a valid defence; SB remains the operative model that miri checks by default and that mir-opt must respect, in line with the recent precedents above.Test plan
tests/mir-opt/reference_prop_mutable_alias.rs— asserts_3 = &mut (_1.0)survives, the write goes through(*_3), and the unsound(_1.0) = const 42never appears.src/tools/miri/tests/pass/ref_prop_mutable_alias.rsandsrc/tools/miri/tests/pass/ref_prop_mutable_alias_opt.rs— the two repros from ReferencePropagation introduces UB into code that is accepted by Stacked Borrows #132898 run UB-free under Stacked Borrows withReferencePropagationforce-enabled, both with debug-assertions on (preserving the full reborrow chain) and off (exercising the residual(*_3)propagation)../x test tests/mir-opt— 395/0 (no expectation churn beyond the new test)../x miri— ui_test pass suites 2420/0, 956/0, 329/0; zero "Undefined Behavior" anywhere; no other regressions. (The single unrelated failure in-p std --libisbacktrace::tests::test_debugfailing withunsupported operation: getcwd not available when isolation is enabled, which is a miri default-isolation limitation orthogonal to this fix.)Notes
This patch was drafted with AI assistance (Claude); the diagnosis, the refutation of less-conservative candidate guards, and the verification against the existing test suite were performed locally and the co-authorship is recorded in the commit trailer.
r? @saethlin
cc @RalfJung @rust-lang/wg-mir-opt @rust-lang/opsem