Merge pull request #34 from x64dbg/per-thread-stepping

Per-thread stepping state + fix hardware-breakpoint reset race on thread exit

TitanEngine/Global.Debugger.cpp

@@ -29,9 +29,7 @@ DWORD ProcessExitCode = 0;
 HANDLE DBGFileHandle;
 std::vector<ULONG_PTR> tlsCallBackList;
 std::vector<PROCESS_ITEM_DATA> hListProcess;
-DWORD engineStepCount = 0;
+std::unordered_map<DWORD, EngineStepThreadState> engineStepThreads;
-LPVOID engineStepCallBack = NULL;
-bool engineStepActive = false;
 bool engineProcessIsNowDetached = false;
 DWORD DBGCode = DBG_CONTINUE;
 bool engineFileIsBeingDebugged = false;
@@ -90,6 +88,7 @@ void DebuggerReset()
     std::vector<BreakPointDetail>().swap(BreakPointBuffer);
     std::unordered_map<ULONG_PTR, MemoryBreakpointPageDetail>().swap(MemoryBreakpointPages);
     recentlyDeletedBpx.clear();
+    engineStepThreads.clear();
 }
 
 void ClearProcessList()

TitanEngine/Global.Debugger.h

@@ -2,6 +2,7 @@
 #define _GLOBAL_DEBUGGER_H
 
 #include <vector>
+#include <unordered_map>
 #include <Windows.h>
 
 extern HARDWARE_DATA DebugRegister[4];
@@ -29,9 +30,18 @@ extern DWORD ProcessExitCode;
 extern HANDLE DBGFileHandle;
 extern std::vector<ULONG_PTR> tlsCallBackList;
 extern std::vector<PROCESS_ITEM_DATA> hListProcess;
-extern DWORD engineStepCount;
+// Per-thread pending single-step state. A single-step (user step or the internal
-extern LPVOID engineStepCallBack;
+// step-over of a breakpoint) is always armed on a specific thread by setting that
-extern bool engineStepActive;
+// thread's trap flag, but the resulting STATUS_SINGLE_STEP event can be delivered
+// interleaved with other threads' events. Keying the step state by thread id makes
+// each thread consume only its own step, and allows several threads to be stepped
+// independently (matching GleeBug).
+struct EngineStepThreadState
+{
+    LPVOID callback; // step callback to fire when the step completes
+    DWORD count;     // remaining repeats (0 = fire the callback on the next step)
+};
+extern std::unordered_map<DWORD, EngineStepThreadState> engineStepThreads;
 extern bool engineProcessIsNowDetached;
 extern DWORD DBGCode;
 extern bool engineFileIsBeingDebugged;

TitanEngine/TitanEngine.Debugger.Control.cpp

@@ -38,7 +38,9 @@ __declspec(dllexport) void TITCALL ForceClose()
 __declspec(dllexport) void TITCALL StepInto(LPVOID StepCallBack)
 {
     EnterCriticalSection(&engineStepActiveCr);
-    if(!engineStepActive)
+    // Arm a single-step for the current event thread only. State is per-thread, so a
+    // step already pending on another thread does not block this one.
+    if(engineStepThreads.find(DBGEvent.dwThreadId) == engineStepThreads.end())
     {
         ULONG_PTR ueCurrentPosition = GetContextData(UE_CIP);
         unsigned char instr[16];
@@ -60,9 +62,7 @@ __declspec(dllexport) void TITCALL StepInto(LPVOID StepCallBack)
             myDBGContext.EFlags |= UE_TRAP_FLAG;
             SetThreadContext(hActiveThread, &myDBGContext);
             EngineCloseHandle(hActiveThread);
-            engineStepActive = true;
+            engineStepThreads[DBGEvent.dwThreadId] = { StepCallBack, 0 };
-            engineStepCallBack = StepCallBack;
-            engineStepCount = 0;
         }
     }
     LeaveCriticalSection(&engineStepActiveCr);
@@ -93,7 +93,11 @@ __declspec(dllexport) void TITCALL StepOut(LPVOID StepOut, bool StepFinal)
 __declspec(dllexport) void TITCALL SingleStep(DWORD StepCount, LPVOID StepCallBack)
 {
     StepInto(StepCallBack);
-    engineStepCount = StepCount - 1; //We already stepped once
+    EnterCriticalSection(&engineStepActiveCr);
+    auto it = engineStepThreads.find(DBGEvent.dwThreadId);
+    if(it != engineStepThreads.end())
+        it->second.count = StepCount - 1; //We already stepped once
+    LeaveCriticalSection(&engineStepActiveCr);
 }
 
 __declspec(dllexport) void TITCALL SetNextDbgContinueStatus(DWORD SetDbgCode)

TitanEngine/TitanEngine.Debugger.DebugLoop.cpp

