Poison address-exposed user variables in debug (#54685)

* Poison address exposed user variables in debug code Fix #13072 * Run jit-format * Use named scratch register and kill it in LSRA * Enable it unconditionally for testing purposes * Remove unnecessary modified reg on ARM * Fix OSR and get rid of test code * Remove a declaration * Undo modified comment and use modulo instead of and * Add a test * Rephrase comment Co-authored-by: Kunal Pathak <[email protected]> * Disable poisoning test on mono * Remove outdated line Co-authored-by: Kunal Pathak <[email protected]>
dotnet · Jul 1, 2021 · b8beed2 · b8beed2
1 parent 09785ef
commit b8beed2
Show file tree

Hide file tree

Showing 10 changed files with 162 additions and 17 deletions.
diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h
@@ -348,6 +348,8 @@ class CodeGen final : public CodeGenInterface
 
     void genAllocLclFrame(unsigned frameSize, regNumber initReg, bool* pInitRegZeroed, regMaskTP maskArgRegsLiveIn);
 
+    void genPoisonFrame(regMaskTP bbRegLiveIn);
+
 #if defined(TARGET_ARM)
 
     bool genInstrWithConstant(

diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp
@@ -12528,3 +12528,65 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const
 }
 #endif // DEBUG
 #endif // USING_VARIABLE_LIVE_RANGE
+
+//-----------------------------------------------------------------------------
+// genPoisonFrame: Generate code that places a recognizable value into address exposed variables.
+//
+// Remarks:
+//   This function emits code to poison address exposed non-zero-inited local variables. We expect this function
+//   to be called when emitting code for the scratch BB that comes right after the prolog.
+//   The variables are poisoned using 0xcdcdcdcd.
+void CodeGen::genPoisonFrame(regMaskTP regLiveIn)
+{
+    assert(compiler->compShouldPoisonFrame());
+    assert((regLiveIn & genRegMask(REG_SCRATCH)) == 0);
+
+    // The first time we need to poison something we will initialize a register to the largest immediate cccccccc that
+    // we can fit.
+    bool hasPoisonImm = false;
+    for (unsigned varNum = 0; varNum < compiler->info.compLocalsCount; varNum++)
+    {
+        LclVarDsc* varDsc = compiler->lvaGetDesc(varNum);
+        if (varDsc->lvIsParam || varDsc->lvMustInit || !varDsc->lvAddrExposed)
+        {
+            continue;
+        }
+
+        assert(varDsc->lvOnFrame);
+
+        if (!hasPoisonImm)
+        {
+#ifdef TARGET_64BIT
+            genSetRegToIcon(REG_SCRATCH, (ssize_t)0xcdcdcdcdcdcdcdcd, TYP_LONG);
+#else
+            genSetRegToIcon(REG_SCRATCH, (ssize_t)0xcdcdcdcd, TYP_INT);
+#endif
+            hasPoisonImm = true;
+        }
+
+// For 64-bit we check if the local is 8-byte aligned. For 32-bit, we assume everything is always 4-byte aligned.
+#ifdef TARGET_64BIT
+        bool fpBased;
+        int  addr = compiler->lvaFrameAddress((int)varNum, &fpBased);
+#else
+        int addr = 0;
+#endif
+        int size = (int)compiler->lvaLclSize(varNum);
+        int end  = addr + size;
+        for (int offs = addr; offs < end;)
+        {
+#ifdef TARGET_64BIT
+            if ((offs % 8) == 0 && end - offs >= 8)
+            {
+                GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, REG_SCRATCH, (int)varNum, offs - addr);
+                offs += 8;
+                continue;
+            }
+#endif
+
+            assert((offs % 4) == 0 && end - offs >= 4);
+            GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, REG_SCRATCH, (int)varNum, offs - addr);
+            offs += 4;
+        }
+    }
+}
diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp
@@ -395,6 +395,13 @@ void CodeGen::genCodeForBBlist()
         compiler->compCurStmt     = nullptr;
         compiler->compCurLifeTree = nullptr;
 
