From d970cdb3dc344c9a4bbe38f54ede0afb13f274ca Mon Sep 17 00:00:00 2001 From: Xusheng Date: Fri, 14 Aug 2026 17:28:03 -0400 Subject: [PATCH 1/2] Terminate the TTD target on the engine thread DbgEng clients are thread-affine, and ExitDispatch() is the only call documented as safe from another thread. Quit() was calling TerminateCurrentProcess() from the requesting thread while the engine thread sat in DispatchCallbacks(), which faults inside WinDbg 1.2606's new V8-based JSProvider.dll. Quit() now only raises m_terminateRequested and wakes the engine loop, which performs the terminate itself. Reproduced with a harness mirroring the adapter: 5/5 crashes before, 0/10 after on 1.2606, and 0/3 on 1.2603. WinDbg 1.2402 and 1.2603 never crashed either way. Fixes #1129 Co-Authored-By: Claude Opus 5 (1M context) --- core/adapters/dbgengadapter.cpp | 17 +++++++++++++++++ core/adapters/dbgengadapter.h | 9 +++++++++ core/adapters/dbgengttdadapter.cpp | 26 ++++++++++++++++++++++---- core/adapters/dbgengttdadapter.h | 1 + 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/core/adapters/dbgengadapter.cpp b/core/adapters/dbgengadapter.cpp index ff4c2934..edd1587f 100644 --- a/core/adapters/dbgengadapter.cpp +++ b/core/adapters/dbgengadapter.cpp @@ -708,6 +708,14 @@ void DbgEngAdapter::EngineLoop() // WaitForEvent(). The real purpose of this call is to wait until the UI/API initiates another control // operation, which then calls ExitDispatch(), which causes the DispatchCallbacks() to return. m_debugClient->DispatchCallbacks(INFINITE); + + // A DbgEng client belongs to the thread that created it, and ExitDispatch() is the only call + // documented as safe to make from another thread. So Quit() only raises this flag and wakes us + // up; the terminate itself has to happen here. Doing it from the requesting thread while this + // one sits in DispatchCallbacks() faults inside WinDbg's data model JS provider on 1.2606 + // (#1129). + if (m_terminateRequested.exchange(false)) + TerminateTargetOnEngineThread(); } // TODO: add step branch and step backs else if ((execution_status == DEBUG_STATUS_GO) || (execution_status == DEBUG_STATUS_STEP_INTO) @@ -908,6 +916,15 @@ bool DbgEngAdapter::Detach() return true; } +bool DbgEngAdapter::TerminateTargetOnEngineThread() +{ + if (!this->m_debugClient) + return false; + + return this->m_debugClient->TerminateProcesses() == S_OK; +} + + bool DbgEngAdapter::Quit() { m_aboutToBeKilled = true; diff --git a/core/adapters/dbgengadapter.h b/core/adapters/dbgengadapter.h index 7f007033..7f5469c1 100644 --- a/core/adapters/dbgengadapter.h +++ b/core/adapters/dbgengadapter.h @@ -21,6 +21,7 @@ limitations under the License. #define NOMINMAX #include #include +#include #include namespace BinaryNinjaDebugger { @@ -138,6 +139,10 @@ namespace BinaryNinjaDebugger { virtual bool Start(); virtual void Reset(); + // Kills the target. Called by EngineLoop() on the thread that created the debug client, never + // directly from Quit(), because DbgEng clients are thread-affine. + virtual bool TerminateTargetOnEngineThread(); + std::vector m_debug_breakpoints {}; bool m_lastOperationIsStepInto = false; @@ -156,6 +161,10 @@ namespace BinaryNinjaDebugger { bool m_aboutToBeKilled = false; + // Raised by Quit() so that EngineLoop() performs the terminate on the thread that owns the + // debug client. See TerminateTargetOnEngineThread(). + std::atomic m_terminateRequested {false}; + std::string m_pdbFileName {}; bool m_usePDBFileName = true; diff --git a/core/adapters/dbgengttdadapter.cpp b/core/adapters/dbgengttdadapter.cpp index 58dd6ad7..c5983bb1 100644 --- a/core/adapters/dbgengttdadapter.cpp +++ b/core/adapters/dbgengttdadapter.cpp @@ -303,18 +303,36 @@ bool DbgEngTTDAdapter::SupportFeature(DebugAdapterCapacity feature) } -bool DbgEngTTDAdapter::Quit() +bool DbgEngTTDAdapter::TerminateTargetOnEngineThread() { - m_aboutToBeKilled = true; - m_lastOperationIsStepInto = false; if (!this->m_debugClient) return false; // I am not sure why TerminateProcesses() would not work. It just let the target run freely till the end of the // trace and not terminating the process at all. - if (this->m_debugClient->TerminateCurrentProcess() != S_OK) + return this->m_debugClient->TerminateCurrentProcess() == S_OK; +} + + +bool DbgEngTTDAdapter::Quit() +{ + m_aboutToBeKilled = true; + m_lastOperationIsStepInto = false; + if (!this->m_debugClient) return false; + // Terminating from this thread crashes inside WinDbg's data model JS provider on 1.2606 (#1129): + // the engine thread owns the debug client and is parked in DispatchCallbacks(), and DbgEng clients + // are thread-affine. Ask that thread to do it and wake it up; ExitDispatch() is the one call that is + // documented as safe to make from here. + m_terminateRequested = true; + + // If the trace is being replayed rather than sitting at a break, the engine thread is inside + // WaitForEvent() where ExitDispatch() will not reach it. SetInterrupt() is safe from any thread and + // brings it back to a break, where the request above is picked up. + if (m_debugControl && (ExecStatus() != DEBUG_STATUS_BREAK)) + m_debugControl->SetInterrupt(DEBUG_INTERRUPT_ACTIVE); + m_debugClient->ExitDispatch(reinterpret_cast(m_debugClient)); return true; } diff --git a/core/adapters/dbgengttdadapter.h b/core/adapters/dbgengttdadapter.h index f493ce7b..12ce6e23 100644 --- a/core/adapters/dbgengttdadapter.h +++ b/core/adapters/dbgengttdadapter.h @@ -36,6 +36,7 @@ namespace BinaryNinjaDebugger { bool Start() override; void Reset() override; + bool TerminateTargetOnEngineThread() override; bool GoReverse() override; bool StepIntoReverse() override; From adaddc0223cbdde59b80f0ce65abe81f343b7501 Mon Sep 17 00:00:00 2001 From: Xusheng Date: Mon, 17 Aug 2026 14:48:59 -0400 Subject: [PATCH 2/2] Do not latch a terminate request into the next session Review feedback on #1182. m_terminateRequested was only ever cleared by EngineLoop() consuming it, but the controller reuses one adapter object across launches, so a request the engine loop never got to (the trace reaching its end first, say) stayed latched and would kill the next session's target at its first control operation. Clear it once per session, at the top of EngineLoop(): both the launch and the attach path run it on the engine thread, and the worker queue cannot deliver a Quit() before the launch operation completes, so no live request can be lost there. Also log a failed terminate. Quit() has already reported success to the controller by then, so the engine loop is the only place the failure can be surfaced. Co-Authored-By: Claude Opus 5 (1M context) --- core/adapters/dbgengadapter.cpp | 11 +++++++++-- core/adapters/dbgengadapter.h | 4 +++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/core/adapters/dbgengadapter.cpp b/core/adapters/dbgengadapter.cpp index edd1587f..b444980d 100644 --- a/core/adapters/dbgengadapter.cpp +++ b/core/adapters/dbgengadapter.cpp @@ -655,6 +655,11 @@ void DbgEngAdapter::EngineLoop() bool outputStateOnStop = settings->Get("debugger.dbgEngOutputStateOnStop"); m_lastExecutionStatus = DEBUG_STATUS_NO_DEBUGGEE; + // The controller reuses one adapter object across launches, so a terminate request that the previous + // session left unserviced (it exited on its own before we picked the request up) would otherwise be + // latched here and kill this target. The worker queue cannot deliver a Quit() until the launch + // operation completes, which needs the stop event posted below, so no live request can be lost here. + m_terminateRequested = false; bool finished = false; while (true) { @@ -714,8 +719,10 @@ void DbgEngAdapter::EngineLoop() // up; the terminate itself has to happen here. Doing it from the requesting thread while this // one sits in DispatchCallbacks() faults inside WinDbg's data model JS provider on 1.2606 // (#1129). - if (m_terminateRequested.exchange(false)) - TerminateTargetOnEngineThread(); + if (m_terminateRequested.exchange(false) && !TerminateTargetOnEngineThread()) + // Quit() has already reported success to its caller, so this is the only place the + // failure can be surfaced. + LogWarn("Failed to terminate the target"); } // TODO: add step branch and step backs else if ((execution_status == DEBUG_STATUS_GO) || (execution_status == DEBUG_STATUS_STEP_INTO) diff --git a/core/adapters/dbgengadapter.h b/core/adapters/dbgengadapter.h index 7f5469c1..3611a655 100644 --- a/core/adapters/dbgengadapter.h +++ b/core/adapters/dbgengadapter.h @@ -162,7 +162,9 @@ namespace BinaryNinjaDebugger { bool m_aboutToBeKilled = false; // Raised by Quit() so that EngineLoop() performs the terminate on the thread that owns the - // debug client. See TerminateTargetOnEngineThread(). + // debug client. See TerminateTargetOnEngineThread(). EngineLoop() clears it when a session + // starts, since the adapter object outlives a session and an unserviced request would + // otherwise be latched into the next one. std::atomic m_terminateRequested {false}; std::string m_pdbFileName {};