Fix for GH issue 410 - https://github.com/dotnet/coreclr/issues/410

Whenever a managed exception is thrown, the details about the thrown exception are also saved off the managed thread object (as LastThrownObject). The VM also has an exception tracker that tracks its dispatch across the managed frames and incase of nested exceptions, the trackers are collapsed correctly, when the nested exception is handled, and last thrown object is updated correctly. The VM works on the premise that the LastThrownObject is updated correctly.

Incase of this bug, a method (M1)is invoked via Reflection and has an exception E1. During exception dispatch for E1, an IL filter is invoked that, in turn, has an exception (E2) that remains unhandled. While this is swallowed by the VM (as expected), the LastThrownObject is not updated to reflect the active exception to be E1. Thus, when the dispatch for original exception E1 completes and no managed handler is found, the exception is caught by Reflection subsystem that extracts the thrown exception using the GET_THROWABLE macro that uses the LastThrownObject to determine the thrown exception. Since the LTO was not updated, it still reflects E2.

The fix is to update the managed exception state, if the filter has an unhandled exception, similar to how we do when a managed catch block successfully handles the exception. I have refactored the code to make the semantic cleaner.

[tfs-changeset: 1525835]

6 files changed, 56 insertions, 29 deletions

diff --git a/src/vm/excep.cpp b/src/vm/excep.cpp
index f1f19dd963..a5aed2da89 100644
--- a/src/vm/excep.cpp
+++ b/src/vm/excep.cpp
@@ -12234,18 +12234,18 @@ BOOL CEHelper::ShouldTreatActiveExceptionAsNonCorrupting()
 //
 // Note: This method must be called once the exception trackers have been adjusted post catch-block execution.
 /* static */
-void CEHelper::ResetLastActiveCorruptionSeverityPostCatchHandler()
+void CEHelper::ResetLastActiveCorruptionSeverityPostCatchHandler(Thread *pThread)
 {
     CONTRACTL
     {
         NOTHROW;
         GC_NOTRIGGER;
         MODE_ANY;
-        PRECONDITION(GetThread() != NULL);
+        PRECONDITION(pThread != NULL);
     }
     CONTRACTL_END;
 
-    ThreadExceptionState *pCurTES = GetThread()->GetExceptionState();
+    ThreadExceptionState *pCurTES = pThread->GetExceptionState();
 
     // By this time, we would have set the correct exception tracker for the active exception domain,
     // if applicable. An example is throwing and catching an exception within a catch block. We will update
diff --git a/src/vm/excep.h b/src/vm/excep.h
index d411411c4c..947d7896e1 100644
--- a/src/vm/excep.h
+++ b/src/vm/excep.h
@@ -859,7 +859,7 @@ public:
     void static SetupCorruptionSeverityForActiveExceptionInUnwindPass(Thread *pCurThread, PTR_ExceptionTracker pEHTracker, BOOL fIsFirstPass, 
                                                                      DWORD dwExceptionCode);
 #endif // WIN64EXCEPTIONS
-    void static ResetLastActiveCorruptionSeverityPostCatchHandler();
+    void static ResetLastActiveCorruptionSeverityPostCatchHandler(Thread *pThread);
 };
 
 #endif // FEATURE_CORRUPTING_EXCEPTIONS
diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp
index 34b304437c..e1b6925723 100644
--- a/src/vm/exceptionhandling.cpp
+++ b/src/vm/exceptionhandling.cpp
@@ -658,15 +658,9 @@ UINT_PTR ExceptionTracker::FinishSecondPass(
     {
         pThread->SafeSetLastThrownObject(NULL);
     }
-    pThread->SafeUpdateLastThrownObject();
-
-#ifdef FEATURE_CORRUPTING_EXCEPTIONS
-    // Since the catch clause has successfully executed and we are exiting it, reset the corruption severity
-    // in the ThreadExceptionState for the last active exception. This will ensure that when the next exception
-    // gets thrown/raised, EH tracker wont pick up an invalid value.
-    CEHelper::ResetLastActiveCorruptionSeverityPostCatchHandler();
-#endif // FEATURE_CORRUPTING_EXCEPTIONS
 
+    // Sync managed exception state, for the managed thread, based upon any active exception tracker
+    pThread->SyncManagedExceptionState(false);
 
     //
     // If we are aborting, we should not resume execution.  Instead, we raise another
@@ -2932,7 +2926,7 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame(
                                 // 3) CallerSP is intact
                                 _ASSERTE(GetSP(pCurRegDisplay->pCallerContext) == GetRegdisplaySP(pCurRegDisplay));
 #endif // _TARGET_ARM_ || _TARGET_ARM64_
-                                 {
+                                {
                                     // CallHandler expects to be in COOP mode.
                                     GCX_COOP();
                                     dwResult = CallHandler(dwFilterStartPC, sf, &EHClause, pMD, Filter ARM_ARG(pCurRegDisplay->pCallerContext) ARM64_ARG(pCurRegDisplay->pCallerContext));
@@ -2960,6 +2954,10 @@ CLRUnwindStatus ExceptionTracker::ProcessManagedCallFrame(
                                 }
 #endif // !FEATURE_PAL 
 
+                                // We had an exception in filter invocation that remained unhandled.
+                                // Sync managed exception state, for the managed thread, based upon the active exception tracker.
+                                pThread->SyncManagedExceptionState(false);
+
                                 // we've returned from the filter abruptly, now out of managed code
                                 m_EHClauseInfo.SetManagedCodeEntered(FALSE);
 
diff --git a/src/vm/i386/excepx86.cpp b/src/vm/i386/excepx86.cpp
index 6bf26a4790..7de7b43e0d 100644
--- a/src/vm/i386/excepx86.cpp
+++ b/src/vm/i386/excepx86.cpp
@@ -1890,18 +1890,11 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread)
 
     pExInfo->UnwindExInfo(esp);
     
-    // This will set the last thrown to be either null if we have handled all the exceptions in the nested chain or
-    // to whatever the current exception is.
+    // Prepare to sync managed exception state
     //
     // In a case when we're nested inside another catch block, the domain in which we're executing may not be the
     // same as the one the domain of the throwable that was just made the current throwable above. Therefore, we
     // make a special effort to preserve the domain of the throwable as we update the the last thrown object.
-    pThread->SafeUpdateLastThrownObject();
-
-#ifdef FEATURE_CORRUPTING_EXCEPTIONS
-    // Since the catch clause has successfully executed and we are exiting it, reset the corruption severity
-    // in the ThreadExceptionState for the last active exception. This will ensure that when the next exception
-    // gets thrown/raised, EH tracker wont pick up an invalid value.
     //
     // This function (COMPlusEndCatch) can also be called by the in-proc debugger helper thread on x86 when
     // an attempt to SetIP takes place to set IP outside the catch clause. In such a case, managed thread object
@@ -1910,13 +1903,10 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread)
     // This behaviour (of debugger doing SetIP) is not allowed on 64bit since the catch clauses are implemented
     // as a seperate funclet and it's just not allowed to set the IP across EH scopes, such as from inside a catch 
     // clause to outside of the catch clause.
-
     bool fIsDebuggerHelperThread = (g_pDebugInterface == NULL) ? false : g_pDebugInterface->ThisIsHelperThread();
-    if (fIsDebuggerHelperThread == false)
-    {
-        CEHelper::ResetLastActiveCorruptionSeverityPostCatchHandler();
-    }
-#endif // FEATURE_CORRUPTING_EXCEPTIONS
+
+    // Sync managed exception state, for the managed thread, based upon any active exception tracker
+    pThread->SyncManagedExceptionState(fIsDebuggerHelperThread);
 
     LOG((LF_EH, LL_INFO1000, "COMPlusPEndCatch: esp=%p\n", esp));
    
@@ -2241,7 +2231,8 @@ int COMPlusThrowCallbackHelper(IJitManager *pJitManager,
                                ThrowCallbackType* pData,
                                EE_ILEXCEPTION_CLAUSE  *EHClausePtr,
                                DWORD nestingLevel,
-                               OBJECTREF throwable
+                               OBJECTREF throwable,
+                               Thread *pThread
                               )
 {
     CONTRACTL
@@ -2292,6 +2283,10 @@ int COMPlusThrowCallbackHelper(IJitManager *pJitManager,
             impersonating = FALSE;
         }
 
+        // We had an exception in filter invocation that remained unhandled.
+        // Sync managed exception state, for the managed thread, based upon the active exception tracker.
+        pThread->SyncManagedExceptionState(false);
+        
         //
         // Swallow exception.  Treat as exception continue search.
         //
@@ -2702,7 +2697,8 @@ StackWalkAction COMPlusThrowCallback(       // SWA value
                                                    pData,
                                                    &EHClause,
                                                    nestingLevel,
-                                                   throwable);
+                                                   throwable,
+                                                   pThread);
 
             pExInfo->m_EHClauseInfo.SetManagedCodeEntered(FALSE);
 
diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp
index 076ffe1e3f..007d233f14 100644
--- a/src/vm/threads.cpp
+++ b/src/vm/threads.cpp
@@ -5524,6 +5524,37 @@ OBJECTREF Thread::SafeSetThrowables(OBJECTREF throwable DEBUG_ARG(ThreadExceptio
     return ret;
 }
 
+// This method will sync the managed exception state to be in sync with the topmost active exception
+// for a given thread
+void Thread::SyncManagedExceptionState(bool fIsDebuggerThread)
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
+        MODE_ANY;
+    }
+    CONTRACTL_END;
+
+    {
+        GCX_COOP();
+
+        // Syncup the LastThrownObject on the managed thread
+        SafeUpdateLastThrownObject();
+    }
+
+#ifdef FEATURE_CORRUPTING_EXCEPTIONS
+    // Since the catch clause has successfully executed and we are exiting it, reset the corruption severity
+    // in the ThreadExceptionState for the last active exception. This will ensure that when the next exception
+    // gets thrown/raised, EH tracker wont pick up an invalid value.
+    if (!fIsDebuggerThread)
+    {
+        CEHelper::ResetLastActiveCorruptionSeverityPostCatchHandler(this);
+    }
+#endif // FEATURE_CORRUPTING_EXCEPTIONS
+
+}
+
 void Thread::SetLastThrownObjectHandle(OBJECTHANDLE h)
 {
     CONTRACTL
diff --git a/src/vm/threads.h b/src/vm/threads.h
index 1b2f6d8a59..0ab550f741 100644
--- a/src/vm/threads.h
+++ b/src/vm/threads.h
@@ -2566,6 +2566,8 @@ public:
     }
 
 
+    void SyncManagedExceptionState(bool fIsDebuggerThread);
+
     //---------------------------------------------------------------
     // Per-thread information used by handler
     //---------------------------------------------------------------