Fix GC issues during exception handling on Unix

This change fixes two issues that happens in some cases when GC scans stack of
a thread that is handling exception at that moment.

First issue was caused by the fact that the stack walker wasn't modified to
take into account the difference in exception handling on Unix. When an exception
is thrown from a catch handler or finally block (a funclet )on Unix, part of the stack
is unwound immediatelly, while on Windows, the stack is not reclaimed until
the exception is fully handled.
The problem was caused by the fact that when GC happens in the funclet before the exception
is processed, but after a point where GC knows that the lifetime of locals in the caller frame
is over, it doesn't update the references in the caller frame for objects that it has relocated.
On Windows, this is detected just from walking the stack, since the funclet frame is still there
and the stack walker can then skip scanning the parent frame GC references.
On Linux, the funclet is not on stack anymore, so this case was not detected and GC attempted to
scan the stale references and crashed.
The fix was to detect that a frame was a caller to a funclet from the chain of previous exception
trackers that is fortunately preserved and the trackers hold the necessary information.

The second issue was more subtle. During interleaved exception handling, when we unwind a native
portion of the stack and switch back to unwinding a managed block of stack frames, we re-create
the exception tracker and carry over just a few members necessary to continue processing the
same exception. The way it was done was that we have first removed the current tracker from
the list of trackers of the current thread, then we have destroyed it, created a new one and
put it back to the front of the list.
The issue happened when GC started walking the stack of the thread in the small time slot when
the current tracker was removed from the list, but the re-created tracker was not added there yet.
Then the detection necessary for handling the previous issue didn't work and we got a crash.
The fix was to make the whole re-creation of the exception tracker atomic w.r.t. the GC.

2 files changed, 87 insertions, 25 deletions

