From d970cdb3dc344c9a4bbe38f54ede0afb13f274ca Mon Sep 17 00:00:00 2001 From: Xusheng Date: Fri, 14 Aug 2026 17:28:03 -0400 Subject: [PATCH] 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;