Fix potential deadlock when calling ExecutionManager::IsManagedCode

The function can deadlock if the calling thread was holding the
ExecutionManager writer lock.

Commit migrated from https://github.com/dotnet/coreclr/commit/a5b5d2f00993872e61c764e2a777f568cc5a8ba3

diff --git a/src/coreclr/src/pal/inc/pal.h b/src/coreclr/src/pal/inc/pal.h
index daf3813..db4a037 100644 (file)
--- a/src/coreclr/src/pal/inc/pal.h
+++ b/src/coreclr/src/pal/inc/pal.h
@@ -5467,7 +5467,7 @@ PALAPI
 FlushProcessWriteBuffers();
 
 typedef void (*PAL_ActivationFunction)(CONTEXT *context);
-typedef BOOL (*PAL_SafeActivationCheckFunction)(SIZE_T ip);
+typedef BOOL (*PAL_SafeActivationCheckFunction)(SIZE_T ip, BOOL checkingCurrentThread);
 
 PALIMPORT
 VOID

diff --git a/src/coreclr/src/pal/src/exception/machexception.cpp b/src/coreclr/src/pal/src/exception/machexception.cpp
index cd4e26d..18eb8c4 100644 (file)
--- a/src/coreclr/src/pal/src/exception/machexception.cpp
+++ b/src/coreclr/src/pal/src/exception/machexception.cpp
@@ -1534,7 +1534,7 @@ InjectActivationInternal(CPalThread* pThread)
                                        &count);
             _ASSERT_MSG(MachRet == KERN_SUCCESS, "thread_get_state for x86_THREAD_STATE64\n");
 
-            if ((g_safeActivationCheckFunction != NULL) && g_safeActivationCheckFunction(ThreadState.__rip))
+            if ((g_safeActivationCheckFunction != NULL) && g_safeActivationCheckFunction(ThreadState.__rip, /* checkingCurrentThread */ FALSE))
             {
                 // TODO: it would be nice to preserve the red zone in case a jitter would want to use it
                 // Do we really care about unwinding through the wrapper?

diff --git a/src/coreclr/src/pal/src/exception/signal.cpp b/src/coreclr/src/pal/src/exception/signal.cpp
index b1b8b29..30d1236 100644 (file)
--- a/src/coreclr/src/pal/src/exception/signal.cpp
+++ b/src/coreclr/src/pal/src/exception/signal.cpp
@@ -414,7 +414,7 @@ static void inject_activation_handler(int code, siginfo_t *siginfo, void *contex
                 &winContext, 
                 CONTEXT_CONTROL | CONTEXT_INTEGER | CONTEXT_FLOATING_POINT);
 
-            if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext)))
+            if (g_safeActivationCheckFunction(CONTEXTGetPC(&winContext), /* checkingCurrentThread */ TRUE))
             {
                 g_activationFunction(&winContext);
             }

diff --git a/src/coreclr/src/vm/exceptionhandling.cpp b/src/coreclr/src/vm/exceptionhandling.cpp
index c17faf1..9c6747f 100644 (file)
--- a/src/coreclr/src/vm/exceptionhandling.cpp
+++ b/src/coreclr/src/vm/exceptionhandling.cpp
@@ -5034,11 +5034,17 @@ bool IsDivByZeroAnIntegerOverflow(PCONTEXT pContext)
 
 BOOL PALAPI IsSafeToHandleHardwareException(PCONTEXT contextRecord, PEXCEPTION_RECORD exceptionRecord)
 {
+    Thread *pThread = GetThread();
     PCODE controlPc = GetIP(contextRecord);
+    // It is safe to call the ExecutionManager::IsManagedCode only if the current thread is in
+    // the cooperative mode. Otherwise ExecutionManager::IsManagedCode could deadlock if 
+    // the exception happened when the thread was holding the ExecutionManager's writer lock.
+    // When the thread is in preemptive mode, we know for sure that it is not executing managed code.
+    BOOL isManagedCode = (pThread != NULL && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(controlPc));
     return g_fEEStarted && (
         exceptionRecord->ExceptionCode == STATUS_BREAKPOINT || 
         exceptionRecord->ExceptionCode == STATUS_SINGLE_STEP ||
-        ExecutionManager::IsManagedCode(controlPc) || 
+        isManagedCode ||
         IsIPInMarkedJitHelper(controlPc));
 }
 
