From 77cd9fe42e490c4be6505b265260a8fe89c14837 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 24 Jul 2025 00:22:00 +0200 Subject: [PATCH 1/2] Fix unhandled exception reporting corner cases There are several issues with processing exceptions that are not handled by managed code on Windows: * Sometimes the AppDomain.UnhandledException event is sent multiple times * Exception flowing to foreign native code is reported as unhandled even when it is actually caught by the native code * In rare cases, the unhandled exception stack trace is doubled This change fixes them by not reporting the unhandled exception in the SfiNext on Windows. It just raises the underlying SEH exception there with the thread marked so that the personality routines for the managed frames won't run the managed exception handling code. If the exception is truly unhandled, the `InternalUnhandledExceptionFilter_Worker` will be called by the unhandled exception filter installed for the process and report the exception as unhandled. If that exception ends up being caught by a foreign native code, then nothing will be reported. Close #115215 --- src/coreclr/vm/exceptionhandling.cpp | 53 +++++++++++----------------- src/coreclr/vm/threads.h | 2 +- 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 1babb39e008dfb..fe9474fa2f5280 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -579,32 +579,21 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord, } #endif - if (pThread->HasThreadStateNC(Thread::TSNC_UnhandledException2ndPass) && !(pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)) - { - // We are in the 1st pass of exception handling, but the thread mark says that it has already executed 2nd pass - // of unhandled exception handling. That means that some external native code on top of the stack has caught the - // exception that runtime considered to be unhandled, and a new native exception was thrown on the current thread. - // We need to reset the flags below so that we no longer block exception handling for the managed frames. - pThread->ResetThreadStateNC(Thread::TSNC_UnhandledException2ndPass); - pThread->ResetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); - } - // Also skip all frames when processing unhandled exceptions. That allows them to reach the host app // level and let 3rd party the chance to handle them. - if (pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException)) + if (pThread->HasThreadStateNC(Thread::TSNC_SkipManagedPersonalityRoutine)) { if (pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING) { - if (!pThread->HasThreadStateNC(Thread::TSNC_UnhandledException2ndPass)) + // The 3rd argument passes to PopExplicitFrame is normally the parent SP to correctly handle InlinedCallFrame embbeded + // in parent managed frame. But at this point there are no further managed frames are on the stack, so we can pass NULL. + // Also don't pop the GC frames, their destructor will pop them as the exception propagates. + // NOTE: this needs to be popped in the 2nd pass to ensure that crash dumps and Watson get the dump with these still + // present. + ExInfo *pExInfo = (ExInfo*)pThread->GetExceptionState()->GetCurrentExceptionTracker(); + if (pExInfo != NULL) { - pThread->SetThreadStateNC(Thread::TSNC_UnhandledException2ndPass); GCX_COOP(); - // The 3rd argument passes to PopExplicitFrame is normally the parent SP to correctly handle InlinedCallFrame embbeded - // in parent managed frame. But at this point there are no further managed frames are on the stack, so we can pass NULL. - // Also don't pop the GC frames, their destructor will pop them as the exception propagates. - // NOTE: this needs to be popped in the 2nd pass to ensure that crash dumps and Watson get the dump with these still - // present. - ExInfo *pExInfo = (ExInfo*)pThread->GetExceptionState()->GetCurrentExceptionTracker(); void *sp = (void*)GetRegdisplaySP(pExInfo->m_frameIter.m_crawl.GetRegisterSet()); PopExplicitFrames(pThread, sp, NULL /* targetCallerSp */, false /* popGCFrames */); ExInfo::PopExInfos(pThread, sp); @@ -3757,7 +3746,10 @@ extern "C" CLR_BOOL QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStack Thread* pThread = GET_THREAD(); ExInfo* pExInfo = (ExInfo*)pThread->GetExceptionState()->GetCurrentExceptionTracker(); - pThread->ResetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); + if (pExInfo->m_passNumber == 1) + { + pThread->ResetThreadStateNC(Thread::TSNC_SkipManagedPersonalityRoutine); + } BEGIN_QCALL; @@ -3872,14 +3864,13 @@ extern "C" CLR_BOOL QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStack } else { + // There are no managed frames on the stack EH_LOG((LL_INFO100, "SfiInit: No more managed frames found on stack\n")); - // There are no managed frames on the stack, fail fast and report unhandled exception - LONG disposition = InternalUnhandledExceptionFilter_Worker((EXCEPTION_POINTERS *)&pExInfo->m_ptrs); #ifdef HOST_WINDOWS - CreateCrashDumpIfEnabled(/* fSOException */ FALSE); - GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); + GetThread()->SetThreadStateNC(Thread::TSNC_SkipManagedPersonalityRoutine); RaiseException(pExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE, pExInfo->m_ptrs.ExceptionRecord->NumberParameters, pExInfo->m_ptrs.ExceptionRecord->ExceptionInformation); #else + LONG disposition = InternalUnhandledExceptionFilter_Worker((EXCEPTION_POINTERS *)&pExInfo->m_ptrs); CrashDumpAndTerminateProcess(pExInfo->m_ExceptionCode); #endif } @@ -3970,6 +3961,7 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid if (isPropagatingToNativeCode) { + // Propagating through Reverse pinvoke #ifdef HOST_UNIX void* callbackCxt = NULL; Interop::ManagedToNativeExceptionCallback callback = Interop::GetPropagatingExceptionCallback( @@ -3990,6 +3982,7 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid } else { + // Propagating to CallDescrWorkerInternal, filter funclet or CallEHFunclet. if (IsCallDescrWorkerInternalReturnAddress(GetIP(pThis->m_crawl.GetRegisterSet()->pCurrentContext))) { EH_LOG((LL_INFO100, "SfiNext: the native frame is CallDescrWorkerInternal")); @@ -4000,10 +3993,6 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid EH_LOG((LL_INFO100, "SfiNext: current frame is filter funclet")); isPropagatingToNativeCode = TRUE; } - else - { - isPropagatingToExternalNativeCode = true; - } } if (isPropagatingToNativeCode) @@ -4027,15 +4016,15 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid EH_LOG((LL_INFO100, "SfiNext (pass %d): no more managed frames on the stack, the exception is unhandled", pTopExInfo->m_passNumber)); if (pTopExInfo->m_passNumber == 1) { +#ifdef HOST_UNIX LONG disposition = InternalUnhandledExceptionFilter_Worker((EXCEPTION_POINTERS *)&pTopExInfo->m_ptrs); -#ifdef HOST_WINDOWS - CreateCrashDumpIfEnabled(/* fSOException */ FALSE); -#endif + GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); +#endif // HOST_UNIX } else { #ifdef HOST_WINDOWS - GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); + GetThread()->SetThreadStateNC(Thread::TSNC_SkipManagedPersonalityRoutine); RaiseException(pTopExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE, pTopExInfo->m_ptrs.ExceptionRecord->NumberParameters, pTopExInfo->m_ptrs.ExceptionRecord->ExceptionInformation); #else CrashDumpAndTerminateProcess(pTopExInfo->m_ExceptionCode); diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 5bcd88edfb7e17..90684b7d49fa48 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -642,7 +642,7 @@ class Thread // effort. // // Once we are completely independent of the OS UEF, we could remove this. - TSNC_UnhandledException2ndPass = 0x02000000, // The unhandled exception propagation is in the 2nd pass + TSNC_SkipManagedPersonalityRoutine = 0x02000000, // Ignore the ProcessCLRException calls when propagating exception to external native code TSNC_DebuggerSleepWaitJoin = 0x04000000, // Indicates to the debugger that this thread is in a sleep wait or join state // This almost mirrors the TS_Interruptible state however that flag can change // during GC-preemptive mode whereas this one cannot. From f0222cade60bc460c5d0b75d2a0177b7df7070ec Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Thu, 7 Aug 2025 21:48:04 +0200 Subject: [PATCH 2/2] Ensure AppDomain.CurrentDomain.UnhandledException occurs before finally --- src/coreclr/vm/exceptionhandling.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index fe9474fa2f5280..818f5dbef72639 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -3975,10 +3975,10 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid pTopExInfo->m_propagateExceptionContext = callbackCxt; } else +#endif // HOST_UNIX { isPropagatingToExternalNativeCode = true; } -#endif // HOST_UNIX } else { @@ -4013,13 +4013,16 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid if (isNotHandledByRuntime && IsExceptionFromManagedCode(pTopExInfo->m_ptrs.ExceptionRecord)) { - EH_LOG((LL_INFO100, "SfiNext (pass %d): no more managed frames on the stack, the exception is unhandled", pTopExInfo->m_passNumber)); + EH_LOG((LL_INFO100, "SfiNext (pass %d): no more managed frames on the stack, the exception is not handled by the runtime\n", pTopExInfo->m_passNumber)); if (pTopExInfo->m_passNumber == 1) { -#ifdef HOST_UNIX - LONG disposition = InternalUnhandledExceptionFilter_Worker((EXCEPTION_POINTERS *)&pTopExInfo->m_ptrs); - GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); -#endif // HOST_UNIX +#ifdef HOST_WINDOWS + if (!isPropagatingToExternalNativeCode) +#endif + { + LONG disposition = InternalUnhandledExceptionFilter_Worker((EXCEPTION_POINTERS *)&pTopExInfo->m_ptrs); + GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException); + } } else {