Skip to content

Commit 14a179d

Browse files
xusheng6xusheng6
authored
Terminate the TTD target on the engine thread (#1182)
1 parent c3d5295 commit 14a179d

4 files changed

Lines changed: 58 additions & 4 deletions

File tree

‎core/adapters/dbgengadapter.cpp‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -655,6 +655,11 @@ void DbgEngAdapter::EngineLoop()
655655
bool outputStateOnStop = settings->Get<bool>("debugger.dbgEngOutputStateOnStop");
656656

657657
m_lastExecutionStatus = DEBUG_STATUS_NO_DEBUGGEE;
658+
// The controller reuses one adapter object across launches, so a terminate request that the previous
659+
// session left unserviced (it exited on its own before we picked the request up) would otherwise be
660+
// latched here and kill this target. The worker queue cannot deliver a Quit() until the launch
661+
// operation completes, which needs the stop event posted below, so no live request can be lost here.
662+
m_terminateRequested = false;
658663
bool finished = false;
659664
while (true)
660665
{
@@ -708,6 +713,16 @@ void DbgEngAdapter::EngineLoop()
708713
// WaitForEvent(). The real purpose of this call is to wait until the UI/API initiates another control
709714
// operation, which then calls ExitDispatch(), which causes the DispatchCallbacks() to return.
710715
m_debugClient->DispatchCallbacks(INFINITE);
716+
717+
// A DbgEng client belongs to the thread that created it, and ExitDispatch() is the only call
718+
// documented as safe to make from another thread. So Quit() only raises this flag and wakes us
719+
// up; the terminate itself has to happen here. Doing it from the requesting thread while this
720+
// one sits in DispatchCallbacks() faults inside WinDbg's data model JS provider on 1.2606
721+
// (#1129).
722+
if (m_terminateRequested.exchange(false) && !TerminateTargetOnEngineThread())
723+
// Quit() has already reported success to its caller, so this is the only place the
724+
// failure can be surfaced.
725+
LogWarn("Failed to terminate the target");
711726
}
712727
// TODO: add step branch and step backs
713728
else if ((execution_status == DEBUG_STATUS_GO) || (execution_status == DEBUG_STATUS_STEP_INTO)
@@ -908,6 +923,15 @@ bool DbgEngAdapter::Detach()
908923
return true;
909924
}
910925

926+
bool DbgEngAdapter::TerminateTargetOnEngineThread()
927+
{
928+
if (!this->m_debugClient)
929+
return false;
930+
931+
return this->m_debugClient->TerminateProcesses() == S_OK;
932+
}
933+
934+
911935
bool DbgEngAdapter::Quit()
912936
{
913937
m_aboutToBeKilled = true;

‎core/adapters/dbgengadapter.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ limitations under the License.
2121
#define NOMINMAX
2222
#include <windows.h>
2323
#include <dbgeng.h>
24+
#include <atomic>
2425
#include <chrono>
2526

2627
namespace BinaryNinjaDebugger {
@@ -138,6 +139,10 @@ namespace BinaryNinjaDebugger {
138139
virtual bool Start();
139140
virtual void Reset();
140141

142+
// Kills the target. Called by EngineLoop() on the thread that created the debug client, never
143+
// directly from Quit(), because DbgEng clients are thread-affine.
144+
virtual bool TerminateTargetOnEngineThread();
145+
141146
std::vector<DebugBreakpoint> m_debug_breakpoints {};
142147
bool m_lastOperationIsStepInto = false;
143148

@@ -156,6 +161,12 @@ namespace BinaryNinjaDebugger {
156161

157162
bool m_aboutToBeKilled = false;
158163

164+
// Raised by Quit() so that EngineLoop() performs the terminate on the thread that owns the
165+
// debug client. See TerminateTargetOnEngineThread(). EngineLoop() clears it when a session
166+
// starts, since the adapter object outlives a session and an unserviced request would
167+
// otherwise be latched into the next one.
168+
std::atomic<bool> m_terminateRequested {false};
169+
159170
std::string m_pdbFileName {};
160171
bool m_usePDBFileName = true;
161172

‎core/adapters/dbgengttdadapter.cpp‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -299,18 +299,36 @@ bool DbgEngTTDAdapter::SupportFeature(DebugAdapterCapacity feature)
299299
}
300300

301301

302-
bool DbgEngTTDAdapter::Quit()
302+
bool DbgEngTTDAdapter::TerminateTargetOnEngineThread()
303303
{
304-
m_aboutToBeKilled = true;
305-
m_lastOperationIsStepInto = false;
306304
if (!this->m_debugClient)
307305
return false;
308306

309307
// I am not sure why TerminateProcesses() would not work. It just let the target run freely till the end of the
310308
// trace and not terminating the process at all.
311-
if (this->m_debugClient->TerminateCurrentProcess() != S_OK)
309+
return this->m_debugClient->TerminateCurrentProcess() == S_OK;
310+
}
311+
312+
313+
bool DbgEngTTDAdapter::Quit()
314+
{
315+
m_aboutToBeKilled = true;
316+
m_lastOperationIsStepInto = false;
317+
if (!this->m_debugClient)
312318
return false;
313319

320+
// Terminating from this thread crashes inside WinDbg's data model JS provider on 1.2606 (#1129):
321+
// the engine thread owns the debug client and is parked in DispatchCallbacks(), and DbgEng clients
322+
// are thread-affine. Ask that thread to do it and wake it up; ExitDispatch() is the one call that is
323+
// documented as safe to make from here.
324+
m_terminateRequested = true;
325+
326+
// If the trace is being replayed rather than sitting at a break, the engine thread is inside
327+
// WaitForEvent() where ExitDispatch() will not reach it. SetInterrupt() is safe from any thread and
328+
// brings it back to a break, where the request above is picked up.
329+
if (m_debugControl && (ExecStatus() != DEBUG_STATUS_BREAK))
330+
m_debugControl->SetInterrupt(DEBUG_INTERRUPT_ACTIVE);
331+
314332
m_debugClient->ExitDispatch(reinterpret_cast<PDEBUG_CLIENT>(m_debugClient));
315333
return true;
316334
}

‎core/adapters/dbgengttdadapter.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ namespace BinaryNinjaDebugger {
3636

3737
bool Start() override;
3838
void Reset() override;
39+
bool TerminateTargetOnEngineThread() override;
3940

4041
bool GoReverse() override;
4142
bool StepIntoReverse() override;

0 commit comments

Comments
 (0)