From 4f161de987fa496da237840704260c1d001f5412 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 21 Apr 2021 18:13:37 +0200 Subject: [PATCH] Poison address exposed user variables in debug code Fix #13072 --- src/coreclr/jit/codegen.h | 3 + src/coreclr/jit/codegencommon.cpp | 88 ++++++++++++++++++- src/coreclr/jit/codegenlinear.cpp | 7 ++ src/coreclr/jit/compiler.h | 6 ++ src/coreclr/jit/flowgraph.cpp | 3 +- .../src/System/Decimal.DecCalc.cs | 15 ---- 6 files changed, 105 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 02971fbde2b8c..1cd8d6bef3c6e 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -348,6 +348,9 @@ class CodeGen final : public CodeGenInterface void genAllocLclFrame(unsigned frameSize, regNumber initReg, bool* pInitRegZeroed, regMaskTP maskArgRegsLiveIn); + void genSetRegsModifiedForPoisoning(); + void genPoisonFrame(regMaskTP bbRegLiveIn); + #if defined(TARGET_ARM) bool genInstrWithConstant( diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index e99f047c061a5..aa4be9c839714 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -4842,7 +4842,7 @@ void CodeGen::genCheckUseBlockInit() } } - // Record number of 4 byte slots that need zeroing. + // Record number of stack slots that need zeroing. genInitStkLclCnt = initStkLclCnt; // Decide if we will do block initialization in the prolog, or use @@ -6733,6 +6733,12 @@ void CodeGen::genFinalizeFrame() } #endif // TARGET_ARM + // When poisoning on ARM we need to use callee-preserved registers. + if (compiler->compShouldPoisonFrame()) + { + genSetRegsModifiedForPoisoning(); + } + #ifdef DEBUG if (verbose) { @@ -12528,3 +12534,83 @@ void CodeGenInterface::VariableLiveKeeper::dumpLvaVariableLiveRanges() const } #endif // DEBUG #endif // USING_VARIABLE_LIVE_RANGE + +void CodeGen::genSetRegsModifiedForPoisoning() +{ + // For ARM32 we need a callee-reserved register since the scratch registers may have args. +#if defined(TARGET_ARM) + regSet.rsSetRegsModified(RBM_R4); +#endif +} + +//----------------------------------------------------------------------------- +// 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 0xcccccccc. This value is stored in EAX/RAX on xarch and r/x0 on ARM, +// so we expect that this reg is not live. +void CodeGen::genPoisonFrame(regMaskTP regLiveIn) +{ +#if defined(TARGET_XARCH) + const regNumber immRegNum = REG_EAX; +#elif defined(TARGET_ARM) + const regNumber immRegNum = REG_R4; +#elif defined(TARGET_ARM64) + const regNumber immRegNum = REG_R9; +#else + return; +#endif + + assert(compiler->compShouldPoisonFrame()); + assert((regLiveIn & genRegMask(immRegNum)) == 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 + instGen_Set_Reg_To_Imm(EA_8BYTE, immRegNum, (ssize_t)0xcccccccccccccccc); +#else + instGen_Set_Reg_To_Imm(EA_4BYTE, immRegNum, (ssize_t)0xcccccccc); +#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 & 7) == 0 && end - offs >= 8) + { + GetEmitter()->emitIns_S_R(ins_Store(TYP_LONG), EA_8BYTE, immRegNum, (int)varNum, offs - addr); + offs += 8; + continue; + } +#endif + + assert((offs & 3) == 0 && end - offs >= 4); + GetEmitter()->emitIns_S_R(ins_Store(TYP_INT), EA_4BYTE, immRegNum, (int)varNum, offs - addr); + offs += 4; + } + } +} diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 796c128b98eac..8bf1f852ffd4d 100644 --- 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 index fe3f987667d38..914e3424fa742 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9828,6 +9828,12 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX return (info.compUnmanagedCallCountWithGCTransition > 0); } + // Returns true if address-exposed user variables should be poisoned with a recognizable value + bool compShouldPoisonFrame() + { + return !info.compInitMem && opts.MinOpts() && opts.compDbgCode; + } + #if defined(DEBUG) void compDispLocalVars(); diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index e7a281cdecaa1..ffe08f967bc95 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -2604,7 +2604,8 @@ void Compiler::fgAddInternal() // 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()) + // Similarly, for poisoning we also need a scratch BB. + if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame()) { fgEnsureFirstBBisScratch(); fgFirstBB->bbFlags |= BBF_DONT_REMOVE; diff --git a/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs b/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs index d56ce8ebb7fc7..a966b35b83c4a 100644 --- 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(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));