[debugger] Fix crash during Async Break when APC and CET are enabled

src/coreclr/debug/ee/controller.cpp

@@ -4469,7 +4469,19 @@ bool DebuggerController::DispatchNativeException(EXCEPTION_RECORD *pException,
         ThisFunctionMayHaveTriggerAGC();
     }
 #endif
-
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+    if (pCurThread->m_State & Thread::TS_SSToExitApcCall)
+    {
+        if (!CheckActivationSafePoint(GetIP(pContext)))
+        {
+            return FALSE;
+        }
+        pCurThread->SetThreadState(Thread::TS_SSToExitApcCallDone);
+        pCurThread->ResetThreadState(Thread::TS_SSToExitApcCall);        
+        DebuggerController::UnapplyTraceFlag(pCurThread);
+        pCurThread->MarkForSuspensionAndWait(Thread::TS_DebugSuspendPending);
+    }
+#endif
 
 
     // Must restore the filter context. After the filter context is gone, we're

src/coreclr/debug/ee/debugger.cpp

@@ -15012,6 +15012,14 @@ HRESULT Debugger::FuncEvalSetup(DebuggerIPCE_FuncEvalInfo *pEvalInfo,
         return CORDBG_E_ILLEGAL_IN_STACK_OVERFLOW;
     }
 
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+    if (pThread->m_hasPendingActivation)
+    {
+        _ASSERTE(!"Should never get here with a pending activation. (Debugger::FuncEvalSetup)");
+        return CORDBG_E_ILLEGAL_IN_NATIVE_CODE;
+    }
+#endif    
+
     bool fInException = pEvalInfo->evalDuringException;
 
     // The thread has to be at a GC safe place for now, just in case the func eval causes a collection. Processing an
@@ -16783,6 +16791,15 @@ void Debugger::ExternalMethodFixupNextStep(PCODE address)
 {
     DebuggerController::DispatchExternalMethodFixup(address);
 }
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+void Debugger::SingleStepToExitApcCall(Thread* pThread, CONTEXT *interruptedContext)
+{
+    pThread->SetThreadState(Thread::TS_SSToExitApcCall);
+    g_pEEInterface->SetThreadFilterContext(pThread, interruptedContext);
+    DebuggerController::EnableSingleStep(pThread);
+    g_pEEInterface->SetThreadFilterContext(pThread, NULL);
+}
+#endif //FEATURE_SPECIAL_USER_MODE_APC
 #endif //DACCESS_COMPILE
 
 unsigned FuncEvalFrame::GetFrameAttribs_Impl(void)

src/coreclr/debug/ee/debugger.h

@@ -3063,6 +3063,9 @@ class Debugger : public DebugInterface
     // Used by Debugger::FirstChanceNativeException to update the context from out of process
     void SendSetThreadContextNeeded(CONTEXT *context, DebuggerSteppingInfo *pDebuggerSteppingInfo = NULL);
     BOOL IsOutOfProcessSetContextEnabled();
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+    void SingleStepToExitApcCall(Thread* pThread, CONTEXT *interruptedContext);
+#endif // FEATURE_SPECIAL_USER_MODE_APC        
 };
 
 

src/coreclr/vm/dbginterface.h

@@ -412,6 +412,9 @@ class DebugInterface
     virtual HRESULT IsMethodDeoptimized(Module *pModule, mdMethodDef methodDef, BOOL *pResult) = 0;
     virtual void MulticastTraceNextStep(DELEGATEREF pbDel, INT32 count) = 0;
     virtual void ExternalMethodFixupNextStep(PCODE address) = 0;
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+    virtual void SingleStepToExitApcCall(Thread* pThread, CONTEXT *interruptedContext) = 0;
+#endif // FEATURE_SPECIAL_USER_MODE_APC        
 #endif //DACCESS_COMPILE
 };
 

src/coreclr/vm/threads.h

@@ -481,6 +481,7 @@ class Thread
     friend void STDCALL OnHijackWorker(HijackArgs * pArgs);
 #ifdef FEATURE_THREAD_ACTIVATION
     friend void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext);
+    friend void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext, bool suspendForDebugger);
     friend BOOL CheckActivationSafePoint(SIZE_T ip);
 #endif // FEATURE_THREAD_ACTIVATION
 
@@ -549,7 +550,7 @@ class Thread
         TS_Hijacked               = 0x00000080,    // Return address has been hijacked
 #endif // FEATURE_HIJACK
 
-        // unused                 = 0x00000100,
+        TS_SSToExitApcCall        = 0x00000100,    // Enable SS and resume the thread to exit an APC Call and keep the thread in suspend state
         TS_Background             = 0x00000200,    // Thread is a background thread
         TS_Unstarted              = 0x00000400,    // Thread has never been started
         TS_Dead                   = 0x00000800,    // Thread is dead
