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;