Fix propagating of unhandled exception to native caller

src/coreclr/vm/exceptionhandling.cpp

@@ -864,7 +864,7 @@ UINT_PTR ExceptionTracker::FinishSecondPass(
 
 void CleanUpForSecondPass(Thread* pThread, bool fIsSO, LPVOID MemoryStackFpForFrameChain, LPVOID MemoryStackFp);
 
-static void PopExplicitFrames(Thread *pThread, void *targetSp, void *targetCallerSp)
+static void PopExplicitFrames(Thread *pThread, void *targetSp, void *targetCallerSp, bool popGCFrames = true)
 {
     Frame* pFrame = pThread->GetFrame();
     while (pFrame < targetSp)
@@ -907,11 +907,14 @@ static void PopExplicitFrames(Thread *pThread, void *targetSp, void *targetCalle
         }
     }
 
-    GCFrame* pGCFrame = pThread->GetGCFrame();
-    while (pGCFrame && pGCFrame < targetSp)
+    if (popGCFrames)
     {
-        pGCFrame->Pop();
-        pGCFrame = pThread->GetGCFrame();
+        GCFrame* pGCFrame = pThread->GetGCFrame();
+        while (pGCFrame && pGCFrame < targetSp)
+        {
+            pGCFrame->Pop();
+            pGCFrame = pThread->GetGCFrame();
+        }
     }
 }
 
@@ -934,11 +937,30 @@ ProcessCLRExceptionNew(IN     PEXCEPTION_RECORD   pExceptionRecord,
 
     // Skip native frames of asm helpers that have the ProcessCLRException set as their personality routine.
     // There is nothing to do for those with the new exception handling.
+    if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc))
+    {
+        return ExceptionContinueSearch;
+    }
+
+    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 unhandledand 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 (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
-        pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
+    if (pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
     {
+        if (pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING)
+        {
+            pThread->SetThreadStateNC(Thread::TSNC_UnhandledException2ndPass);
+        }
+
         return ExceptionContinueSearch;
     }
 
@@ -8438,6 +8460,8 @@ 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);
+
     BEGIN_QCALL;
 
     Frame* pFrame = pThread->GetFrame();
@@ -8554,7 +8578,7 @@ extern "C" CLR_BOOL QCALLTYPE SfiInit(StackFrameIterator* pThis, CONTEXT* pStack
 #ifdef HOST_WINDOWS
         CreateCrashDumpIfEnabled(/* fSOException */ FALSE);
         GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException);
-        RaiseException(pExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE_EXCEPTION, pExInfo->m_ptrs.ExceptionRecord->NumberParameters, pExInfo->m_ptrs.ExceptionRecord->ExceptionInformation);
+        RaiseException(pExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE, pExInfo->m_ptrs.ExceptionRecord->NumberParameters, pExInfo->m_ptrs.ExceptionRecord->ExceptionInformation);
 #else
         CrashDumpAndTerminateProcess(pExInfo->m_ExceptionCode);
 #endif
@@ -8690,8 +8714,17 @@ extern "C" CLR_BOOL QCALLTYPE SfiNext(StackFrameIterator* pThis, uint* uExCollid
                 else
                 {
 #ifdef HOST_WINDOWS
+                    {
+                        GCX_COOP();
+                        void *sp = (void*)GetRegdisplaySP(pThis->m_crawl.GetRegisterSet());
+                        // 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.
+                        PopExplicitFrames(pThread, sp, NULL /* targetCallerSp */, false /* popGCFrames */);
+                        ExInfo::PopExInfos(pThread, sp);
+                    }
                     GetThread()->SetThreadStateNC(Thread::TSNC_ProcessedUnhandledException);
-                    RaiseException(pTopExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE_EXCEPTION, pTopExInfo->m_ptrs.ExceptionRecord->NumberParameters, pTopExInfo->m_ptrs.ExceptionRecord->ExceptionInformation);
+                    RaiseException(pTopExInfo->m_ExceptionCode, EXCEPTION_NONCONTINUABLE, pTopExInfo->m_ptrs.ExceptionRecord->NumberParameters, pTopExInfo->m_ptrs.ExceptionRecord->ExceptionInformation);
 #else
                     CrashDumpAndTerminateProcess(pTopExInfo->m_ExceptionCode);
 #endif

src/coreclr/vm/threads.h

@@ -655,7 +655,7 @@ class Thread
                                                       // effort.
                                                       //
                                                       // Once we are completely independent of the OS UEF, we could remove this.
-        // unused                       = 0x02000000,
+        TSNC_UnhandledException2ndPass  = 0x02000000, // The unhandled exception propagation is in the 2nd pass
         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.

src/tests/Exceptions/ForeignThread/ForeignThreadExceptions.cs

@@ -15,6 +15,12 @@ public class ForeignThreadExceptionsTest
     [DllImport("ForeignThreadExceptionsNative")]
     public static extern void InvokeCallbackOnNewThread(MyCallback callback);
 
+    [DllImport("ForeignThreadExceptionsNative")]
+    public static extern void InvokeCallbackAndCatchTwiceOnNewThread(MyCallback callback);
+
+    [DllImport("ForeignThreadExceptionsNative")]
+    public static extern void ThrowException();
+
     public static void MethodThatThrows()
     {
         throw new Exception("This is MethodThatThrows.");
@@ -56,6 +62,25 @@ public static void RunTest()
                 Console.WriteLine("Caught hardware exception in a delegate called through Reverse PInvoke on a foreign thread.");
             }
         });
+
+        InvokeCallbackAndCatchTwiceOnNewThread(() => {
+            throw new Exception("Exception unhandled in any managed code");
+        });
+
+        int finallyCallsCount = 0;
+        InvokeCallbackAndCatchTwiceOnNewThread(() => {
+            try
+            {
+                // Throw native exception that is not handled in any managed code
+                ThrowException();
+            }
+            finally
+            {
+                finallyCallsCount++;
+            }
+        });
+
+        Assert.Equal(2, finallyCallsCount);
     }
 
     [Fact]