@@ -566,7 +567,7 @@ class Thread
         TS_ReportDead             = 0x00010000,    // in WaitForOtherThreads()
         TS_FullyInitialized       = 0x00020000,    // Thread is fully initialized and we are ready to broadcast its existence to external clients
 
-        // unused                 = 0x00040000,
+        TS_SSToExitApcCallDone    = 0x00040000,    // The thread exited an APC Call and it is already resumed and paused on SS
 
         TS_SyncSuspended          = 0x00080000,    // Suspended via WaitSuspendEvent
         TS_DebugWillSync          = 0x00100000,    // Debugger will wait for this thread to sync
@@ -2570,7 +2571,9 @@ class Thread
     //-------------------------------------------------------------
     // Waiting & Synchronization
     //-------------------------------------------------------------
-
+friend class DebuggerController;
+protected:
+    void MarkForSuspensionAndWait(ULONG bit);
     // For suspends.  The thread waits on this event.  A client sets the event to cause
     // the thread to resume.
     void    WaitSuspendEvents();

src/coreclr/vm/threadsuspend.cpp

@@ -4215,6 +4215,18 @@ bool Thread::SysSweepThreadsForDebug(bool forceSync)
         if ((thread->m_State & TS_DebugWillSync) == 0)
             continue;
 
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+        if (thread->m_State & Thread::TS_SSToExitApcCallDone)
+        {
+            thread->ResetThreadState(Thread::TS_SSToExitApcCallDone);
+            goto Label_MarkThreadAsSynced;
+        }
+        if (thread->m_State & Thread::TS_SSToExitApcCall)
+        {
+            continue;
+        }
+#endif
+
         if (!UseContextBasedThreadRedirection())
         {
             // On platforms that do not support safe thread suspension we either
@@ -5331,6 +5343,19 @@ BOOL Thread::HandledJITCase()
 #endif // FEATURE_HIJACK
 
 // Some simple helpers to keep track of the threads we are waiting for
+void Thread::MarkForSuspensionAndWait(ULONG bit)
+{
+    CONTRACTL {
+        NOTHROW;
+        GC_NOTRIGGER;
+    }
+    CONTRACTL_END;
+    m_DebugSuspendEvent.Reset();
+    InterlockedOr((LONG*)&m_State, bit);
+    ThreadStore::IncrementTrapReturningThreads();
+    m_DebugSuspendEvent.Wait(INFINITE,FALSE);
+}
+
 void Thread::MarkForSuspension(ULONG bit)
 {
     CONTRACTL {
@@ -5753,7 +5778,8 @@ BOOL CheckActivationSafePoint(SIZE_T ip)
 //       address to take the thread to the appropriate stub (based on the return
 //       type of the method) which will then handle preparing the thread for GC.
 //
-void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext)
+
+void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext, bool suspendForDebugger)
 {
     struct AutoClearPendingThreadActivation
     {
@@ -5789,6 +5815,18 @@ void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext)
     if (!codeInfo.IsValid())
         return;
 
+#ifdef FEATURE_SPECIAL_USER_MODE_APC
+    // It's not allowed to change the IP while paused in an APC Callback for security reasons if CET is turned on
+    // So we enable the single step in the thread that is running the APC Callback
+    // and then it will be paused using single step exception after exiting the APC callback
+    // this will allow the debugger to setIp to execute FuncEvalHijack.
+    if (suspendForDebugger)
+    {
+        g_pDebugInterface->SingleStepToExitApcCall(pThread, interruptedContext);
+        return;
+    }
+#endif        
+
     DWORD addrOffset = codeInfo.GetRelOffset();
 
     ICodeManager *pEECM = codeInfo.GetCodeManager();
@@ -5863,6 +5901,11 @@ void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext)
     }
 }
 
+void HandleSuspensionForInterruptedThread(CONTEXT *interruptedContext)
+{
+    HandleSuspensionForInterruptedThread(interruptedContext, false);
+}
+
 #ifdef FEATURE_SPECIAL_USER_MODE_APC
 void Thread::ApcActivationCallback(ULONG_PTR Parameter)
 {
@@ -5889,10 +5932,10 @@ void Thread::ApcActivationCallback(ULONG_PTR Parameter)
 
     switch (reason)
     {
-        case ActivationReason::SuspendForGC:
         case ActivationReason::SuspendForDebugger:
+        case ActivationReason::SuspendForGC:
         case ActivationReason::ThreadAbort:
-            HandleSuspensionForInterruptedThread(pContext);
+            HandleSuspensionForInterruptedThread(pContext, reason == ActivationReason::SuspendForDebugger);
             break;
 
         default: