From 4c3b2c54e02832c58437491d7a862f6c150475e6 Mon Sep 17 00:00:00 2001 From: Johan Lorensson Date: Fri, 31 Jul 2026 11:46:47 +0200 Subject: [PATCH] [release/10.0] [mono][interp] Fix miscompile of self-assignment via newobj with alised byref args. (#131586) Fixes Issue https://github.com/dotnet/runtime/issues/122237 main PR https://github.com/dotnet/runtime/pull/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. --- src/mono/mono/mini/interp/transform-opt.c | 5 +- .../JitBlue/Runtime_122237/Runtime_122237.cs | 70 +++++++++++++++++++ .../Runtime_122237/Runtime_122237.csproj | 8 +++ 3 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.csproj diff --git a/src/mono/mono/mini/interp/transform-opt.c b/src/mono/mono/mini/interp/transform-opt.c index 3d9cd1d55055e4..1bf529f313e08f 100644 --- a/src/mono/mono/mini/interp/transform-opt.c +++ b/src/mono/mono/mini/interp/transform-opt.c @@ -3849,9 +3849,12 @@ interp_super_instructions (TransformData *td) if (def->opcode != MINT_DEF_ARG && def->opcode != MINT_PHI && def->opcode != MINT_DEF_TIER_VAR && !(def->flags & INTERP_INST_FLAG_PROTECTED_NEWOBJ)) { int dreg = ins->dreg; + if (var_has_indirects (td, dreg)) { + // Don't bother with indirect locals + } // if var is not ssa or it is a renamed fixed, then we can't replace the dreg // since there can be conflicting liveness, unless the instructions are adjacent - if ((var_is_ssa_form (td, dreg) && !td->vars [dreg].renamed_ssa_fixed) || + else if ((var_is_ssa_form (td, dreg) && !td->vars [dreg].renamed_ssa_fixed) || interp_prev_ins (ins) == def) { def->dreg = dreg; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs b/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs new file mode 100644 index 00000000000000..35441bdb614327 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.cs @@ -0,0 +1,70 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Runtime_122237; + +using Xunit; + +// Self-reassignment through a constructor whose `in` (byref) parameters alias the +// destination: `a = new GEJ(a.x, a.y, a.z, a.infinity)`. Per the ECMA-335 semantics of +// `newobj`, the object must be constructed into a temporary and only then copied to `a`, +// so the constructor observes the *old* value of `a` through the `in` pointers. A copy +// elimination that forwards the constructed value directly into the address-taken `a` +// would make the constructor read the storage it is simultaneously writing, zeroing the +// fields. The loop runs long enough to reach the Mono interpreter's optimized tier. +public readonly struct FE +{ + public readonly uint n0, n1, n2, n3, n4, n5, n6, n7, n8, n9; + public readonly int magnitude; + public readonly bool normalized; + + public FE(uint a0, uint a1, uint a2, uint a3, uint a4, uint a5, uint a6, uint a7, uint a8, uint a9) + { + n0 = a0; n1 = a1; n2 = a2; n3 = a3; n4 = a4; + n5 = a5; n6 = a6; n7 = a7; n8 = a8; n9 = a9; + magnitude = 1; + normalized = true; + } +} + +public readonly struct GEJ +{ + public readonly FE x, y, z; + public readonly bool infinity; + + public GEJ(in FE x, in FE y, in FE z, bool infinity) + { + this.x = x; + this.y = y; + this.z = z; + this.infinity = infinity; + } +} + +public class Runtime_122237 +{ + [Fact] + public static void TestEntryPoint() + { + var a = new GEJ( + new FE(1, 2, 3, 4, 5, 6, 7, 8, 9, 10), + new FE(11, 22, 33, 44, 55, 66, 77, 88, 99, 11), + new FE(21, 22, 23, 24, 25, 26, 27, 28, 29, 210), + false); + + uint expected = a.x.n0; + + int firstBad = -1; + for (int i = 0; i < 5000; i++) + { + a = new GEJ(a.x, a.y, a.z, a.infinity); + if (a.x.n0 != expected) + { + firstBad = i; + break; + } + } + + Assert.Equal(-1, firstBad); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_122237/Runtime_122237.csproj @@ -0,0 +1,8 @@ + + + True + + + + +