From ae4dc9cef942258098164beae987589c9d569215 Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Sun, 2 Oct 2022 16:57:04 -0700 Subject: [PATCH] Delete security object from GCInfo encoder/decoder (#76487) Fixes #76482 --- src/coreclr/gcdump/gcdumpnonx86.cpp | 22 +--------------- src/coreclr/gcinfo/gcinfoencoder.cpp | 33 +++--------------------- src/coreclr/inc/eetwain.h | 1 - src/coreclr/inc/gcinfodecoder.h | 4 +-- src/coreclr/inc/gcinfoencoder.h | 6 ++--- src/coreclr/inc/gcinfotypes.h | 1 - src/coreclr/jit/gcencode.cpp | 9 ------- src/coreclr/vm/eetwain.cpp | 49 ------------------------------------ src/coreclr/vm/gcinfodecoder.cpp | 26 +------------------ src/coreclr/vm/stackwalktypes.h | 22 +--------------- 10 files changed, 9 insertions(+), 164 deletions(-) diff --git a/src/coreclr/gcdump/gcdumpnonx86.cpp b/src/coreclr/gcdump/gcdumpnonx86.cpp index d9790a1..f93e9bd 100644 --- a/src/coreclr/gcdump/gcdumpnonx86.cpp +++ b/src/coreclr/gcdump/gcdumpnonx86.cpp @@ -293,8 +293,7 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock, ), 0); - if (NO_SECURITY_OBJECT != hdrdecoder.GetSecurityObjectStackSlot() || - NO_GENERICS_INST_CONTEXT != hdrdecoder.GetGenericsInstContextStackSlot() || + if (NO_GENERICS_INST_CONTEXT != hdrdecoder.GetGenericsInstContextStackSlot() || NO_GS_COOKIE == hdrdecoder.GetGSCookieStackSlot()) { gcPrintf("Prolog size: "); @@ -302,25 +301,6 @@ size_t GCDump::DumpGCTable(PTR_CBYTE gcInfoBlock, gcPrintf("%d\n", prologSize); } - gcPrintf("Security object: "); - if (NO_SECURITY_OBJECT == hdrdecoder.GetSecurityObjectStackSlot()) - { - gcPrintf("\n"); - } - else - { - INT32 ofs = hdrdecoder.GetSecurityObjectStackSlot(); - char sign = '+'; - - if (ofs < 0) - { - sign = '-'; - ofs = -ofs; - } - - gcPrintf("caller.sp%c%x\n", sign, ofs); - } - gcPrintf("GS cookie: "); if (NO_GS_COOKIE == hdrdecoder.GetGSCookieStackSlot()) { diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index 8d01159..d459633 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -473,7 +473,6 @@ GcInfoEncoder::GcInfoEncoder( m_NumCallSites = 0; #endif - m_SecurityObjectStackSlot = NO_SECURITY_OBJECT; m_GSCookieStackSlot = NO_GS_COOKIE; m_GSCookieValidRangeStart = 0; _ASSERTE(sizeof(m_GSCookieValidRangeEnd) == sizeof(UINT32)); @@ -694,18 +693,6 @@ void GcInfoEncoder::SetCodeLength( UINT32 length ) m_CodeLength = length; } - -void GcInfoEncoder::SetSecurityObjectStackSlot( INT32 spOffset ) -{ - _ASSERTE( spOffset != NO_SECURITY_OBJECT ); -#if defined(TARGET_AMD64) - _ASSERTE( spOffset < 0x10 && "The security object cannot reside in an input variable!" ); -#endif - _ASSERTE( m_SecurityObjectStackSlot == NO_SECURITY_OBJECT || m_SecurityObjectStackSlot == spOffset ); - - m_SecurityObjectStackSlot = spOffset; -} - void GcInfoEncoder::SetPrologSize( UINT32 prologSize ) { _ASSERTE(prologSize != 0); @@ -1019,12 +1006,11 @@ void GcInfoEncoder::Build() /////////////////////////////////////////////////////////////////////// - UINT32 hasSecurityObject = (m_SecurityObjectStackSlot != NO_SECURITY_OBJECT); UINT32 hasGSCookie = (m_GSCookieStackSlot != NO_GS_COOKIE); UINT32 hasContextParamType = (m_GenericsInstContextStackSlot != NO_GENERICS_INST_CONTEXT); UINT32 hasReversePInvokeFrame = (m_ReversePInvokeFrameSlot != NO_REVERSE_PINVOKE_FRAME); - BOOL slimHeader = (!m_IsVarArg && !hasSecurityObject && !hasGSCookie && (m_PSPSymStackSlot == NO_PSP_SYM) && + BOOL slimHeader = (!m_IsVarArg && !hasGSCookie && (m_PSPSymStackSlot == NO_PSP_SYM) && !hasContextParamType && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame && ((m_StackBaseRegister == NO_STACK_BASE_REGISTER) || (NORMALIZE_STACK_BASE_REGISTER(m_StackBaseRegister) == 0))) && (m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) && @@ -1052,7 +1038,7 @@ void GcInfoEncoder::Build() { GCINFO_WRITE(m_Info1, 1, 1, FlagsSize); // Fat encoding GCINFO_WRITE(m_Info1, (m_IsVarArg ? 1 : 0), 1, FlagsSize); - GCINFO_WRITE(m_Info1, (hasSecurityObject ? 1 : 0), 1, FlagsSize); + GCINFO_WRITE(m_Info1, 0 /* unused - was hasSecurityObject */, 1, FlagsSize); GCINFO_WRITE(m_Info1, (hasGSCookie ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, ((m_PSPSymStackSlot != NO_PSP_SYM) ? 1 : 0), 1, FlagsSize); GCINFO_WRITE(m_Info1, m_contextParamType, 2, FlagsSize); @@ -1095,7 +1081,7 @@ void GcInfoEncoder::Build() GCINFO_WRITE_VARL_U(m_Info1, normPrologSize-1, NORM_PROLOG_SIZE_ENCBASE, ProEpilogSize); GCINFO_WRITE_VARL_U(m_Info1, normEpilogSize, NORM_EPILOG_SIZE_ENCBASE, ProEpilogSize); } - else if (hasSecurityObject || hasContextParamType) + else if (hasContextParamType) { _ASSERTE(!slimHeader); // Save the prolog size, to be used for determining when it is not safe @@ -1107,19 +1093,6 @@ void GcInfoEncoder::Build() GCINFO_WRITE_VARL_U(m_Info1, normPrologSize-1, NORM_PROLOG_SIZE_ENCBASE, ProEpilogSize); } - // Encode the offset to the security object. - if(hasSecurityObject) - { - _ASSERTE(!slimHeader); -#ifdef _DEBUG - LOG((LF_GCINFO, LL_INFO1000, "Security object at " FMT_STK "\n", - DBG_STK(m_SecurityObjectStackSlot) - )); -#endif - - GCINFO_WRITE_VARL_S(m_Info1, NORMALIZE_STACK_SLOT(m_SecurityObjectStackSlot), SECURITY_OBJECT_STACK_SLOT_ENCBASE, SecObjSize); - } - // Encode the offset to the GS cookie. if(hasGSCookie) { diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h index aba7a14..9d75c2f 100644 --- a/src/coreclr/inc/eetwain.h +++ b/src/coreclr/inc/eetwain.h @@ -701,7 +701,6 @@ struct hdrInfo bool doubleAlign; // is the stack double-aligned? locals addressed relative to ESP, and arguments relative to EBP bool interruptible; // intr. at all times (excluding prolog/epilog), not just call sites - bool securityCheck; // has a slot for security object bool handlers; // has callable handlers bool localloc; // uses localloc bool editNcontinue; // has been compiled in EnC mode diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index 3a7d310..93bdd33 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -224,7 +224,7 @@ enum GcInfoDecoderFlags enum GcInfoHeaderFlags { GC_INFO_IS_VARARG = 0x1, - GC_INFO_HAS_SECURITY_OBJECT = 0x2, + // unused = 0x2, // was GC_INFO_HAS_SECURITY_OBJECT GC_INFO_HAS_GS_COOKIE = 0x4, GC_INFO_HAS_PSP_SYM = 0x8, GC_INFO_HAS_GENERICS_INST_CONTEXT_MASK = 0x30, @@ -528,7 +528,6 @@ public: // Miscellaneous method information //------------------------------------------------------------------------ - INT32 GetSecurityObjectStackSlot(); INT32 GetGSCookieStackSlot(); UINT32 GetGSCookieValidRangeStart(); UINT32 GetGSCookieValidRangeEnd(); @@ -571,7 +570,6 @@ private: #elif defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) bool m_HasTailCalls; #endif // TARGET_AMD64 - INT32 m_SecurityObjectStackSlot; INT32 m_GSCookieStackSlot; INT32 m_ReversePInvokeFrameStackSlot; UINT32 m_ValidRangeStart; diff --git a/src/coreclr/inc/gcinfoencoder.h b/src/coreclr/inc/gcinfoencoder.h index dd438a7..33c1054 100644 --- a/src/coreclr/inc/gcinfoencoder.h +++ b/src/coreclr/inc/gcinfoencoder.h @@ -21,7 +21,7 @@ Fat Header for other cases: - EncodingType[Fat] - Flag: isVarArg, - hasSecurityObject, + unused (was hasSecurityObject), hasGSCookie, hasPSPSymStackSlot, hasGenericsInstContextStackSlot, @@ -32,7 +32,7 @@ hasReversePInvokeFrame, - ReturnKind (Fat: 4 bits) - CodeLength - - Prolog (if hasSecurityObject || hasGenericsInstContextStackSlot || hasGSCookie) + - Prolog (if hasGenericsInstContextStackSlot || hasGSCookie) - Epilog (if hasGSCookie) - SecurityObjectStackSlot (if any) - GSCookieStackSlot (if any) @@ -420,7 +420,6 @@ public: // Miscellaneous method information //------------------------------------------------------------------------ - void SetSecurityObjectStackSlot( INT32 spOffset ); void SetPrologSize( UINT32 prologSize ); void SetGSCookieStackSlot( INT32 spOffsetGSCookie, UINT32 validRangeStart, UINT32 validRangeEnd ); void SetPSPSymStackSlot( INT32 spOffsetPSPSym ); @@ -502,7 +501,6 @@ private: #elif defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) bool m_HasTailCalls; #endif // TARGET_AMD64 - INT32 m_SecurityObjectStackSlot; INT32 m_GSCookieStackSlot; UINT32 m_GSCookieValidRangeStart; UINT32 m_GSCookieValidRangeEnd; diff --git a/src/coreclr/inc/gcinfotypes.h b/src/coreclr/inc/gcinfotypes.h index 454babf..57cae4d 100644 --- a/src/coreclr/inc/gcinfotypes.h +++ b/src/coreclr/inc/gcinfotypes.h @@ -596,7 +596,6 @@ void FASTCALL decodeCallPattern(int pattern, // Stack offsets must be 8-byte aligned, so we use this unaligned // offset to represent that the method doesn't have a security object -#define NO_SECURITY_OBJECT (-1) #define NO_GS_COOKIE (-1) #define NO_STACK_BASE_REGISTER (0xffffffff) #define NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA (0xffffffff) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index e85066b6..99ddf4f 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -3793,15 +3793,6 @@ public: } } - void SetSecurityObjectStackSlot(INT32 spOffset) - { - m_gcInfoEncoder->SetSecurityObjectStackSlot(spOffset); - if (m_doLogging) - { - printf("Set security object stack slot to %d.\n", spOffset); - } - } - void SetIsVarArg() { m_gcInfoEncoder->SetIsVarArg(); diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index a45336c..a6eb212 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -252,7 +252,6 @@ size_t DecodeGCHdrInfo(GCInfoToken gcInfoToken, infoPtr->revPInvokeOffset = header.revPInvokeOffset; infoPtr->doubleAlign = header.doubleAlign; - infoPtr->securityCheck = header.security; infoPtr->handlers = header.handlers; infoPtr->localloc = header.localloc; infoPtr->editNcontinue = header.editNcontinue; @@ -378,19 +377,6 @@ size_t DecodeGCHdrInfo(GCInfoToken gcInfoToken, // We do a "pop eax; jmp eax" to return from a fault or finally handler const size_t END_FIN_POP_STACK = sizeof(TADDR); - -// The offset (in bytes) from EBP for the secutiy object on the stack -inline size_t GetSecurityObjectOffset(hdrInfo * info) -{ - LIMITED_METHOD_DAC_CONTRACT; - - _ASSERTE(info->securityCheck && info->ebpFrame); - - unsigned position = info->savedRegsCountExclFP + - 1; - return position * sizeof(TADDR); -} - inline size_t GetLocallocSPOffset(hdrInfo * info) { @@ -399,7 +385,6 @@ size_t GetLocallocSPOffset(hdrInfo * info) _ASSERTE(info->localloc && info->ebpFrame); unsigned position = info->savedRegsCountExclFP + - info->securityCheck + 1; return position * sizeof(TADDR); } @@ -412,7 +397,6 @@ size_t GetParamTypeArgOffset(hdrInfo * info) _ASSERTE((info->genericsContext || info->handlers) && info->ebpFrame); unsigned position = info->savedRegsCountExclFP + - info->securityCheck + info->localloc + 1; // For CORINFO_GENERICS_CTXT_FROM_PARAMTYPEARG return position * sizeof(TADDR); @@ -906,9 +890,6 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, /* @TODO: Check if we have grown out of space for locals, in the face of localloc */ _ASSERTE(!oldInfo.localloc && !newInfo.localloc); - // Always reserve space for the securityCheck slot - _ASSERTE(oldInfo.securityCheck && newInfo.securityCheck); - // @TODO: If nesting level grows above the MAX_EnC_HANDLER_NESTING_LEVEL, // we should return EnC_NESTED_HANLDERS _ASSERTE(oldInfo.handlers && newInfo.handlers); @@ -1046,14 +1027,6 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, TADDR callerSP = oldStackBase + oldFixedStackSize; - // If the old code saved a security object, store the object's reference now. - OBJECTREF securityObject = NULL; - INT32 nOldSecurityObjectStackSlot = oldGcDecoder.GetSecurityObjectStackSlot(); - if (nOldSecurityObjectStackSlot != NO_SECURITY_OBJECT) - { - securityObject = ObjectToOBJECTREF(*PTR_PTR_Object(callerSP + nOldSecurityObjectStackSlot)); - } - #ifdef _DEBUG // If the old method has a PSPSym, then its value should == FP for x64 and callerSP for arm64 INT32 nOldPspSymStackSlot = oldGcDecoder.GetPSPSymStackSlot(); @@ -1434,19 +1407,6 @@ HRESULT EECodeManager::FixContextForEnC(PCONTEXT pCtx, #elif defined(TARGET_AMD64) || defined(TARGET_ARM64) memset((void*)newStackBase, 0, newFixedStackSize - frameHeaderSize); - // On AMD64/ARM64, after zeroing out the stack, restore the security object and PSPSym... - - // There is no relationship we can guarantee between the old code having a security - // object and the new code having a security object. If the new code does have a - // security object, then we copy over the old security object's reference if there - // was one (else we copy over NULL, which is fine). If the new code doesn't have a - // security object, we do nothing. - INT32 nNewSecurityObjectStackSlot = newGcDecoder.GetSecurityObjectStackSlot(); - if (nNewSecurityObjectStackSlot != NO_SECURITY_OBJECT) - { - *PTR_PTR_Object(callerSP + nNewSecurityObjectStackSlot) = OBJECTREFToObject(securityObject); - } - // Restore PSPSym for the new function. Its value should be set to our new FP. But // first, we gotta find PSPSym's location on the stack INT32 nNewPspSymStackSlot = newGcDecoder.GetPSPSymStackSlot(); @@ -4263,15 +4223,6 @@ bool UnwindStackFrame(PREGDISPLAY pContext, if (pUnwindInfo != NULL) { - pUnwindInfo->securityObjectOffset = 0; - if (info->securityCheck) - { - _ASSERTE(info->ebpFrame); - SIZE_T securityObjectOffset = (GetSecurityObjectOffset(info) / sizeof(void*)); - _ASSERTE(securityObjectOffset != 0); - pUnwindInfo->securityObjectOffset = DWORD(securityObjectOffset); - } - pUnwindInfo->fUseEbpAsFrameReg = info->ebpFrame; pUnwindInfo->fUseEbp = ((info->savedRegMask & RM_EBP) != 0); } diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index f69513e..67fbb3b 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -125,7 +125,6 @@ GcInfoDecoder::GcInfoDecoder( } m_IsVarArg = headerFlags & GC_INFO_IS_VARARG; - int hasSecurityObject = headerFlags & GC_INFO_HAS_SECURITY_OBJECT; int hasGSCookie = headerFlags & GC_INFO_HAS_GS_COOKIE; int hasPSPSym = headerFlags & GC_INFO_HAS_PSP_SYM; int hasGenericsInstContext = (headerFlags & GC_INFO_HAS_GENERICS_INST_CONTEXT_MASK) != GC_INFO_HAS_GENERICS_INST_CONTEXT_NONE; @@ -177,7 +176,7 @@ GcInfoDecoder::GcInfoDecoder( m_ValidRangeEnd = (UINT32) DENORMALIZE_CODE_OFFSET(normCodeLength - normEpilogSize); _ASSERTE(m_ValidRangeStart < m_ValidRangeEnd); } - else if (hasSecurityObject || hasGenericsInstContext) + else if (hasGenericsInstContext) { // Decode prolog information UINT32 normPrologSize = (UINT32) m_Reader.DecodeVarLengthUnsigned(NORM_PROLOG_SIZE_ENCBASE) + 1; @@ -197,23 +196,6 @@ GcInfoDecoder::GcInfoDecoder( return; } - // Decode the offset to the security object. - if(hasSecurityObject) - { - m_SecurityObjectStackSlot = (INT32) DENORMALIZE_STACK_SLOT(m_Reader.DecodeVarLengthSigned(SECURITY_OBJECT_STACK_SLOT_ENCBASE)); - } - else - { - m_SecurityObjectStackSlot = NO_SECURITY_OBJECT; - } - - remainingFlags &= ~DECODE_SECURITY_OBJECT; - if (remainingFlags == 0) - { - // Bail, if we've decoded enough, - return; - } - // Decode the offset to the GS cookie. if(hasGSCookie) { @@ -506,12 +488,6 @@ void GcInfoDecoder::EnumerateInterruptibleRanges ( } } -INT32 GcInfoDecoder::GetSecurityObjectStackSlot() -{ - _ASSERTE( m_Flags & DECODE_SECURITY_OBJECT ); - return m_SecurityObjectStackSlot; -} - INT32 GcInfoDecoder::GetGSCookieStackSlot() { _ASSERTE( m_Flags & DECODE_GS_COOKIE ); diff --git a/src/coreclr/vm/stackwalktypes.h b/src/coreclr/vm/stackwalktypes.h index 640d5f3..601e001 100644 --- a/src/coreclr/vm/stackwalktypes.h +++ b/src/coreclr/vm/stackwalktypes.h @@ -86,7 +86,6 @@ struct StackwalkCacheUnwindInfo ULONG RBPOffset; ULONG RSPOffsetFromUnwindInfo; #else // !TARGET_AMD64 - size_t securityObjectOffset; // offset of SecurityObject. 0 if there is no security object BOOL fUseEbp; // Is EBP modified by the method - either for a frame-pointer or for a scratch-register? BOOL fUseEbpAsFrameReg; // use EBP as the frame pointer? #endif // !TARGET_AMD64 @@ -114,10 +113,9 @@ StackwalkCacheEntry UINT_PTR IP; #if !defined(TARGET_AMD64) WORD ESPOffset:15; // stack offset (frame size + pending arguments + etc) - WORD securityObjectOffset:3;// offset of SecurityObject. 0 if there is no security object WORD fUseEbp:1; // For ESP methods, is EBP touched at all? WORD fUseEbpAsFrameReg:1; // use EBP as the frame register? - WORD argSize:11; // size of args pushed on stack + WORD argSize:15; // size of args pushed on stack #else // TARGET_AMD64 DWORD RSPOffset; DWORD RBPOffset; @@ -136,9 +134,6 @@ StackwalkCacheEntry this->ESPOffset = SPOffset; this->argSize = argSize; - this->securityObjectOffset = (WORD)pUnwindInfo->securityObjectOffset; - _ASSERTE(this->securityObjectOffset == pUnwindInfo->securityObjectOffset); - this->fUseEbp = pUnwindInfo->fUseEbp; this->fUseEbpAsFrameReg = pUnwindInfo->fUseEbpAsFrameReg; _ASSERTE(!fUseEbpAsFrameReg || fUseEbp); @@ -162,20 +157,6 @@ StackwalkCacheEntry #endif // !TARGET_X86 && !TARGET_AMD64 } - inline BOOL HasSecurityObject() - { - LIMITED_METHOD_CONTRACT; - -#if defined(TARGET_X86) - return securityObjectOffset != 0; -#else // !TARGET_X86 - // On AMD64 we don't save anything by grabbing the security object before it is needed. This is because - // we need to crack the GC info in order to find the security object, and to unwind we only need to - // crack the unwind info. - return FALSE; -#endif // !TARGET_X86 - } - inline BOOL IsSafeToUseCache() { LIMITED_METHOD_CONTRACT; @@ -236,7 +217,6 @@ inline StackwalkCacheUnwindInfo::StackwalkCacheUnwindInfo(StackwalkCacheEntry * #if defined(TARGET_AMD64) RBPOffset = pCacheEntry->RBPOffset; #else // !TARGET_AMD64 - securityObjectOffset = pCacheEntry->securityObjectOffset; fUseEbp = pCacheEntry->fUseEbp; fUseEbpAsFrameReg = pCacheEntry->fUseEbpAsFrameReg; #endif // !TARGET_AMD64 -- 2.7.4