Terminate the TTD target on the engine thread

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;

core/adapters/dbgengadapter.h

@@ -21,6 +21,7 @@ limitations under the License.
 #define NOMINMAX
 #include <windows.h>
 #include <dbgeng.h>
+#include <atomic>
 #include <chrono>
 
 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<DebugBreakpoint> 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<bool> m_terminateRequested {false};
+
         std::string m_pdbFileName {};
         bool m_usePDBFileName = true;
 

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<PDEBUG_CLIENT>(m_debugClient));
 	return true;
 }

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;