+        // Emit poisoning into scratch BB that comes right after prolog.
+        // We cannot emit this code in the prolog as it might make the prolog too large.
+        if (compiler->compShouldPoisonFrame() && compiler->fgBBisScratch(block))
+        {
+            genPoisonFrame(newLiveRegSet);
+        }
+
         // Traverse the block in linear order, generating code for each node as we
         // as we encounter it.
         CLANG_FORMAT_COMMENT_ANCHOR;

diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h
@@ -9839,6 +9839,16 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
         return (info.compUnmanagedCallCountWithGCTransition > 0);
     }
 
+    // Returns true if address-exposed user variables should be poisoned with a recognizable value
+    bool compShouldPoisonFrame()
+    {
+#ifdef FEATURE_ON_STACK_REPLACEMENT
+        if (opts.IsOSR())
+            return false;
+#endif
+        return !info.compInitMem && opts.compDbgCode;
+    }
+
 #if defined(DEBUG)
 
     void compDispLocalVars();

diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp
@@ -2603,8 +2603,8 @@ void Compiler::fgAddInternal()
     noway_assert(!compIsForInlining());
 
     // The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is
-    // required. Create it here.
-    if (compMethodRequiresPInvokeFrame())
+    // required. Similarly, we need a scratch BB for poisoning. Create it here.
+    if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame())
     {
         fgEnsureFirstBBisScratch();
         fgFirstBB->bbFlags |= BBF_DONT_REMOVE;

diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp
@@ -2333,6 +2333,15 @@ void LinearScan::buildIntervals()
             // assert(block->isRunRarely());
         }
 
+        // For frame poisoning we generate code into scratch BB right after prolog since
+        // otherwise the prolog might become too large. In this case we will put the poison immediate
+        // into the scratch register, so it will be killed here.
+        if (compiler->compShouldPoisonFrame() && compiler->fgFirstBBisScratch() && block == compiler->fgFirstBB)
+        {
+            addRefsForPhysRegMask(genRegMask(REG_SCRATCH), currentLoc + 1, RefTypeKill, true);
+            currentLoc += 2;
+        }
+
         LIR::Range& blockRange = LIR::AsRange(block);
         for (GenTree* node : blockRange)
         {

diff --git a/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs b/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs
@@ -153,13 +153,6 @@ private ulong Low64
                 1e80
             };
 
-            // Used to fill uninitialized stack variables with non-zero pattern in debug builds
-            [Conditional("DEBUG")]
-            private static unsafe void DebugPoison<T>(ref T s) where T : unmanaged
-            {
-                MemoryMarshal.AsBytes(MemoryMarshal.CreateSpan(ref s, 1)).Fill(0xCD);
-            }
-
 #region Decimal Math Helpers
 
             private static unsafe uint GetExponent(float f)
@@ -990,7 +983,6 @@ internal static unsafe void DecAddSub(ref DecCalc d1, ref DecCalc d2, bool sign)
                     // Have to scale by a bunch. Move the number to a buffer where it has room to grow as it's scaled.
                     //
                     Unsafe.SkipInit(out Buf24 bufNum);
-                    DebugPoison(ref bufNum);
 
                     bufNum.Low64 = low64;
                     bufNum.Mid64 = tmp64;
@@ -1339,7 +1331,6 @@ internal static unsafe void VarDecMul(ref DecCalc d1, ref DecCalc d2)
                 ulong tmp;
                 uint hiProd;
                 Unsafe.SkipInit(out Buf24 bufProd);
-                DebugPoison(ref bufProd);
 
                 if ((d1.High | d1.Mid) == 0)
                 {
@@ -1920,7 +1911,6 @@ internal static int GetHashCode(in decimal d)
             internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2)
             {
                 Unsafe.SkipInit(out Buf12 bufQuo);
-                DebugPoison(ref bufQuo);
 
                 uint power;
                 int curScale;
@@ -2021,7 +2011,6 @@ internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2)
                     // Shift both dividend and divisor left by curScale.
                     //
                     Unsafe.SkipInit(out Buf16 bufRem);
-                    DebugPoison(ref bufRem);
 
                     bufRem.Low64 = d1.Low64 << curScale;
                     bufRem.High64 = (d1.Mid + ((ulong)d1.High << 32)) >> (32 - curScale);
@@ -2090,7 +2079,6 @@ internal static unsafe void VarDecDiv(ref DecCalc d1, ref DecCalc d2)
                         // Start by finishing the shift left by curScale.
                         //
                         Unsafe.SkipInit(out Buf12 bufDivisor);