@@ -16,22 +16,28 @@
 static void engineStep()
 {
     EnterCriticalSection(&engineStepActiveCr);
-    if(engineStepActive)
+    // Only deliver the pending single-step to the thread it was armed for. State is
+    // per-thread, so an automatic breakpoint step-over on a different thread cannot
+    // consume this thread's step or fire its callback on the wrong thread.
+    auto it = engineStepThreads.find(DBGEvent.dwThreadId);
+    if(it != engineStepThreads.end())
     {
         DBGCode = DBG_CONTINUE;
-        if(engineStepCount == 0)
+        if(it->second.count == 0)
         {
             typedef void(TITCALL * fCustomBreakPoint)(void);
-            auto cbStep = fCustomBreakPoint(engineStepCallBack);
+            auto cbStep = fCustomBreakPoint(it->second.callback);
-            engineStepActive = false;
+            engineStepThreads.erase(it);
-            engineStepCallBack = NULL;
             LeaveCriticalSection(&engineStepActiveCr);
             cbStep();
         }
         else
         {
-            SingleStep(engineStepCount, engineStepCallBack);
+            auto count = it->second.count;
+            auto callback = it->second.callback;
+            engineStepThreads.erase(it); // SingleStep re-arms this thread's entry
             LeaveCriticalSection(&engineStepActiveCr);
+            SingleStep(count, callback);
         }
     }
     else
@@ -154,7 +160,54 @@ __declspec(dllexport) void TITCALL DebugLoop()
             {
                 if(ThreadBeingProcessed != 0 && DBGEvent.dwThreadId == ThreadBeingProcessed)
                 {
-                    // Resume the other threads since the thread being processed is exiting
+                    // The thread was in the middle of a breakpoint step-over (its
+                    // breakpoint was temporarily removed and a restore was pending on its
+                    // next single-step) but it is exiting before that single-step arrives.
+                    // Complete the restore now, while the other threads are still
+                    // suspended - otherwise the breakpoint is left removed and the next
+                    // thread's event would consume this dead thread's orphaned reset state
+                    // and be misinterpreted as a step-over reset, producing a stray
+                    // single-step that is passed to (and crashes) the debuggee.
+                    if(ResetBPX)
+                    {
+                        EnableBPX(ResetBPXAddressTo);
+                        ResetBPXAddressTo = 0;
+                        ResetBPX = false;
+                    }
+                    if(ResetHwBPX)
+                    {
+                        // Re-enable the stepped-over slot in the authoritative DebugRegister[]
+                        // table (DeleteHardwareBreakPoint cleared it when the breakpoint was
+                        // hit), then re-arm every enabled hardware breakpoint on each still-
+                        // suspended thread from that table - exactly as CREATE_THREAD arms a
+                        // new thread. Rebuilding from DebugRegister[] rather than from the
+                        // exiting thread's (gone) context or a live thread's DR7 (which may
+                        // have been cleared) is what keeps the OTHER hardware breakpoints from
+                        // being disabled. This must run before the threads are resumed:
+                        // SetThreadContext does not reliably update a running thread's debug
+                        // registers.
+                        int drIdx = (DebugRegisterXId == UE_DR0) ? 0 : (DebugRegisterXId == UE_DR1) ? 1 : (DebugRegisterXId == UE_DR2) ? 2 : 3;
+                        DebugRegister[drIdx] = DebugRegisterX;
+                        static const DWORD drIds[4] = { UE_DR0, UE_DR1, UE_DR2, UE_DR3 };
+                        for(auto & itr : SuspendedThreads)
+                            for(int s = 0; s < 4; s++)
+                                if(DebugRegister[s].DrxEnabled)
+                                    SetHardwareBreakPointEx(itr.second.hThread, DebugRegister[s].DrxBreakAddress, drIds[s], DebugRegister[s].DrxBreakPointType, DebugRegister[s].DrxBreakPointSize, (LPVOID)DebugRegister[s].DrxCallBack, NULL);
+                        ResetHwBPX = false;
+                    }
+                    if(ResetMemBPX)
+                    {
+                        ResetMemBpxCallback();
+                        if(ResetMemBpxExtraCallback != nullptr)
+                        {
+                            ResetMemBpxExtraCallback();
+                            ResetMemBpxExtraCallback = nullptr;
+                        }
+                        ResetMemBPX = false;
+                    }
+                    PushfBPX = false;
+
+                    // Resume the other threads now that the breakpoint has been restored.
                     for(auto & itr : SuspendedThreads)
                         ResumeThread(itr.second.hThread);
 
@@ -344,6 +397,15 @@ __declspec(dllexport) void TITCALL DebugLoop()
                     break;
                 }
             }
+
+            // Drop any per-thread single-step state for the exiting thread. A thread can
+            // exit after StepInto() armed its step but before the trap event is delivered
+            // (e.g. it is terminated externally); without this, a reused thread id would
+            // inherit the stale entry - StepInto() would decline to arm the trap flag, or
+            // a later single-step could fire the dead thread's callback.
+            EnterCriticalSection(&engineStepActiveCr);
+            engineStepThreads.erase(DBGEvent.dwThreadId);
+            LeaveCriticalSection(&engineStepActiveCr);
         }
         break;
 
@@ -1288,7 +1350,11 @@ __declspec(dllexport) void TITCALL DebugLoop()
             //general unhandled exception callback
             if(DBGCode == DBG_EXCEPTION_NOT_HANDLED)
             {
-                engineStepActive = false;
+                // The current thread's pending single-step (if any) did not complete
+                // normally; cancel it. Other threads' steps are unaffected.
+                EnterCriticalSection(&engineStepActiveCr);
+                engineStepThreads.erase(DBGEvent.dwThreadId);
+                LeaveCriticalSection(&engineStepActiveCr);
 
                 if(DBGCustomHandler->chUnhandledException != NULL)
                 {