diff --git a/src/vm/exceptionhandling.cpp b/src/vm/exceptionhandling.cpp
index d2b13a7315..49e0ed510a 100644
--- a/src/vm/exceptionhandling.cpp
+++ b/src/vm/exceptionhandling.cpp
@@ -3612,8 +3612,6 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker(
     bool fIsInterleavedHandling = false;
     bool fTransitionFromSecondToFirstPass = false;
 
-    PartialTrackerState previousTrackerPartialState;
-
     // Initialize the out parameter.
     *pStackTraceState = STS_Append;
 
@@ -3649,12 +3647,58 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker(
             {
                 if (fIsInterleavedHandling)
                 {
-                    // Remember part of the current state that needs to be transferred to
-                    // the newly created tracker.
-                    previousTrackerPartialState.Save(pTracker);
-
-                    // We just transitioned from 2nd pass to 1st pass when we handle the exception in an interleaved manner
+                    // We just transitioned from 2nd pass to 1st pass when we handle the exception in an interleaved manner.
+                    // In interleaved exception handling a single exception is handled in an interleaved
+                    // series of first and second passes. We re-create the exception tracker in-place
+                    // and carry over several members that were set when processing one of the previous frames.
                     EH_LOG((LL_INFO100, ">>continued processing of PREVIOUS exception (interleaved handling)\n"));
+                    {
+                        GCX_COOP();
+
+                        // Remember part of the current state that needs to be transferred to
+                        // the newly created tracker.
+                        PartialTrackerState previousTrackerPartialState;
+                        previousTrackerPartialState.Save(pTracker);
+
+                        // Rember the previous tracker pointer so that we can restore it
+                        ExceptionTracker* prevTracker = pTracker->m_pPrevNestedInfo;
+
+                        // Free the tracker resources so that we can recreate a new one in-place
+                        pTracker->ReleaseResources();
+
+                        new (pTracker) ExceptionTracker(ControlPc,
+                                                        pExceptionRecord,
+                                                        pContextRecord);
+
+                        // Restore part of the tracker state that was saved before recreating the tracker
+                        previousTrackerPartialState.Restore(pTracker);
+                        // Reset the 'unwind has started' flag to indicate we are in the first pass again
+                        pTracker->m_ExceptionFlags.ResetUnwindHasStarted();
+
+                        // Restore the trackerlink to the previous exception tracker
+                        pTracker->m_pPrevNestedInfo = prevTracker;
+
+                        OBJECTREF oThrowable = CreateThrowable(pExceptionRecord, bAsynchronousThreadStop);
+
+                        GCX_FORBID();   // we haven't protected oThrowable
+
+                        CONSISTENCY_CHECK(oThrowable != NULL);
+                        CONSISTENCY_CHECK(NULL == pTracker->m_hThrowable);
+
+                        pThread->SafeSetThrowables(oThrowable);
+
+                        if (pTracker->CanAllocateMemory())
+                        {
+                            pTracker->m_StackTraceInfo.AllocateStackTrace();
+                        }
+
+                        INDEBUG(oThrowable = NULL);
+
+                        _ASSERTE(pTracker->m_pLimitFrame == NULL);
+                        pTracker->ResetLimitFrame();
+                    }
+
+                    *pStackTraceState = STS_Append;
                 }
                 else
                 {
@@ -3662,14 +3706,13 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker(
                     // This means that some unmanaged frame outside of the EE catches the previous exception,
                     // so we should trash the current tracker and create a new one.
                     EH_LOG((LL_INFO100, ">>NEW exception (the previous second pass finishes at some unmanaged frame outside of the EE)\n"));
-                }
+                    {
+                        GCX_COOP();
+                        ExceptionTracker::PopTrackers(sf, false);
+                    }
 
-                {
-                    GCX_COOP();
-                    ExceptionTracker::PopTrackers(sf, false);
+                    fCreateNewTracker = true;
                 }
-
-                fCreateNewTracker = true;
             }
             else
             {
@@ -3741,17 +3784,6 @@ ExceptionTracker* ExceptionTracker::GetOrCreateTracker(
                                            pExceptionRecord,
                                            pContextRecord);
 
-        if (fIsInterleavedHandling && fTransitionFromSecondToFirstPass)
-        {
-            // In interleaved exception handling a single exception is handled in an interleaved
-            // series of first and second passes. When we create a new tracker as a result
-            // of switching from the 2nd pass back to the 1st pass, we need to carry over
-            // several members that were set when processing one of the previous frames.
-            previousTrackerPartialState.Restore(pNewTracker);
-            // Reset the 'unwind has started' flag to indicate we are in the first pass again
-            pNewTracker->m_ExceptionFlags.ResetUnwindHasStarted();
-        }
-
         CONSISTENCY_CHECK(pNewTracker->IsValid());
         CONSISTENCY_CHECK(pThread == pNewTracker->m_pThread);
 
diff --git a/src/vm/stackwalk.cpp b/src/vm/stackwalk.cpp
index 3301dec092..2951f272c5 100644
--- a/src/vm/stackwalk.cpp
+++ b/src/vm/stackwalk.cpp
@@ -1659,6 +1659,7 @@ StackWalkAction StackFrameIterator::Filter(void)
         fSkippingFunclet = false;
         
 #if defined(WIN64EXCEPTIONS)
+        ExceptionTracker* pTracker = m_crawl.pThread->GetExceptionState()->GetCurrentExceptionTracker();
         fRecheckCurrentFrame = false;
         fSkipFuncletCallback = true;
         
@@ -1743,6 +1744,36 @@ ProcessFuncletsForGCReporting:
                     // Check if we are in the mode of enumerating GC references (or not)
                     if (m_flags & GC_FUNCLET_REFERENCE_REPORTING)
                     {
+#ifdef FEATURE_PAL
+                        // For interleaved exception handling on non-windows systems, we need to find out if the current frame
+                        // was a caller of an already executed exception handler based on the previous exception trackers.
+                        // The handler funclet frames are already gone from the stack, so the exception trackers are the
+                        // only source of evidence about it.
+                        // This is different from Windows where the full stack is preserved until an exception is fully handled
+                        // and so we can detect it just from walking the stack.
+                        if (!fSkippingFunclet && (pTracker != NULL))
+                        {
+                            ExceptionTracker* pCurrTracker = pTracker;
+                            // Scan all previous trackers and see if the current frame is a caller of any of
+                            // the handling frames. 
+                            while ((pCurrTracker = pCurrTracker->GetPreviousExceptionTracker()) != NULL)
+                            {
+                                StackFrame sfFuncletParent = pCurrTracker->GetCallerOfActualHandlingFrame();
+                                if (ExceptionTracker::IsUnwoundToTargetParentFrame(&m_crawl, sfFuncletParent))
+                                {
+                                    // We have found that the current frame is a caller of a handling frame.
+                                    // Set the members the same way we would set them on Windows when we
+                                    // would detect this just from stack walking.
+                                    m_sfParent = sfFuncletParent;
+                                    m_sfFuncletParent = sfFuncletParent;
+                                    m_fProcessNonFilterFunclet = true;
+                                    pTracker = pCurrTracker;
+
+                                    break;
+                                }
+                            }
+                        }
+#endif // FEATURE_PAL
                         // Do we already have a reference to a funclet parent?
                         if (!m_sfFuncletParent.IsNull())
                         {
@@ -1997,7 +2028,6 @@ ProcessFuncletsForGCReporting:
                                         STRESS_LOG0(LF_GCROOTS, LL_INFO100,
                                         "STACKWALK: Reached parent of funclet which didn't report GC roots, since funclet is already unwound.\n");
 
-                                        ExceptionTracker* pTracker = m_crawl.pThread->GetExceptionState()->GetCurrentExceptionTracker();
                                         if (pTracker->GetCallerOfActualHandlingFrame() == m_sfFuncletParent)
                                         {
                                             // we should not skip reporting for this parent frame