-                        DebugPoison(ref bufDivisor);
 
                         bufDivisor.Low64 = divisor;
                         bufDivisor.U2 = (uint)((d2.Mid + ((ulong)d2.High << 32)) >> (32 - curScale));
@@ -2243,7 +2231,6 @@ internal static void VarDecMod(ref DecCalc d1, ref DecCalc d2)
                         d1.uflags = d2.uflags;
                         // Try to scale up dividend to match divisor.
                         Unsafe.SkipInit(out Buf12 bufQuo);
-                        DebugPoison(ref bufQuo);
 
                         bufQuo.Low64 = d1.Low64;
                         bufQuo.U2 = d1.High;
@@ -2303,7 +2290,6 @@ private static unsafe void VarDecModFull(ref DecCalc d1, ref DecCalc d2, int sca
                 int shift = BitOperations.LeadingZeroCount(tmp);
 
                 Unsafe.SkipInit(out Buf28 b);
-                DebugPoison(ref b);
 
                 b.Buf24.Low64 = d1.Low64 << shift;
                 b.Buf24.Mid64 = (d1.Mid + ((ulong)d1.High << 32)) >> (32 - shift);
@@ -2358,7 +2344,6 @@ private static unsafe void VarDecModFull(ref DecCalc d1, ref DecCalc d2, int sca
                 else
                 {
                     Unsafe.SkipInit(out Buf12 bufDivisor);
-                    DebugPoison(ref bufDivisor);
 
                     bufDivisor.Low64 = d2.Low64 << shift;
                     bufDivisor.U2 = (uint)((d2.Mid + ((ulong)d2.High << 32)) >> (32 - shift));

diff --git a/src/tests/JIT/Directed/debugging/poison.cs b/src/tests/JIT/Directed/debugging/poison.cs
@@ -0,0 +1,56 @@
+using System;
+using System.Runtime.CompilerServices;
+
+public class Program
+{
+    [SkipLocalsInit]
+    public static unsafe int Main()
+    {
+        bool result = true;
+
+        int poisoned;
+        Unsafe.SkipInit(out poisoned);
+        result &= VerifyPoison(&poisoned, sizeof(int));
+
+        GCRef zeroed;
+        Unsafe.SkipInit(out zeroed);
+        result &= VerifyZero(Unsafe.AsPointer(ref zeroed), Unsafe.SizeOf<GCRef>());
+
+        WithoutGCRef poisoned2;
+        Unsafe.SkipInit(out poisoned2);
+        result &= VerifyPoison(&poisoned2, sizeof(WithoutGCRef));
+
+        return result ? 100 : 101;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static unsafe bool VerifyPoison(void* val, int size)
+        => AllEq(new Span<byte>(val, size), 0xCD);
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static unsafe bool VerifyZero(void* val, int size)
+        => AllEq(new Span<byte>(val, size), 0);
+
+    private static unsafe bool AllEq(Span<byte> span, byte byteVal)
+    {
+        foreach (byte b in span)
+        {
+            if (b != byteVal)
+                return false;
+        }
+
+        return true;
+    }
+
+    private struct GCRef
+    {
+        public object ARef;
+    }
+
+    private struct WithoutGCRef
+    {
+        public double ADouble;
+        public int ANumber;
+        public float AFloat;
+    }
+}
diff --git a/src/tests/JIT/Directed/debugging/poison.csproj b/src/tests/JIT/Directed/debugging/poison.csproj
@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>False</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/issues.targets b/src/tests/issues.targets
@@ -1650,6 +1650,9 @@
         <ExcludeList Include = "$(XunitTestBinBase)tracing/eventsource/eventsourcetrace/eventsourcetrace/**">
             <Issue>needs triage</Issue>
         </ExcludeList>
+        <ExcludeList Include = "$(XunitTestBinBase)/JIT/Directed/debugging/poison/**">
+            <Issue>Tests coreclr JIT's debug poisoning of address taken variables</Issue>
+        </ExcludeList>
     </ItemGroup>
 
     <!-- Known failures for mono runtime on *all* architectures/operating systems in interpreter runtime mode -->