mirror of https://github.com/x64dbg/TitanEngine
Address review: preserve other hardware breakpoints and per-thread step state on thread exit
Two fixes from PR review of the thread-exit reset-recovery: - When restoring the hardware breakpoint after the stepped-over thread exits, rebuild each still-suspended thread's debug registers from the authoritative DebugRegister[] table (re-arming every enabled slot, as CREATE_THREAD does) instead of from the exiting thread's now-gone context. The old code rebuilt DR7 from GetContextData() (the dead thread), writing a DR7 with only the restored slot to every thread and silently disabling any other hardware breakpoint. The restore is also moved before the threads are resumed, since SetThreadContext does not reliably update the debug registers of a running thread. - Erase the per-thread single-step state in the EXIT_THREAD handler. A thread can exit after StepInto() armed its step but before the trap event arrives; leaving the entry behind means a reused thread id inherits it (StepInto declines to arm, or a later single-step fires the dead thread's callback).
This commit is contained in:
parent
ba2a3aa7c8
commit
ed4bdd1a01
|
|
@ -160,20 +160,14 @@ __declspec(dllexport) void TITCALL DebugLoop()
|
|||
{
|
||||
if(ThreadBeingProcessed != 0 && DBGEvent.dwThreadId == ThreadBeingProcessed)
|
||||
{
|
||||
// Resume the other threads since the thread being processed is exiting
|
||||
for(auto & itr : SuspendedThreads)
|
||||
ResumeThread(itr.second.hThread);
|
||||
|
||||
SuspendedThreads.clear();
|
||||
ThreadBeingProcessed = 0;
|
||||
|
||||
// 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: 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.
|
||||
// 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);
|
||||
|
|
@ -182,7 +176,23 @@ __declspec(dllexport) void TITCALL DebugLoop()
|
|||
}
|
||||
if(ResetHwBPX)
|
||||
{
|
||||
SetHardwareBreakPoint(DebugRegisterX.DrxBreakAddress, DebugRegisterXId, DebugRegisterX.DrxBreakPointType, DebugRegisterX.DrxBreakPointSize, (LPVOID)DebugRegisterX.DrxCallBack);
|
||||
// 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)
|
||||
|
|
@ -196,6 +206,13 @@ __declspec(dllexport) void TITCALL DebugLoop()
|
|||
ResetMemBPX = false;
|
||||
}
|
||||
PushfBPX = false;
|
||||
|
||||
// Resume the other threads now that the breakpoint has been restored.
|
||||
for(auto & itr : SuspendedThreads)
|
||||
ResumeThread(itr.second.hThread);
|
||||
|
||||
SuspendedThreads.clear();
|
||||
ThreadBeingProcessed = 0;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -380,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;
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue