From 4cdb59e5434d4894452c85e17bceb2c4bab2fc18 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 27 Jan 2020 11:35:36 +0100 Subject: [PATCH 1/4] Fix x86 InlinedCallFrame popping during EH There is a bug in InlinedCallFrame popping on x86 during EH with R2R compiled code. The code is popping this frame if it is at the top of the explicit frames stack before executing catch handler. But it is missing a check to see if the frame was actually in the unwound part of the stack. So in some corner cases, it can end up popping a frame that belongs to an active pinvoke somewhere up the call chain. The fix is to do the popping only if the InlinedCallFrame is located at an address smaller than the resume ESP. --- src/coreclr/src/vm/i386/excepx86.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/vm/i386/excepx86.cpp b/src/coreclr/src/vm/i386/excepx86.cpp index da0dc4fc4b6e05..32510e2ed9535e 100644 --- a/src/coreclr/src/vm/i386/excepx86.cpp +++ b/src/coreclr/src/vm/i386/excepx86.cpp @@ -1868,8 +1868,10 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread) // Sync managed exception state, for the managed thread, based upon any active exception tracker pThread->SyncManagedExceptionState(fIsDebuggerHelperThread); - if (InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) + if ((pThread->m_pFrame < esp) && InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) { + // The current explicit frame is below the resume esp and it is an inlined call frame with an active call. + // // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The From fdd1e11c53f1e7850d3446ac58b8661fb0cdf59b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 27 Jan 2020 15:39:37 +0100 Subject: [PATCH 2/4] Fix typos in a comment close to my change --- src/coreclr/src/vm/i386/excepx86.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/i386/excepx86.cpp b/src/coreclr/src/vm/i386/excepx86.cpp index 32510e2ed9535e..3fb2a51043aef3 100644 --- a/src/coreclr/src/vm/i386/excepx86.cpp +++ b/src/coreclr/src/vm/i386/excepx86.cpp @@ -1875,9 +1875,9 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread) // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The - // ICF is not linked at the method prolog and unlined at the epilog when running R2R code. Since the + // ICF is not linked at the method prolog and unlinked at the epilog when running R2R code. Since the // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method - // has another pinovoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called + // has another pinvoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress)) { From 08a2fd7262e50d82dca35f856b2f09e8ba68ce52 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 1 Feb 2020 20:48:21 +0100 Subject: [PATCH 3/4] Use correct ESP to compare to the location of the InlinedCallFrame The correct ESP to use for detecting the InlinedCallFrame affected by the current exception handling is the the ESP of the caller of the method that contains the exception handler. I have moved the check to a different location where the CrawlFrame with the context of the exception handler is still available and we can unwind it to get the caller's SP. --- src/coreclr/src/vm/i386/excepx86.cpp | 52 ++++++++++++++++++---------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/coreclr/src/vm/i386/excepx86.cpp b/src/coreclr/src/vm/i386/excepx86.cpp index 3fb2a51043aef3..85f18d97d6972b 100644 --- a/src/coreclr/src/vm/i386/excepx86.cpp +++ b/src/coreclr/src/vm/i386/excepx86.cpp @@ -1868,23 +1868,6 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread) // Sync managed exception state, for the managed thread, based upon any active exception tracker pThread->SyncManagedExceptionState(fIsDebuggerHelperThread); - if ((pThread->m_pFrame < esp) && InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) - { - // The current explicit frame is below the resume esp and it is an inlined call frame with an active call. - // - // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from - // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls - // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The - // ICF is not linked at the method prolog and unlinked at the epilog when running R2R code. Since the - // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method - // has another pinvoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called - - if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress)) - { - pThread->m_pFrame->Pop(pThread); - } - } - LOG((LF_EH, LL_INFO1000, "COMPlusPEndCatch: esp=%p\n", esp)); return esp; @@ -3140,6 +3123,39 @@ void ResumeAtJitEH(CrawlFrame* pCf, *pHandlerEnd = EHClausePtr->HandlerEndPC; } + MethodDesc* pMethodDesc = pCf->GetCodeInfo()->GetMethodDesc(); + TADDR startAddress = pCf->GetCodeInfo()->GetStartAddress(); + if (InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) + { + // When unwinding an exception in ReadyToRun, the JIT_PInvokeEnd helper which unlinks the ICF from + // the thread will be skipped. This is because unlike jitted code, each pinvoke is wrapped by calls + // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The + // ICF is not linked at the method prolog and unlinked at the epilog when running R2R code. Since the + // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method + // has another pinvoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called. + + // Check that the InlinedCallFrame is in the method with the exception handler. There can be other + // InlinedCallFrame somewhere up the call chain that is not related to the current exception + // handling. + +#ifdef DEBUG + TADDR handlerFrameSP = pCf->GetRegisterSet()->SP; +#endif // DEBUG + // Find the ESP of the caller of the method with the exception handler. + bool unwindSuccess = pCf->GetCodeManager()->UnwindStackFrame(pCf->GetRegisterSet(), + pCf->GetCodeInfo(), + pCf->GetCodeManagerFlags(), + pCf->GetCodeManState(), + NULL /* StackwalkCacheUnwindInfo* */); + _ASSERTE(unwindSuccess); + + if (((TADDR)pThread->m_pFrame < pCf->GetRegisterSet()->SP) && ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress)) + { + _ASSERTE((TADDR)pThread->m_pFrame >= handlerFrameSP); + pThread->m_pFrame->Pop(pThread); + } + } + // save esp so that endcatch can restore it (it always restores, so want correct value) ExInfo* pExInfo = &(pThread->GetExceptionState()->m_currentExInfo); pExInfo->m_dEsp = (LPVOID)context.GetSP(); @@ -3253,7 +3269,7 @@ void ResumeAtJitEH(CrawlFrame* pCf, // that the handle for the current ExInfo has been freed has been delivered pExInfo->m_EHClauseInfo.SetManagedCodeEntered(TRUE); - ETW::ExceptionLog::ExceptionCatchBegin(pCf->GetCodeInfo()->GetMethodDesc(), (PVOID)pCf->GetCodeInfo()->GetStartAddress()); + ETW::ExceptionLog::ExceptionCatchBegin(pMethodDesc, (PVOID)startAddress); ResumeAtJitEHHelper(&context); UNREACHABLE_MSG("Should never return from ResumeAtJitEHHelper!"); From 54720eca474cdd740263f4c1a596cdbd317facf9 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Sat, 1 Feb 2020 21:25:51 +0100 Subject: [PATCH 4/4] Remove workaround for the original issue --- src/coreclr/src/vm/assemblynative.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/coreclr/src/vm/assemblynative.cpp b/src/coreclr/src/vm/assemblynative.cpp index 645d58b3d7b7e8..6567cd9dce1ab1 100644 --- a/src/coreclr/src/vm/assemblynative.cpp +++ b/src/coreclr/src/vm/assemblynative.cpp @@ -45,9 +45,6 @@ void QCALLTYPE AssemblyNative::InternalLoad(QCall::ObjectHandleOnStack assemblyN GCX_COOP(); - // Workaround for https://github.com/dotnet/runtime/issues/2240 - FrameWithCookie workaround; - if (assemblyName.Get() == NULL) { COMPlusThrow(kArgumentNullException, W("ArgumentNull_AssemblyName")); @@ -130,8 +127,6 @@ void QCALLTYPE AssemblyNative::InternalLoad(QCall::ObjectHandleOnStack assemblyN retAssembly.Set(pAssembly->GetExposedObject()); } - workaround.Pop(); - END_QCALL; }