From 0725c10cfe131bc4c1f7964b91bb56ac25b1b414 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 21 Aug 2024 16:28:37 -0700 Subject: [PATCH] Convert ValidateObject and ValidateByref. --- .../src/System/StubHelpers.cs | 13 +- src/coreclr/vm/dllimport.cpp | 12 +- src/coreclr/vm/ecalllist.h | 2 - src/coreclr/vm/qcallentrypoints.cpp | 2 + src/coreclr/vm/stubhelpers.cpp | 174 +++++++++--------- src/coreclr/vm/stubhelpers.h | 24 +-- 6 files changed, 102 insertions(+), 125 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs index 61d60437bdda5d..4bdeee34d4f3f3 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs @@ -1621,14 +1621,17 @@ internal static unsafe void LayoutDestroyNativeInternal(object obj, byte* pNativ [MethodImpl(MethodImplOptions.InternalCall)] internal static extern uint CalcVaListSize(IntPtr va_list); - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void ValidateObject(object obj, IntPtr pMD, object pThis); - [MethodImpl(MethodImplOptions.InternalCall)] internal static extern void LogPinnedArgument(IntPtr localDesc, IntPtr nativeArg); - [MethodImpl(MethodImplOptions.InternalCall)] - internal static extern void ValidateByref(IntPtr byref, IntPtr pMD, object pThis); // the byref is pinned so we can safely "cast" it to IntPtr + [LibraryImport(RuntimeHelpers.QCall, EntryPoint="StubHelpers_ValidateObject")] + private static partial void ValidateObject(ObjectHandleOnStack obj, IntPtr pMD); + + internal static void ValidateObject(object obj, IntPtr pMD) + => ValidateObject(ObjectHandleOnStack.Create(ref obj), pMD); + + [LibraryImport(RuntimeHelpers.QCall, EntryPoint="StubHelpers_ValidateByref")] + internal static partial void ValidateByref(IntPtr byref, IntPtr pMD); // the byref is pinned so we can safely "cast" it to IntPtr [Intrinsic] [MethodImpl(MethodImplOptions.InternalCall)] diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 4916afb4124446..c29fe69b386513 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -2287,30 +2287,28 @@ void NDirectStubLinker::EmitValidateLocal(ILCodeStream* pcsEmit, DWORD dwLocalNu if (SF_IsDelegateStub(dwStubFlags)) { - pcsEmit->EmitLoadNullPtr(); pcsEmit->EmitLoadThis(); + pcsEmit->EmitCALL(METHOD__DELEGATE__GET_INVOKE_METHOD, 1, 1); } else if (SF_IsCALLIStub(dwStubFlags)) { pcsEmit->EmitLoadNullPtr(); - pcsEmit->EmitLDNULL(); } else { // P/Invoke, CLR->COM EmitLoadStubContext(pcsEmit, dwStubFlags); - pcsEmit->EmitLDNULL(); } if (fIsByref) { - // StubHelpers.ValidateByref(byref, pMD, pThis) - pcsEmit->EmitCALL(METHOD__STUBHELPERS__VALIDATE_BYREF, 3, 0); + // StubHelpers.ValidateByref(byref, pMD) + pcsEmit->EmitCALL(METHOD__STUBHELPERS__VALIDATE_BYREF, 2, 0); } else { - // StubHelpers.ValidateObject(obj, pMD, pThis) - pcsEmit->EmitCALL(METHOD__STUBHELPERS__VALIDATE_OBJECT, 3, 0); + // StubHelpers.ValidateObject(obj, pMD) + pcsEmit->EmitCALL(METHOD__STUBHELPERS__VALIDATE_OBJECT, 2, 0); } } diff --git a/src/coreclr/vm/ecalllist.h b/src/coreclr/vm/ecalllist.h index fd987efd49fe64..9cadd51c06ae4b 100644 --- a/src/coreclr/vm/ecalllist.h +++ b/src/coreclr/vm/ecalllist.h @@ -451,8 +451,6 @@ FCFuncStart(gStubHelperFuncs) FCFuncElement("GetCOMIPFromRCW", StubHelpers::GetCOMIPFromRCW) #endif // FEATURE_COMINTEROP FCFuncElement("CalcVaListSize", StubHelpers::CalcVaListSize) - FCFuncElement("ValidateObject", StubHelpers::ValidateObject) - FCFuncElement("ValidateByref", StubHelpers::ValidateByref) FCFuncElement("LogPinnedArgument", StubHelpers::LogPinnedArgument) FCFuncElement("GetStubContext", StubHelpers::GetStubContext) FCFuncElement("MulticastDebuggerTraceHelper", StubHelpers::MulticastDebuggerTraceHelper) diff --git a/src/coreclr/vm/qcallentrypoints.cpp b/src/coreclr/vm/qcallentrypoints.cpp index 29cbe5b9f69b6f..108738af6b9cca 100644 --- a/src/coreclr/vm/qcallentrypoints.cpp +++ b/src/coreclr/vm/qcallentrypoints.cpp @@ -427,6 +427,8 @@ static const Entry s_QCall[] = DllImportEntry(StubHelpers_ThrowInteropParamException) DllImportEntry(StubHelpers_MarshalToManagedVaList) DllImportEntry(StubHelpers_MarshalToUnmanagedVaList) + DllImportEntry(StubHelpers_ValidateObject) + DllImportEntry(StubHelpers_ValidateByref) #ifdef PROFILING_SUPPORTED DllImportEntry(StubHelpers_ProfilerBeginTransitionCallback) DllImportEntry(StubHelpers_ProfilerEndTransitionCallback) diff --git a/src/coreclr/vm/stubhelpers.cpp b/src/coreclr/vm/stubhelpers.cpp index ea96453d1c308d..a765b79ad8fd9b 100644 --- a/src/coreclr/vm/stubhelpers.cpp +++ b/src/coreclr/vm/stubhelpers.cpp @@ -28,77 +28,58 @@ #ifdef VERIFY_HEAP -CQuickArray StubHelpers::s_ByrefValidationEntries; -SIZE_T StubHelpers::s_ByrefValidationIndex = 0; -CrstStatic StubHelpers::s_ByrefValidationLock; - -// static -void StubHelpers::Init() +struct ByrefValidationEntry final { - WRAPPER_NO_CONTRACT; - s_ByrefValidationLock.Init(CrstPinnedByrefValidation); -} + void *pByref; // pointer to GC heap + MethodDesc *pMD; // interop MD this byref was passed to +}; -// static -void StubHelpers::ValidateObjectInternal(Object *pObjUNSAFE, BOOL fValidateNextObj) -{ - CONTRACTL - { - NOTHROW; - GC_NOTRIGGER; - MODE_ANY; -} - CONTRACTL_END; - - _ASSERTE(GCHeapUtilities::GetGCHeap()->RuntimeStructuresValid()); - - // validate the object - there's no need to validate next object's - // header since we validate the next object explicitly below - if (pObjUNSAFE) - { - pObjUNSAFE->Validate(/*bDeep=*/ TRUE, /*bVerifyNextHeader=*/ FALSE, /*bVerifySyncBlock=*/ TRUE); - } - - // and the next object as required - if (fValidateNextObj) - { - Object *nextObj = GCHeapUtilities::GetGCHeap()->NextObj(pObjUNSAFE); - if (nextObj != NULL) - { - // Note that the MethodTable of the object (i.e. the pointer at offset 0) can change from - // g_pFreeObjectMethodTable to NULL, from NULL to , or possibly also from - // g_pFreeObjectMethodTable to concurrently while executing this function. - // Once is seen, we believe that the object should pass the Validate check. - // We have to be careful and read the pointer only once to avoid "phantom reads". - MethodTable *pMT = VolatileLoad(nextObj->GetMethodTablePtr()); - if (pMT != NULL && pMT != g_pFreeObjectMethodTable) - { - // do *not* verify the next object's syncblock - the next object is not guaranteed to - // be "alive" so the finalizer thread may have already released its syncblock - nextObj->Validate(/*bDeep=*/ TRUE, /*bVerifyNextHeader=*/ FALSE, /*bVerifySyncBlock=*/ FALSE); - } - } - } -} +static CQuickArray s_ByrefValidationEntries; +static SIZE_T s_ByrefValidationIndex = 0; +static CrstStatic s_ByrefValidationLock; -// static -MethodDesc *StubHelpers::ResolveInteropMethod(Object *pThisUNSAFE, MethodDesc *pMD) +static void ValidateObjectInternal(Object *pObjUNSAFE, BOOL fValidateNextObj) { - WRAPPER_NO_CONTRACT; + CONTRACTL + { + NOTHROW; + GC_NOTRIGGER; + MODE_ANY; + } + CONTRACTL_END; + + _ASSERTE(GCHeapUtilities::GetGCHeap()->RuntimeStructuresValid()); - if (pMD == NULL && pThisUNSAFE != NULL) + // validate the object - there's no need to validate next object's + // header since we validate the next object explicitly below + if (pObjUNSAFE) { - // if this is a call via delegate, get its Invoke method - MethodTable *pMT = pThisUNSAFE->GetMethodTable(); + pObjUNSAFE->Validate(/*bDeep=*/ TRUE, /*bVerifyNextHeader=*/ FALSE, /*bVerifySyncBlock=*/ TRUE); + } - _ASSERTE(pMT->IsDelegate()); - return ((DelegateEEClass *)pMT->GetClass())->GetInvokeMethod(); + // and the next object as required + if (fValidateNextObj) + { + Object *nextObj = GCHeapUtilities::GetGCHeap()->NextObj(pObjUNSAFE); + if (nextObj != NULL) + { + // Note that the MethodTable of the object (i.e. the pointer at offset 0) can change from + // g_pFreeObjectMethodTable to NULL, from NULL to , or possibly also from + // g_pFreeObjectMethodTable to concurrently while executing this function. + // Once is seen, we believe that the object should pass the Validate check. + // We have to be careful and read the pointer only once to avoid "phantom reads". + MethodTable *pMT = VolatileLoad(nextObj->GetMethodTablePtr()); + if (pMT != NULL && pMT != g_pFreeObjectMethodTable) + { + // do *not* verify the next object's syncblock - the next object is not guaranteed to + // be "alive" so the finalizer thread may have already released its syncblock + nextObj->Validate(/*bDeep=*/ TRUE, /*bVerifyNextHeader=*/ FALSE, /*bVerifySyncBlock=*/ FALSE); + } + } } - return pMD; } -// static -void StubHelpers::FormatValidationMessage(MethodDesc *pMD, SString &ssErrorString) +static void FormatValidationMessage(MethodDesc *pMD, SString &ssErrorString) { CONTRACTL { @@ -108,16 +89,16 @@ void StubHelpers::FormatValidationMessage(MethodDesc *pMD, SString &ssErrorStrin } CONTRACTL_END; - ssErrorString.Append(W("Detected managed heap corruption, likely culprit is interop call through ")); + ssErrorString.Append("Detected managed heap corruption, likely culprit is interop call through "); if (pMD == NULL) { // the only case where we don't have interop MD is CALLI - ssErrorString.Append(W("CALLI.")); + ssErrorString.Append("CALLI."); } else { - ssErrorString.Append(W("method '")); + ssErrorString.Append("method '"); StackSString ssClassName; pMD->GetMethodTable()->_GetFullyQualifiedNameForClass(ssClassName); @@ -126,7 +107,7 @@ void StubHelpers::FormatValidationMessage(MethodDesc *pMD, SString &ssErrorStrin ssErrorString.Append(NAMESPACE_SEPARATOR_CHAR); ssErrorString.AppendUTF8(pMD->GetName()); - ssErrorString.Append(W("'.")); + ssErrorString.Append("'."); } } @@ -178,6 +159,15 @@ void StubHelpers::ProcessByrefValidationList() #endif // VERIFY_HEAP +// static +void StubHelpers::Init() +{ + WRAPPER_NO_CONTRACT; +#ifdef VERIFY_HEAP + s_ByrefValidationLock.Init(CrstPinnedByrefValidation); +#endif // VERIFY_HEAP +} + #ifdef FEATURE_COMINTEROP FORCEINLINE static void GetCOMIPFromRCW_ClearFP() @@ -618,55 +608,59 @@ extern "C" void QCALLTYPE StubHelpers_MarshalToUnmanagedVaList(va_list va, DWORD END_QCALL; } -FCIMPL3(void, StubHelpers::ValidateObject, Object *pObjUNSAFE, MethodDesc *pMD, Object *pThisUNSAFE) +extern "C" void QCALLTYPE StubHelpers_ValidateObject(QCall::ObjectHandleOnStack pObj, MethodDesc *pMD) { - FCALL_CONTRACT; + QCALL_CONTRACT; + + BEGIN_QCALL; #ifdef VERIFY_HEAP - HELPER_METHOD_FRAME_BEGIN_0(); + GCX_COOP(); StackSString errorString; EX_TRY { AVInRuntimeImplOkayHolder AVOkay; - // don't validate the next object if a BGC is in progress. we can race with background - // sweep which could make the next object a Free object underneath us if it's dead. - ValidateObjectInternal(pObjUNSAFE, !(GCHeapUtilities::GetGCHeap()->IsConcurrentGCInProgress())); + // don't validate the next object if a BGC is in progress. we can race with background + // sweep which could make the next object a Free object underneath us if it's dead. + ValidateObjectInternal(OBJECTREFToObject(pObj.Get()), !(GCHeapUtilities::GetGCHeap()->IsConcurrentGCInProgress())); } EX_CATCH { - FormatValidationMessage(ResolveInteropMethod(pThisUNSAFE, pMD), errorString); + FormatValidationMessage(pMD, errorString); EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_EXECUTIONENGINE, errorString.GetUnicode()); } EX_END_CATCH_UNREACHABLE; - HELPER_METHOD_FRAME_END(); #else // VERIFY_HEAP - FCUnique(0xa3); - UNREACHABLE_MSG("No validation support without VERIFY_HEAP"); + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_FAILFAST, "No validation support without VERIFY_HEAP"); #endif // VERIFY_HEAP + + END_QCALL; } -FCIMPLEND -FCIMPL3(void, StubHelpers::ValidateByref, void *pByref, MethodDesc *pMD, Object *pThisUNSAFE) +extern "C" void QCALLTYPE StubHelpers_ValidateByref(void *pByref, MethodDesc *pMD) { - FCALL_CONTRACT; - -#ifdef VERIFY_HEAP - // We cannot validate byrefs at this point as code:GCHeap.GetContainingObject could potentially race - // with allocations on other threads. We'll just remember this byref along with the interop MD and - // perform the validation on next GC (see code:StubHelpers.ProcessByrefValidationList). + QCALL_CONTRACT; // Skip byref if is not pointing inside managed heap if (!GCHeapUtilities::GetGCHeap()->IsHeapPointer(pByref)) { return; } + + BEGIN_QCALL; + +#ifdef VERIFY_HEAP + GCX_COOP(); + + // We cannot validate byrefs at this point as code:GCHeap.GetContainingObject could potentially race + // with allocations on other threads. We'll just remember this byref along with the interop MD and + // perform the validation on next GC (see code:StubHelpers.ProcessByrefValidationList). + ByrefValidationEntry entry; entry.pByref = pByref; - entry.pMD = ResolveInteropMethod(pThisUNSAFE, pMD); - - HELPER_METHOD_FRAME_BEGIN_0(); + entry.pMD = pMD; SIZE_T NumOfEntries = 0; { @@ -695,14 +689,12 @@ FCIMPL3(void, StubHelpers::ValidateByref, void *pByref, MethodDesc *pMD, Object // if the list is too big, trigger GC now GCHeapUtilities::GetGCHeap()->GarbageCollect(0); } - - HELPER_METHOD_FRAME_END(); #else // VERIFY_HEAP - FCUnique(0xa4); - UNREACHABLE_MSG("No validation support without VERIFY_HEAP"); + EEPOLICY_HANDLE_FATAL_ERROR_WITH_MESSAGE(COR_E_FAILFAST, "No validation support without VERIFY_HEAP"); #endif // VERIFY_HEAP + + END_QCALL; } -FCIMPLEND FCIMPL0(void*, StubHelpers::GetStubContext) { diff --git a/src/coreclr/vm/stubhelpers.h b/src/coreclr/vm/stubhelpers.h index cd60d263bd1e57..5b0bb00d684ba7 100644 --- a/src/coreclr/vm/stubhelpers.h +++ b/src/coreclr/vm/stubhelpers.h @@ -15,27 +15,10 @@ class StubHelpers { -#ifdef VERIFY_HEAP - struct ByrefValidationEntry - { - void *pByref; // pointer to GC heap - MethodDesc *pMD; // interop MD this byref was passed to - }; - - static CQuickArray s_ByrefValidationEntries; - static SIZE_T s_ByrefValidationIndex; - static CrstStatic s_ByrefValidationLock; - - static void ValidateObjectInternal(Object *pObjUNSAFE, BOOL fValidateNextObj); - static MethodDesc *ResolveInteropMethod(Object *pThisUNSAFE, MethodDesc *pMD); - static void FormatValidationMessage(MethodDesc *pMD, SString &ssErrorString); - public: static void Init(); +#ifdef VERIFY_HEAP static void ProcessByrefValidationList(); -#else // VERIFY_HEAP -public: - static void Init() { LIMITED_METHOD_CONTRACT; } #endif // VERIFY_HEAP //------------------------------------------------------- @@ -55,8 +38,6 @@ class StubHelpers static FCDECL0(void*, GetStubContext); static FCDECL2(void, LogPinnedArgument, MethodDesc *localDesc, Object *nativeArg); static FCDECL1(DWORD, CalcVaListSize, VARARGS *varargs); - static FCDECL3(void, ValidateObject, Object *pObjUNSAFE, MethodDesc *pMD, Object *pThisUNSAFE); - static FCDECL3(void, ValidateByref, void *pByref, MethodDesc *pMD, Object *pThisUNSAFE); static FCDECL2(void, MulticastDebuggerTraceHelper, Object*, INT32); @@ -90,4 +71,7 @@ extern "C" void QCALLTYPE StubHelpers_ThrowInteropParamException(INT resID, INT extern "C" void QCALLTYPE StubHelpers_MarshalToManagedVaList(va_list va, VARARGS* pArgIterator); extern "C" void QCALLTYPE StubHelpers_MarshalToUnmanagedVaList(va_list va, DWORD cbVaListSize, const VARARGS* pArgIterator); +extern "C" void QCALLTYPE StubHelpers_ValidateObject(QCall::ObjectHandleOnStack pObj, MethodDesc *pMD); +extern "C" void QCALLTYPE StubHelpers_ValidateByref(void *pByref, MethodDesc *pMD); + #endif // __STUBHELPERS_h__