@@ -5050,8 +5056,10 @@ VOID PALAPI HandleHardwareException(PAL_SEHException* ex)
     {
         // A hardware exception is handled only if it happened in a jitted code or 
         // in one of the JIT helper functions (JIT_MemSet, ...)
+        Thread *pThread = GetThread();
         PCODE controlPc = GetIP(&ex->ContextRecord);
-        if (ExecutionManager::IsManagedCode(controlPc) && IsGcMarker(ex->ExceptionRecord.ExceptionCode, &ex->ContextRecord))
+        BOOL isManagedCode = (pThread != NULL && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(controlPc));
+        if (isManagedCode && IsGcMarker(ex->ExceptionRecord.ExceptionCode, &ex->ContextRecord))
         {
             RtlRestoreContext(&ex->ContextRecord, &ex->ExceptionRecord);
             UNREACHABLE();

diff --git a/src/coreclr/src/vm/threadsuspend.cpp b/src/coreclr/src/vm/threadsuspend.cpp
index b571504..6d25012 100644 (file)
--- a/src/coreclr/src/vm/threadsuspend.cpp
+++ b/src/coreclr/src/vm/threadsuspend.cpp
@@ -8348,9 +8348,16 @@ retry_for_debugger:
 
 // This function is called by PAL to check if the specified instruction pointer
 // is in a function where we can safely inject activation. 
-BOOL PALAPI CheckActivationSafePoint(SIZE_T ip)
+BOOL PALAPI CheckActivationSafePoint(SIZE_T ip, BOOL checkingCurrentThread)
 {
-    return ExecutionManager::IsManagedCode(ip);
+    Thread *pThread = GetThread();
+    // It is safe to call the ExecutionManager::IsManagedCode only if we are making the check for
+    // a thread different from the current one or if the current thread is in the cooperative mode.
+    // Otherwise ExecutionManager::IsManagedCode could deadlock if the activation happened when the
+    // thread was holding the ExecutionManager's writer lock.
+    // When the thread is in preemptive mode, we know for sure that it is not executing managed code.
+    BOOL checkForManagedCode = !checkingCurrentThread || (pThread != NULL && pThread->PreemptiveGCDisabled());
+    return checkForManagedCode && ExecutionManager::IsManagedCode(ip);
 }
 
 // This function is called when a GC is pending. It tries to ensure that the current
@@ -8362,7 +8369,7 @@ BOOL PALAPI CheckActivationSafePoint(SIZE_T ip)
 //
 //     - If the thread is in interruptible managed code, we will push a frame that
 //       has information about the context that was interrupted and then switch to
-//       premptive GC mode so that the pending GC can proceed, and then switch back.
+//       preemptive GC mode so that the pending GC can proceed, and then switch back.
 //
 //     - If the thread is in uninterruptible managed code, we will patch the return
 //       address to take the thread to the appropriate stub (based on the return 
@@ -8379,7 +8386,7 @@ void PALAPI HandleGCSuspensionForInterruptedThread(CONTEXT *interruptedContext)
 
     // This function can only be called when the interrupted thread is in 
     // an activation safe point.
-    _ASSERTE(CheckActivationSafePoint(ip));
+    _ASSERTE(CheckActivationSafePoint(ip, /* checkingCurrentThread */ TRUE));
 
     Thread::WorkingOnThreadContextHolder workingOnThreadContext(pThread);
     if (!workingOnThreadContext.Acquired())