[release/10.0] [mono][interp] Fix miscompile of self-assignment via newobj with alised byref args. (#131586) - #131927
Conversation
…ewobj with alised byref args. (dotnet#131586) Fixes Issue dotnet#122237 main PR dotnet#131586 # Description The interpreter's optimized tier (INTERP_OPT_SUPER_INSTRUCTIONS) miscompiled a self-reassignment through a constructor whose `in` (byref) parameters alias the destination, e.g.: a = new GEJ(a.x, a.y, a.z, a.infinity); Per ECMA-335, `newobj` must construct into a temporary and only then copy the result to `a`, so the constructor observes the old value of `a` through the `in` pointers. The `interp_super_instructions` "forward dreg" pass was retargeting the constructed value's store directly into the address-taken local `a` (`def->dreg = dreg`), eliminating the intermediate move. This made the constructor read the very storage it was simultaneously writing, zeroing the fields once the method tiered up to the optimized tier (observed on Android after ~1000 iterations, thats when the interpreter tiering kicks in). Add an address-taken guard (`var_has_indirects`) before the retarget, bailing out when either the source or destination local has had its address taken. This mirrors the existing guard in `interp_cprop` and the Mono JIT `vreg_is_volatile` discipline in local-propagation.c. Add a regression test under JIT/Regression/JitBlue/Runtime_122237. It fails before the fix and passes after, verified under both the default (auto) and forced interpreter tiering modes. The test infrastructure differs between `main` and `release/10.0`: on `main` the test is registered via the merged `Regression_ro_2.csproj`, while `release/10.0` still uses one `.csproj` per test, so this backport adds a standalone `Runtime_122237.csproj` instead. # Customer Impact Fixes a correctness bug in the Mono interpreter's optimized tier that can silently corrupt data (zeroed struct fields) for code that reassigns a local to a new instance of itself through a constructor with `in` (byref) parameters, once the method has tiered up (observed on Android after ~1000 iterations). # Regression Yes, from the introduction of INTERP_OPT_SUPER_INSTRUCTIONS optimized tier. Not a regression from a prior release/10.0 servicing fix. # Testing Added a new regression test (Runtime_122237) that fails before the fix and passes after, verified locally under both default (auto) and forced interpreter tiering modes. Relying on this PR's CI for `release/10.0` validation. # Risk Low. The change adds a targeted guard that only disables a copy-elimination optimization when a local's address has been taken, matching an existing guard used elsewhere in the same optimization pass (`interp_cprop`). # Package authoring signed off? N/A - runtime-only change, no shipping package authoring impact.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @vitek-karas, @BrzVlad, @kotlarmilos |
There was a problem hiding this comment.
Pull request overview
Backport to release/10.0 of a Mono interpreter optimized-tier (INTERP_OPT_SUPER_INSTRUCTIONS) correctness fix: prevent a “forward dreg” copy-elimination from retargeting newobj construction directly into an address-taken local, which can violate newobj temp-then-copy semantics when in (byref) args alias the destination.
Changes:
- Add an address-taken/indirect-local guard in
interp_super_instructionsbefore retargetingdef->dreg. - Add a new JIT regression test
Runtime_122237exercising self-reassignment vianew GEJ(a.x, a.y, ...)withinparameters. - Add a standalone
.csprojfor the new regression test (perrelease/10.0test layout).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform-opt.c | Adds an indirect-local guard around the “forward dreg” optimization in interp_super_instructions. |
| src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs | New regression test reproducing the aliasing self-assignment scenario and asserting stability over many iterations. |
| src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.csproj | New standalone test project file for release/10.0’s per-test csproj structure. |
|
Hi, the code complete date for 10.0.12 (the September 2026 release) is Monday 10 August. Make sure to merge this PR on that date at the latest, or it won't make it into that release. As a reminder, if this is a product change, you also need Tactics approval before merging this PR (test-only or infra-only changes don't require Tactics approval). |
Backport of #131586 to release/10.0
Description
The interpreter's optimized tier (INTERP_OPT_SUPER_INSTRUCTIONS) miscompiled a self-reassignment through a constructor whose
in(byref) parameters alias the destination, e.g.:Per ECMA-335,
newobjmust construct into a temporary and only then copy the result toa, so the constructor observes the old value ofathrough theinpointers. Theinterp_super_instructions"forward dreg" pass was retargeting the constructed value's store directly into the address-taken locala(def->dreg = dreg), eliminating the intermediate move. This made the constructor read the very storage it was simultaneously writing, zeroing the fields once the method tiered up to the optimized tier (observed on Android after ~1000 iterations, thats when the interpreter tiering kicks in).Add an address-taken guard (
var_has_indirects) before the retarget, bailing out when either the source or destination local has had its address taken. This mirrors the existing guard ininterp_cpropand the Mono JITvreg_is_volatilediscipline in local-propagation.c.Add a regression test under JIT/Regression/JitBlue/Runtime_122237. It fails before the fix and passes after, verified under both the default (auto) and forced interpreter tiering modes.
The test infrastructure differs between
mainandrelease/10.0: onmainthe test is registered via the mergedRegression_ro_2.csproj, whilerelease/10.0still uses one.csprojper test, so this backport adds a standaloneRuntime_122237.csprojinstead.Customer Impact
Fixes a correctness bug in the Mono interpreter's optimized tier that can silently corrupt data (zeroed struct fields) for code that reassigns a local to a new instance of itself through a constructor with
in(byref) parameters, once the method has tiered up (observed on Android after ~1000 iterations).Regression
Yes, from the introduction of INTERP_OPT_SUPER_INSTRUCTIONS optimized tier. Not a regression from a prior release/10.0 servicing fix.
Testing
Added a new regression test (Runtime_122237) that fails before the fix and passes after, verified locally under both default (auto) and forced interpreter tiering modes. Relying on this PR's CI for
release/10.0validation.Risk
Low. The change adds a targeted guard that only disables a copy-elimination optimization when a local's address has been taken, matching an existing guard used elsewhere in the same optimization pass (
interp_cprop).