From a943ad5d8fd868570e70b4ca55e4bd1ff54694ab Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 8 Sep 2021 13:36:49 +0200 Subject: [PATCH 1/2] Fix x86 EH for UnmanagedCallersOnly methods There is a bug in the exception handling on x86 when exception is propagated from a method marked with UnmanagedCallersOnly attribute to an immediate caller that's also managed (and calls the method via a native delegate). In this case, there is no native frame in between and the stack walker accidentally skips the InlinedCallFrame processing during first pass of exception handling. The InlinedCallFrame is expected to be processed before the related PInvoke IL stub and it doesn't happen in this case, as a native frame is required to make the stack frame iterator move to the explicit InlinedCallFrame. The fact that it is not processed results in ignoring the upper limit on the stack walk and walking the stack further than expected. This change fixes it by detecting the direct transition from UnmanagedCallersOnly to managed caller and forces the stack frame iterator to process the InlinedCallFrame. --- src/coreclr/vm/ilstubcache.cpp | 1 + src/coreclr/vm/method.cpp | 32 --------------------------- src/coreclr/vm/method.hpp | 29 +++++++++++++++++++++--- src/coreclr/vm/methodtablebuilder.cpp | 21 ++++++++++++++++++ src/coreclr/vm/stackwalk.cpp | 12 ++++++++++ 5 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/coreclr/vm/ilstubcache.cpp b/src/coreclr/vm/ilstubcache.cpp index df989b8d9ef972..5854c4c72ae1bc 100644 --- a/src/coreclr/vm/ilstubcache.cpp +++ b/src/coreclr/vm/ilstubcache.cpp @@ -273,6 +273,7 @@ MethodDesc* ILStubCache::CreateNewMethodDesc(LoaderHeap* pCreationHeap, MethodTa if (SF_IsReverseStub(dwStubFlags)) { pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdReverseStub | DynamicMethodDesc::nomdUnmanagedCallersOnlyStub; + pMD->SetUnmanagedCallersOnly(); pMD->GetILStubResolver()->SetStubType(ILStubResolver::NativeToCLRInteropStub); } else diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index 9185f484d4de41..2e2bcb27f683a6 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -3603,38 +3603,6 @@ void NDirectMethodDesc::InitEarlyBoundNDirectTarget() SetNDirectTarget((LPVOID)target); } -//******************************************************************************* -BOOL MethodDesc::HasUnmanagedCallersOnlyAttribute() -{ - CONTRACTL - { - THROWS; - GC_NOTRIGGER; - FORBID_FAULT; - } - CONTRACTL_END; - - if (IsILStub()) - { - return AsDynamicMethodDesc()->IsUnmanagedCallersOnlyStub(); - } - - HRESULT hr = GetCustomAttribute( - WellKnownAttribute::UnmanagedCallersOnly, - nullptr, - nullptr); - if (hr != S_OK) - { - // See https://github.com/dotnet/runtime/issues/37622 - hr = GetCustomAttribute( - WellKnownAttribute::NativeCallableInternal, - nullptr, - nullptr); - } - - return (hr == S_OK) ? TRUE : FALSE; -} - //******************************************************************************* BOOL MethodDesc::ShouldSuppressGCTransition() { diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index cdd50fc038a727..1ebb36c7e7b8f4 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -147,7 +147,9 @@ enum MethodDescClassification // Method is static mdcStatic = 0x0080, - // unused = 0x0100, + // Method has UnmanagedCallersOnly attribute + mdcUnmanagedCallersOnly = 0x0100, + // unused = 0x0200, // Duplicate method. When a method needs to be placed in multiple slots in the @@ -647,7 +649,18 @@ class MethodDesc return GetMethodTable()->IsInterface(); } - BOOL HasUnmanagedCallersOnlyAttribute(); + inline BOOL HasUnmanagedCallersOnlyAttribute() + { + LIMITED_METHOD_CONTRACT; + return (m_wFlags & mdcUnmanagedCallersOnly); + } + + inline void SetUnmanagedCallersOnly() + { + WRAPPER_NO_CONTRACT; + m_wFlags |= mdcUnmanagedCallersOnly; + } + BOOL ShouldSuppressGCTransition(); #ifdef FEATURE_COMINTEROP @@ -846,15 +859,25 @@ class MethodDesc } HRESULT GetCustomAttribute(WellKnownAttribute attribute, + Module *pModule, const void **ppData, ULONG *pcbData) const { WRAPPER_NO_CONTRACT; - Module *pModule = GetModule(); PREFIX_ASSUME(pModule != NULL); return pModule->GetCustomAttribute(GetMemberDef(), attribute, ppData, pcbData); } + HRESULT GetCustomAttribute(WellKnownAttribute attribute, + const void **ppData, + ULONG *pcbData) const + { + WRAPPER_NO_CONTRACT; + Module *pModule = GetModule(); + PREFIX_ASSUME(pModule != NULL); + return GetCustomAttribute(attribute, pModule, ppData, pcbData); + } + #ifndef DACCESS_COMPILE IMetaDataEmit* GetEmitter() { diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 53bffa8c170c37..659cb4e3e8311f 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -6156,6 +6156,27 @@ MethodTableBuilder::InitMethodDesc( #endif // !_DEBUG pNewMD->SetSynchronized(); + HRESULT hr = pNewMD->GetCustomAttribute( + WellKnownAttribute::UnmanagedCallersOnly, + GetModule(), + nullptr, + nullptr); + + if (hr != S_OK) + { + // See https://github.com/dotnet/runtime/issues/37622 + hr = pNewMD->GetCustomAttribute( + WellKnownAttribute::NativeCallableInternal, + GetModule(), + nullptr, + nullptr); + } + + if (hr == S_OK) + { + pNewMD->SetUnmanagedCallersOnly(); + } + #ifdef _DEBUG pNewMD->m_pszDebugMethodName = (LPUTF8)pszDebugMethodName; pNewMD->m_pszDebugClassName = (LPUTF8)pszDebugClassName; diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index e61802b9849508..2f4900297e08ba 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -3143,6 +3143,18 @@ void StackFrameIterator::PostProcessingForManagedFrames(void) m_frameState = SFITER_NATIVE_MARKER_FRAME; m_crawl.isNativeMarker = true; } +#ifdef TARGET_X86 + else if (m_crawl.pFunc && m_crawl.pFunc->HasUnmanagedCallersOnlyAttribute()) + { + // The managed frame we've unwound from was marked with the UnmanagedCallersOnly attribute. Since we are on a frameless + // frame, that means that the method was called from managed code without any native frames in between. + // On x86, the InlinedCallFrame of the pinvoke would get skipped as we've just unwound to the pinvoke IL stub and + // for this architecture, the inlined call frames are supposed to be processed before the managed frame they are stored in. + // So we force the stack frame iterator to process the InlinedCallFrame before the IL stub. + _ASSERTE(InlinedCallFrame::FrameHasActiveCall(m_crawl.pFrame)); + m_crawl.isFrameless = false; + } +#endif } // StackFrameIterator::PostProcessingForManagedFrames() //--------------------------------------------------------------------------------------- From 36861ee7f64bf9221bc7eeec83911c9294300178 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Wed, 8 Sep 2021 21:36:27 +0200 Subject: [PATCH 2/2] Reflect PR feedback, use GCInfo instead of parsing the attribute --- src/coreclr/vm/ilstubcache.cpp | 1 - src/coreclr/vm/method.cpp | 32 +++++++++++++++++++++++++++ src/coreclr/vm/method.hpp | 29 +++--------------------- src/coreclr/vm/methodtablebuilder.cpp | 21 ------------------ src/coreclr/vm/stackwalk.cpp | 14 +++++++++--- 5 files changed, 46 insertions(+), 51 deletions(-) diff --git a/src/coreclr/vm/ilstubcache.cpp b/src/coreclr/vm/ilstubcache.cpp index 5854c4c72ae1bc..df989b8d9ef972 100644 --- a/src/coreclr/vm/ilstubcache.cpp +++ b/src/coreclr/vm/ilstubcache.cpp @@ -273,7 +273,6 @@ MethodDesc* ILStubCache::CreateNewMethodDesc(LoaderHeap* pCreationHeap, MethodTa if (SF_IsReverseStub(dwStubFlags)) { pMD->m_dwExtendedFlags |= DynamicMethodDesc::nomdReverseStub | DynamicMethodDesc::nomdUnmanagedCallersOnlyStub; - pMD->SetUnmanagedCallersOnly(); pMD->GetILStubResolver()->SetStubType(ILStubResolver::NativeToCLRInteropStub); } else diff --git a/src/coreclr/vm/method.cpp b/src/coreclr/vm/method.cpp index 2e2bcb27f683a6..9185f484d4de41 100644 --- a/src/coreclr/vm/method.cpp +++ b/src/coreclr/vm/method.cpp @@ -3603,6 +3603,38 @@ void NDirectMethodDesc::InitEarlyBoundNDirectTarget() SetNDirectTarget((LPVOID)target); } +//******************************************************************************* +BOOL MethodDesc::HasUnmanagedCallersOnlyAttribute() +{ + CONTRACTL + { + THROWS; + GC_NOTRIGGER; + FORBID_FAULT; + } + CONTRACTL_END; + + if (IsILStub()) + { + return AsDynamicMethodDesc()->IsUnmanagedCallersOnlyStub(); + } + + HRESULT hr = GetCustomAttribute( + WellKnownAttribute::UnmanagedCallersOnly, + nullptr, + nullptr); + if (hr != S_OK) + { + // See https://github.com/dotnet/runtime/issues/37622 + hr = GetCustomAttribute( + WellKnownAttribute::NativeCallableInternal, + nullptr, + nullptr); + } + + return (hr == S_OK) ? TRUE : FALSE; +} + //******************************************************************************* BOOL MethodDesc::ShouldSuppressGCTransition() { diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp index 1ebb36c7e7b8f4..cdd50fc038a727 100644 --- a/src/coreclr/vm/method.hpp +++ b/src/coreclr/vm/method.hpp @@ -147,9 +147,7 @@ enum MethodDescClassification // Method is static mdcStatic = 0x0080, - // Method has UnmanagedCallersOnly attribute - mdcUnmanagedCallersOnly = 0x0100, - + // unused = 0x0100, // unused = 0x0200, // Duplicate method. When a method needs to be placed in multiple slots in the @@ -649,18 +647,7 @@ class MethodDesc return GetMethodTable()->IsInterface(); } - inline BOOL HasUnmanagedCallersOnlyAttribute() - { - LIMITED_METHOD_CONTRACT; - return (m_wFlags & mdcUnmanagedCallersOnly); - } - - inline void SetUnmanagedCallersOnly() - { - WRAPPER_NO_CONTRACT; - m_wFlags |= mdcUnmanagedCallersOnly; - } - + BOOL HasUnmanagedCallersOnlyAttribute(); BOOL ShouldSuppressGCTransition(); #ifdef FEATURE_COMINTEROP @@ -858,16 +845,6 @@ class MethodDesc return pModule->GetMDImport(); } - HRESULT GetCustomAttribute(WellKnownAttribute attribute, - Module *pModule, - const void **ppData, - ULONG *pcbData) const - { - WRAPPER_NO_CONTRACT; - PREFIX_ASSUME(pModule != NULL); - return pModule->GetCustomAttribute(GetMemberDef(), attribute, ppData, pcbData); - } - HRESULT GetCustomAttribute(WellKnownAttribute attribute, const void **ppData, ULONG *pcbData) const @@ -875,7 +852,7 @@ class MethodDesc WRAPPER_NO_CONTRACT; Module *pModule = GetModule(); PREFIX_ASSUME(pModule != NULL); - return GetCustomAttribute(attribute, pModule, ppData, pcbData); + return pModule->GetCustomAttribute(GetMemberDef(), attribute, ppData, pcbData); } #ifndef DACCESS_COMPILE diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 659cb4e3e8311f..53bffa8c170c37 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -6156,27 +6156,6 @@ MethodTableBuilder::InitMethodDesc( #endif // !_DEBUG pNewMD->SetSynchronized(); - HRESULT hr = pNewMD->GetCustomAttribute( - WellKnownAttribute::UnmanagedCallersOnly, - GetModule(), - nullptr, - nullptr); - - if (hr != S_OK) - { - // See https://github.com/dotnet/runtime/issues/37622 - hr = pNewMD->GetCustomAttribute( - WellKnownAttribute::NativeCallableInternal, - GetModule(), - nullptr, - nullptr); - } - - if (hr == S_OK) - { - pNewMD->SetUnmanagedCallersOnly(); - } - #ifdef _DEBUG pNewMD->m_pszDebugMethodName = (LPUTF8)pszDebugMethodName; pNewMD->m_pszDebugClassName = (LPUTF8)pszDebugClassName; diff --git a/src/coreclr/vm/stackwalk.cpp b/src/coreclr/vm/stackwalk.cpp index 2f4900297e08ba..45bdf3088db074 100644 --- a/src/coreclr/vm/stackwalk.cpp +++ b/src/coreclr/vm/stackwalk.cpp @@ -18,6 +18,8 @@ #include "interpreter.h" #endif // FEATURE_INTERPRETER +#include "gcinfodecoder.h" + #ifdef FEATURE_EH_FUNCLETS #define PROCESS_EXPLICIT_FRAME_BEFORE_MANAGED_FRAME #endif @@ -3135,6 +3137,12 @@ void StackFrameIterator::PostProcessingForManagedFrames(void) m_exInfoWalk.WalkToPosition(GetRegdisplaySP(m_crawl.pRD), (m_flags & POPFRAMES)); #endif // ELIMINATE_FEF +#ifdef TARGET_X86 + hdrInfo gcHdrInfo; + DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo); + bool hasReversePInvoke = gcHdrInfo.revPInvokeOffset != INVALID_REV_PINVOKE_OFFSET; +#endif // TARGET_X86 + ProcessIp(GetControlPC(m_crawl.pRD)); // if we have unwound to a native stack frame, stop and set the frame state accordingly @@ -3143,10 +3151,10 @@ void StackFrameIterator::PostProcessingForManagedFrames(void) m_frameState = SFITER_NATIVE_MARKER_FRAME; m_crawl.isNativeMarker = true; } -#ifdef TARGET_X86 - else if (m_crawl.pFunc && m_crawl.pFunc->HasUnmanagedCallersOnlyAttribute()) +#ifdef TARGET_X86 + else if (hasReversePInvoke) { - // The managed frame we've unwound from was marked with the UnmanagedCallersOnly attribute. Since we are on a frameless + // The managed frame we've unwound from had reverse PInvoke frame. Since we are on a frameless // frame, that means that the method was called from managed code without any native frames in between. // On x86, the InlinedCallFrame of the pinvoke would get skipped as we've just unwound to the pinvoke IL stub and // for this architecture, the inlined call frames are supposed to be processed before the managed frame they are stored in.