src/tests/Exceptions/ForeignThread/ForeignThreadExceptionsNative.cpp

@@ -15,29 +15,72 @@
 
 #include <platformdefines.h>
 
+extern "C" DLL_EXPORT void STDMETHODCALLTYPE ThrowException()
+{
+    throw std::exception{};
+}
+
 typedef void (*PFNACTION1)();
 extern "C" DLL_EXPORT void InvokeCallback(PFNACTION1 callback)
 {
     callback();
 }
 
+extern "C" DLL_EXPORT void InvokeCallbackAndCatchTwice(PFNACTION1 callback)
+{
+    try
+    {
+        callback();
+    }
+    catch (...)
+    {
+        printf("Caught exception once\n");
+    }
+
+    try
+    {
+        // Put garbage on the stack that was possibly used by the previous callback to catch
+        // cases when the explicit frames or ExInfos were not cleaned up properly when the
+        // exception is not handled in the managed code and reaches this native caller.
+        unsigned int *p = (unsigned int *)alloca(16384);
+        for (int i = 0; i < 16384 / sizeof(unsigned int); i++)
+        {
+            *p++ = 0xbaadf00d;
+        }
+        callback();
+    }
+    catch (...)
+    {
+        printf("Caught exception again\n");
+    }
+}
+
 #ifndef _WIN32
 void* InvokeCallbackUnix(void* callback)
 {
     InvokeCallback((PFNACTION1)callback);
     return NULL;
 }
 
+void* InvokeCallbackAndCatchTwiceUnix(void* callback)
+{
+    InvokeCallbackAndCatchTwice((PFNACTION1)callback);
+    return NULL;
+}
+
 #define AbortIfFail(st) if (st != 0) abort()
 
 #endif // !_WIN32
 
-extern "C" DLL_EXPORT void InvokeCallbackOnNewThread(PFNACTION1 callback)
-{
 #ifdef _WIN32
-    std::thread t1(InvokeCallback, callback);
+void InvokeCallbackOnNewThreadCommon(PFNACTION1 callback, void (*startRoutine)(PFNACTION1))
+{
+    std::thread t1(startRoutine, callback);
     t1.join();
+}
 #else // _WIN32
+void InvokeCallbackOnNewThreadCommon(PFNACTION1 callback, void *(*startRoutine)(void*))
+{
     // For Unix, we need to use pthreads to create the thread so that we can set its stack size.
     // We need to set the stack size due to the very small (80kB) default stack size on MUSL
     // based Linux distros.
@@ -50,10 +93,28 @@ extern "C" DLL_EXPORT void InvokeCallbackOnNewThread(PFNACTION1 callback)
     AbortIfFail(st);
 
     pthread_t t;
-    st = pthread_create(&t, &attr, InvokeCallbackUnix, (void*)callback);
+    st = pthread_create(&t, &attr, startRoutine, (void*)callback);
     AbortIfFail(st);
 
     st = pthread_join(t, NULL);
     AbortIfFail(st);
+}
 #endif // _WIN32
+
+extern "C" DLL_EXPORT void InvokeCallbackAndCatchTwiceOnNewThread(PFNACTION1 callback)
+{
+#ifdef _WIN32
+    InvokeCallbackOnNewThreadCommon(callback, InvokeCallbackAndCatchTwice);
+#else // _WIN32
+    InvokeCallbackOnNewThreadCommon(callback, InvokeCallbackAndCatchTwiceUnix);
+#endif
+}
+
+extern "C" DLL_EXPORT void InvokeCallbackOnNewThread(PFNACTION1 callback)
+{
+#ifdef _WIN32
+    InvokeCallbackOnNewThreadCommon(callback, InvokeCallback);
+#else // _WIN32
+    InvokeCallbackOnNewThreadCommon(callback, InvokeCallbackUnix);
+#endif
 }