diff --git a/TitanEngine/Global.Debugger.cpp b/TitanEngine/Global.Debugger.cpp index f267967..11e4c5f 100644 --- a/TitanEngine/Global.Debugger.cpp +++ b/TitanEngine/Global.Debugger.cpp @@ -29,9 +29,7 @@ DWORD ProcessExitCode = 0; HANDLE DBGFileHandle; std::vector tlsCallBackList; std::vector hListProcess; -DWORD engineStepCount = 0; -LPVOID engineStepCallBack = NULL; -bool engineStepActive = false; +std::unordered_map engineStepThreads; bool engineProcessIsNowDetached = false; DWORD DBGCode = DBG_CONTINUE; bool engineFileIsBeingDebugged = false; @@ -90,6 +88,7 @@ void DebuggerReset() std::vector().swap(BreakPointBuffer); std::unordered_map().swap(MemoryBreakpointPages); recentlyDeletedBpx.clear(); + engineStepThreads.clear(); } void ClearProcessList() diff --git a/TitanEngine/Global.Debugger.h b/TitanEngine/Global.Debugger.h index 1861756..d0b8e11 100644 --- a/TitanEngine/Global.Debugger.h +++ b/TitanEngine/Global.Debugger.h @@ -2,6 +2,7 @@ #define _GLOBAL_DEBUGGER_H #include +#include #include extern HARDWARE_DATA DebugRegister[4]; @@ -29,9 +30,18 @@ extern DWORD ProcessExitCode; extern HANDLE DBGFileHandle; extern std::vector tlsCallBackList; extern std::vector hListProcess; -extern DWORD engineStepCount; -extern LPVOID engineStepCallBack; -extern bool engineStepActive; +// Per-thread pending single-step state. A single-step (user step or the internal +// step-over of a breakpoint) is always armed on a specific thread by setting that +// 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 engineStepThreads; extern bool engineProcessIsNowDetached; extern DWORD DBGCode; extern bool engineFileIsBeingDebugged; diff --git a/TitanEngine/TitanEngine.Debugger.Control.cpp b/TitanEngine/TitanEngine.Debugger.Control.cpp index 548f746..3984c0d 100644 --- a/TitanEngine/TitanEngine.Debugger.Control.cpp +++ b/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; - engineStepCallBack = StepCallBack; - engineStepCount = 0; + engineStepThreads[DBGEvent.dwThreadId] = { StepCallBack, 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) diff --git a/TitanEngine/TitanEngine.Debugger.DebugLoop.cpp b/TitanEngine/TitanEngine.Debugger.DebugLoop.cpp index d11889f..69ab90b 100644 --- a/TitanEngine/TitanEngine.Debugger.DebugLoop.cpp +++ b/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); - engineStepActive = false; - engineStepCallBack = NULL; + auto cbStep = fCustomBreakPoint(it->second.callback); + engineStepThreads.erase(it); 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) {