diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index 1dd3be70..4f80db00 100644 --- a/core/debuggercontroller.cpp +++ b/core/debuggercontroller.cpp @@ -17,6 +17,7 @@ limitations under the License. #include "debuggercontroller.h" #include #include +#include "base/assertions.h" #include "lowlevelilinstruction.h" #include "mediumlevelilinstruction.h" #include "highlevelilinstruction.h" @@ -24,6 +25,10 @@ limitations under the License. using namespace BinaryNinjaDebugger; +namespace BinaryNinjaDebugger { + thread_local DebuggerController* t_controllerOnWorker = nullptr; +} + DebuggerController::DebuggerController(BinaryViewRef data): BinaryDataNotification(Rebased) { INIT_DEBUGGER_API_OBJECT(); @@ -36,14 +41,32 @@ DebuggerController::DebuggerController(BinaryViewRef data): BinaryDataNotificati m_state = new DebuggerState(data, this); m_adapter = nullptr; m_shouldAnnotateStackVariable = Settings::Instance()->Get("debugger.stackVariableAnnotations"); - RegisterEventCallback([this](const DebuggerEvent& event) { EventHandler(event); }, "Debugger Core"); m_debuggerEventThread = std::thread([&]{ DebuggerMainThread(); }); + + m_workerShouldExit = false; + m_workerThread = std::thread([this] { WorkerThreadMain(); }); } DebuggerController::~DebuggerController() { + // The worker can be blocked on either condition variable -- m_workQueueCv (idle in + // the outer loop) or m_adapterStopCv (inside WaitForAdapterStop during an op). Each + // CV's wait predicate reads m_workerShouldExit, so the flag must be modified while + // holding the matching mutex to publish the change correctly to that waiter. We hold + // BOTH mutexes when setting the flag (scoped_lock is deadlock-safe) and then notify + // both CVs. Otherwise a waiter on the CV whose mutex we didn't hold can miss the + // wakeup and never observe the flag, hanging this destructor on join. + { + std::scoped_lock shutdownLock(m_workQueueMutex, m_adapterStopMutex); + m_workerShouldExit = true; + } + m_workQueueCv.notify_all(); + m_adapterStopCv.notify_all(); + if (m_workerThread.joinable()) + m_workerThread.join(); + m_shouldExit = true; m_cv.notify_all(); if (m_debuggerEventThread.joinable()) @@ -60,6 +83,28 @@ DebuggerController::~DebuggerController() } +void DebuggerController::WorkerThreadMain() +{ + t_controllerOnWorker = this; + while (true) + { + std::function task; + { + std::unique_lock lock(m_workQueueMutex); + m_workQueueCv.wait(lock, [this] { + return m_workerShouldExit || !m_workQueue.empty(); + }); + if (m_workerShouldExit && m_workQueue.empty()) + break; + task = std::move(m_workQueue.front()); + m_workQueue.pop(); + } + task(); + } + t_controllerOnWorker = nullptr; +} + + void DebuggerController::AddBreakpoint(uint64_t address) { m_state->AddBreakpoint(address); @@ -290,14 +335,13 @@ bool DebuggerController::Launch() if (!CanStartDebgging()) return false; - std::thread([&]() { LaunchAndWait(); }).detach(); + Submit([this] { LaunchAndWaitOnWorker(); }); return true; } DebugStopReason DebuggerController::LaunchAndWaitInternal() { - m_userRequestedBreak = false; if (Settings::Instance()->Get("debugger.safeMode")) { @@ -329,7 +373,7 @@ DebugStopReason DebuggerController::LaunchAndWaitInternal() } -DebugStopReason DebuggerController::LaunchAndWait() +DebugStopReason DebuggerController::LaunchAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -339,7 +383,7 @@ DebugStopReason DebuggerController::LaunchAndWait() return InternalError; auto reason = LaunchAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -347,13 +391,19 @@ DebugStopReason DebuggerController::LaunchAndWait() } +DebugStopReason DebuggerController::LaunchAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return LaunchAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Attach() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) return false; - std::thread([&]() { AttachAndWait(); }).detach(); + Submit([this] { AttachAndWaitOnWorker(); }); return true; } @@ -361,7 +411,6 @@ bool DebuggerController::Attach() DebugStopReason DebuggerController::AttachAndWaitInternal() { m_firstAttach = false; - m_userRequestedBreak = false; DebuggerEvent event; event.type = LaunchEventType; @@ -380,7 +429,7 @@ DebugStopReason DebuggerController::AttachAndWaitInternal() } -DebugStopReason DebuggerController::AttachAndWait() +DebugStopReason DebuggerController::AttachAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -390,7 +439,7 @@ DebugStopReason DebuggerController::AttachAndWait() return InternalError; auto reason = AttachAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -398,13 +447,19 @@ DebugStopReason DebuggerController::AttachAndWait() } +DebugStopReason DebuggerController::AttachAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return AttachAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Connect() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) return false; - std::thread([&]() { ConnectAndWait(); }).detach(); + Submit([this] { ConnectAndWaitOnWorker(); }); return true; } @@ -412,7 +467,6 @@ bool DebuggerController::Connect() DebugStopReason DebuggerController::ConnectAndWaitInternal() { m_firstConnect = false; - m_userRequestedBreak = false; DebuggerEvent event; event.type = LaunchEventType; @@ -431,7 +485,7 @@ DebugStopReason DebuggerController::ConnectAndWaitInternal() } -DebugStopReason DebuggerController::ConnectAndWait() +DebugStopReason DebuggerController::ConnectAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanStartDebgging()) @@ -441,7 +495,7 @@ DebugStopReason DebuggerController::ConnectAndWait() return InternalError; auto reason = ConnectAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -449,6 +503,12 @@ DebugStopReason DebuggerController::ConnectAndWait() } +DebugStopReason DebuggerController::ConnectAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return ConnectAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Execute() { std::unique_lock lock(m_targetControlMutex); @@ -543,7 +603,7 @@ bool DebuggerController::Go() if (!CanResumeTarget()) return false; - std::thread([&]() { GoAndWait(); }).detach(); + Submit([this] { GoAndWaitOnWorker(); }); return true; } @@ -554,13 +614,13 @@ bool DebuggerController::GoReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { GoReverseAndWait(); }).detach(); + Submit([this] { GoReverseAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::GoAndWait() +DebugStopReason DebuggerController::GoAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -570,14 +630,21 @@ DebugStopReason DebuggerController::GoAndWait() return InternalError; auto reason = GoAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); return reason; } -DebugStopReason DebuggerController::GoReverseAndWait() + +DebugStopReason DebuggerController::GoAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoAndWaitOnWorker(); }, timeout); +} + + +DebugStopReason DebuggerController::GoReverseAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -587,7 +654,7 @@ DebugStopReason DebuggerController::GoReverseAndWait() return InternalError; auto reason = GoReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -595,6 +662,12 @@ DebugStopReason DebuggerController::GoReverseAndWait() } +DebugStopReason DebuggerController::GoReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::StepIntoIL(BNFunctionGraphType il) { switch (il) @@ -807,7 +880,6 @@ DebugStopReason DebuggerController::StepIntoReverseIL(BNFunctionGraphType il) DebugStopReason DebuggerController::StepIntoReverseAndWaitInternal() { - m_userRequestedBreak = false; // TODO: check if StepInto() succeeds return ExecuteAdapterAndWait(DebugAdapterStepIntoReverse); } @@ -817,7 +889,7 @@ bool DebuggerController::StepInto(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepIntoAndWait(il); }).detach(); + Submit([this, il] { StepIntoAndWaitOnWorker(il); }); return true; } @@ -827,12 +899,12 @@ bool DebuggerController::StepIntoReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepIntoReverseAndWait(il); }).detach(); + Submit([this, il] { StepIntoReverseAndWaitOnWorker(il); }); return true; } -DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepIntoReverseAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -842,14 +914,20 @@ DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType i return InternalError; auto reason = StepIntoReverseIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); return reason; } -DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepIntoReverseAndWaitOnWorker(il); }, timeout); +} + +DebugStopReason DebuggerController::StepIntoAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -859,13 +937,19 @@ DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il) return InternalError; auto reason = StepIntoIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); return reason; } +DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepIntoAndWaitOnWorker(il); }, timeout); +} + DebugStopReason DebuggerController::StepOverIL(BNFunctionGraphType il) { switch (il) @@ -1078,7 +1162,7 @@ bool DebuggerController::StepOver(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepOverAndWait(il); }).detach(); + Submit([this, il] { StepOverAndWaitOnWorker(il); }); return true; } @@ -1089,13 +1173,13 @@ bool DebuggerController::StepOverReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - std::thread([&, il]() { StepOverReverseAndWait(il); }).detach(); + Submit([this, il] { StepOverReverseAndWaitOnWorker(il); }); return true; } -DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepOverAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1105,7 +1189,7 @@ DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) return InternalError; auto reason = StepOverIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1113,7 +1197,14 @@ DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) } -DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il) +DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepOverAndWaitOnWorker(il); }, timeout); +} + + +DebugStopReason DebuggerController::StepOverReverseAndWaitOnWorker(BNFunctionGraphType il) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1123,7 +1214,7 @@ DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType i return InternalError; auto reason = StepOverReverseIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1131,6 +1222,13 @@ DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType i } +DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this, il] { return StepOverReverseAndWaitOnWorker(il); }, timeout); +} + + DebugStopReason DebuggerController::EmulateStepReturnAndWait() { uint64_t address = m_state->IP(); @@ -1154,7 +1252,6 @@ DebugStopReason DebuggerController::EmulateStepReturnAndWait() DebugStopReason DebuggerController::StepReturnAndWaitInternal() { - m_userRequestedBreak = false; if (true /* StepReturnAvailable() */) { @@ -1170,7 +1267,6 @@ DebugStopReason DebuggerController::StepReturnAndWaitInternal() DebugStopReason DebuggerController::StepReturnReverseAndWaitInternal() { - m_userRequestedBreak = false; if (true /* StepReturnReverseAvailable() */) { @@ -1184,7 +1280,7 @@ bool DebuggerController::StepReturn() if (!CanResumeTarget()) return false; - std::thread([&]() { StepReturnAndWait(); }).detach(); + Submit([this] { StepReturnAndWaitOnWorker(); }); return true; } @@ -1195,13 +1291,13 @@ bool DebuggerController::StepReturnReverse() if (!CanResumeTarget()) return false; - std::thread([&]() { StepReturnReverseAndWait(); }).detach(); + Submit([this] { StepReturnReverseAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::StepReturnAndWait() +DebugStopReason DebuggerController::StepReturnAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1211,7 +1307,7 @@ DebugStopReason DebuggerController::StepReturnAndWait() return InternalError; auto reason = StepReturnAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1219,7 +1315,13 @@ DebugStopReason DebuggerController::StepReturnAndWait() } -DebugStopReason DebuggerController::StepReturnReverseAndWait() +DebugStopReason DebuggerController::StepReturnAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return StepReturnAndWaitOnWorker(); }, timeout); +} + + +DebugStopReason DebuggerController::StepReturnReverseAndWaitOnWorker() { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1229,7 +1331,7 @@ DebugStopReason DebuggerController::StepReturnReverseAndWait() return InternalError; auto reason = StepReturnReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1237,9 +1339,14 @@ DebugStopReason DebuggerController::StepReturnReverseAndWait() } +DebugStopReason DebuggerController::StepReturnReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return StepReturnReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vector& remoteAddresses) { - m_userRequestedBreak = false; for (uint64_t remoteAddress : remoteAddresses) { @@ -1265,7 +1372,6 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vector& remoteAddresses) { - m_userRequestedBreak = false; for (uint64_t remoteAddress : remoteAddresses) { @@ -1295,7 +1401,7 @@ bool DebuggerController::RunTo(const std::vector& remoteAddresses) if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { RunToAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToAndWaitOnWorker(remoteAddresses); }); return true; } @@ -1307,13 +1413,13 @@ bool DebuggerController::RunToReverse(const std::vector& remoteAddress if (!CanResumeTarget()) return false; - std::thread([&, remoteAddresses]() { RunToReverseAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToReverseAndWaitOnWorker(remoteAddresses); }); return true; } -DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses) +DebugStopReason DebuggerController::RunToAndWaitOnWorker(const std::vector& remoteAddresses) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1323,7 +1429,7 @@ DebugStopReason DebuggerController::RunToAndWait(const std::vector& re return InternalError; auto reason = RunToAndWaitInternal(remoteAddresses); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); m_targetControlMutex.unlock(); @@ -1331,7 +1437,15 @@ DebugStopReason DebuggerController::RunToAndWait(const std::vector& re } -DebugStopReason DebuggerController::RunToReverseAndWait(const std::vector& remoteAddresses) +DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait( + [this, remoteAddresses] { return RunToAndWaitOnWorker(remoteAddresses); }, timeout); +} + + +DebugStopReason DebuggerController::RunToReverseAndWaitOnWorker(const std::vector& remoteAddresses) { // This is an API function of the debugger. We only do these checks at the API level. if (!CanResumeTarget()) @@ -1341,7 +1455,7 @@ DebugStopReason DebuggerController::RunToReverseAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout) +{ + return SubmitAndWait( + [this, remoteAddresses] { return RunToReverseAndWaitOnWorker(remoteAddresses); }, timeout); +} + + bool DebuggerController::CreateDebuggerBinaryView() { BinaryViewRef data = GetData(); @@ -1457,18 +1579,34 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; - std::thread([&]() { RestartAndWait(); }).detach(); + // Interrupt any in-flight resume op so the queued Restart can actually run. + // Without this, if the target is running the worker is blocked in WaitForAdapterStop + // and Restart would sit in the queue indefinitely. + RequestInterrupt(); + Submit([this] { RestartAndWaitOnWorker(); }); return true; } -DebugStopReason DebuggerController::RestartAndWait() +DebugStopReason DebuggerController::RestartAndWaitOnWorker() { if (!m_state->IsConnected()) return InvalidStatusOrOperation; - QuitAndWait(); - return LaunchAndWait(); + // Bypass the public sync wrappers; we are already on the worker and want to + // run these inline without re-entering Submit. + QuitAndWaitOnWorker(); + return LaunchAndWaitOnWorker(); +} + + +DebugStopReason DebuggerController::RestartAndWait(std::chrono::milliseconds timeout) +{ + if (!m_state->IsConnected()) + return InvalidStatusOrOperation; + + RequestInterrupt(); + return SubmitAndWait([this] { return RestartAndWaitOnWorker(); }, timeout); } @@ -1517,11 +1655,13 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; - std::thread([&]() { DetachAndWait(); }).detach(); + // Interrupt any in-flight resume op (see Restart for rationale). + RequestInterrupt(); + Submit([this] { DetachAndWaitOnWorker(); }); } -void DebuggerController::DetachAndWait() +void DebuggerController::DetachAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1545,16 +1685,28 @@ void DebuggerController::DetachAndWait() } +void DebuggerController::DetachAndWait(std::chrono::milliseconds timeout) +{ + if (!m_state->IsConnected()) + return; + + RequestInterrupt(); + SubmitAndWait([this] { DetachAndWaitOnWorker(); }, timeout); +} + + void DebuggerController::Quit() { if (!m_state->IsConnected()) return; - std::thread([&]() { QuitAndWait(); }).detach(); + // Interrupt any in-flight resume op (see Restart for rationale). + RequestInterrupt(); + Submit([this] { QuitAndWaitOnWorker(); }); } -void DebuggerController::QuitAndWait() +void DebuggerController::QuitAndWaitOnWorker() { bool locked = false; if (m_targetControlMutex.try_lock()) @@ -1569,8 +1721,13 @@ void DebuggerController::QuitAndWait() if (m_state->IsRunning()) { - // We must pause the target if it is currently running, at least for DbgEngAdapter - PauseAndWait(); + // We must pause the target if it is currently running, at least for DbgEngAdapter. + // Call PauseAndWaitInternal (not the public PauseAndWait) so we go through + // ExecuteAdapterAndWait(Pause) and actually wait for the engine to stop via the + // adapter-stop channel. The public PauseAndWait would re-enter Submit inline here + // (we are on the worker) and return without waiting, leaving the engine still + // running when we issue Quit below. + PauseAndWaitInternal(); } // TODO: return whether the operation is successful @@ -1584,52 +1741,75 @@ void DebuggerController::QuitAndWait() } +void DebuggerController::QuitAndWait(std::chrono::milliseconds timeout) +{ + if (!m_state->IsConnected()) + return; + + RequestInterrupt(); + SubmitAndWait([this] { QuitAndWaitOnWorker(); }, timeout); +} + + bool DebuggerController::Pause() { if (!m_state->IsConnected()) return false; - std::thread([&]() { PauseAndWait(); }).detach(); - + // Out-of-band: signal the engine to break on the caller's thread. The worker + // is presumed to be blocked inside ExecuteAdapterAndWait for whatever op is + // in flight (Go/Step/RunTo/etc.); when the engine receives the break it will + // report a stop, the worker's op will return, and its OnWorker wrapper will + // call NotifyStopped. We do not queue any work for the worker here. + RequestInterrupt(); return true; } DebugStopReason DebuggerController::PauseAndWaitInternal() { - m_userRequestedBreak = true; return ExecuteAdapterAndWait(DebugAdapterPause); } -DebugStopReason DebuggerController::PauseAndWait() +DebugStopReason DebuggerController::PauseAndWait(std::chrono::milliseconds timeout) { if (!m_state->IsConnected()) return InvalidStatusOrOperation; - auto reason = PauseAndWaitInternal(); - if ((reason != ProcessExited) && (reason != InternalError)) - NotifyStopped(reason); - return reason; + RequestInterrupt(); + + // Wait for the currently-running worker task (if any) to finish processing + // the break. Submitting a no-op gives us a future that resolves once the + // worker drains past whatever was in flight at the time of the break. + auto fut = Submit([] {}); + if (timeout == std::chrono::milliseconds::max()) + { + fut.wait(); + } + else if (fut.wait_for(timeout) != std::future_status::ready) + { + // BreakInto has already been signaled; there's nothing else to do. + return InternalError; + } + + return DebugStopReason::UserRequestedBreak; } DebugStopReason DebuggerController::GoAndWaitInternal() { - m_userRequestedBreak = false; return ExecuteAdapterAndWait(DebugAdapterGo); } DebugStopReason DebuggerController::GoReverseAndWaitInternal() { - m_userRequestedBreak = false; return ExecuteAdapterAndWait(DebugAdapterGoReverse); } DebugStopReason DebuggerController::StepIntoAndWaitInternal() { - m_userRequestedBreak = false; // TODO: check if StepInto() succeeds return ExecuteAdapterAndWait(DebugAdapterStepInto); } @@ -1645,7 +1825,11 @@ DebugStopReason DebuggerController::EmulateStepOverAndWait() return InternalError; size_t size = remoteArch->GetMaxInstructionLength(); - DataBuffer buffer = m_adapter->ReadMemory(remoteIP, size); + DataBuffer buffer; + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + buffer = m_adapter->ReadMemory(remoteIP, size); + } size_t bytesRead = buffer.GetLength(); Ref ilFunc = new LowLevelILFunction(remoteArch, nullptr); @@ -1688,7 +1872,6 @@ DebugStopReason DebuggerController::EmulateStepOverReverseAndWait() DebugStopReason DebuggerController::StepOverAndWaitInternal() { - m_userRequestedBreak = false; if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOver)) { @@ -1703,7 +1886,6 @@ DebugStopReason DebuggerController::StepOverAndWaitInternal() DebugStopReason DebuggerController::StepOverReverseAndWaitInternal() { - m_userRequestedBreak = false; if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOverReverse)) { @@ -1842,8 +2024,26 @@ void DebuggerController::Destroy() } -// This is the central hub of event dispatch. All events first arrive here and then get dispatched based on the content -void DebuggerController::EventHandler(const DebuggerEvent& event) +// The controller's own state mutations for each event type. Called inline from +// PostDebuggerEvent (on whichever thread posted the event) BEFORE the event is +// enqueued for the dispatcher. Previously this body lived in EventHandler running +// on the dispatcher thread; that created a race where the worker could observe +// stale m_state after WaitForAdapterStop returned but before the dispatcher had +// gotten around to running EventHandler. Now the controller's state is updated +// happen-before the broadcast, and the dispatcher only fans out to external +// (UI, plugin, scripting) consumers. +// +// Thread-safety: this body touches m_state's connection/execution status, plus +// m_lastIP / m_currentIP / m_exitCode on the controller -- all plain (non-atomic, +// unlocked) fields read from other threads (UI render layer, file accessor) without +// synchronization. Those are pre-existing data races reported in #1091; this PR +// does not address them, and adding synchronization there is tracked separately. +// Moving the mutations off the dispatcher and onto the event-posting thread (this +// function vs the old EventHandler) does not change the race set -- it just changes +// which thread is the writer -- but it does fix the unrelated *control-flow* race +// where the worker resumed before the dispatcher had updated state, which is what +// this commit is for. +void DebuggerController::ApplyOwnStateForEvent(const DebuggerEvent& event) { switch (event.type) { @@ -1875,15 +2075,11 @@ void DebuggerController::EventHandler(const DebuggerEvent& event) if (m_accessor) { - // Defer deletion to a detached thread. The accessor holds a DbgRef, - // and if it is the last reference, deleting it here (on the event thread) would trigger - // ~DebuggerController which calls m_debuggerEventThread.join() -- deadlocking/crashing - // because we ARE the event thread. - // - // This can happen when Destroy() races with event processing: EventHandler sets - // ConnectionStatus to NotConnected (line above), and another thread observes this, - // calls Destroy() which removes the global array ref, making the accessor's DbgRef - // the last reference to the controller. + // Defer deletion to a detached thread. The accessor holds a DbgRef; + // if it's the last reference, deleting it here would trigger ~DebuggerController which + // calls m_workerThread.join() and m_debuggerEventThread.join(). If we happen to be on + // either of those threads (e.g. the worker just finished Quit and called us), the join + // would deadlock. A detached thread sidesteps that regardless of who called us. auto* accessor = m_accessor; m_accessor = nullptr; std::thread([accessor]() { delete accessor; }).detach(); @@ -1996,11 +2192,56 @@ bool DebuggerController::RemoveEventCallbackInternal(size_t index) void DebuggerController::PostDebuggerEvent(const DebuggerEvent& event) { - // During conditional breakpoint auto-resume, suppress the ResumeEventType that adapters - // post inside Go(). The target is already considered running by the UI, and posting this - // event from the dispatcher thread would trigger a re-entrant warning. - if (m_suppressResumeEvent && event.type == ResumeEventType) + // Adapter stops are an internal signal to the worker, not a user-facing event. + // Route them to the adapter-stop channel and skip the public dispatcher queue. + if (event.type == AdapterStoppedEventType) + { + DebugStopReason reason = event.data.targetStoppedData.reason; + bool inWait; + { + std::lock_guard lk(m_adapterStopMutex); + inWait = m_inAdapterWait; + if (inWait) + m_adapterStopPending = reason; + } + if (inWait) + { + m_adapterStopCv.notify_all(); + } + else + { + // No controller op is in flight — the adapter stopped on its own (e.g. + // the user typed `si` directly into the LLDB REPL). Queue a handler on + // the worker to update caches and synthesize a TargetStoppedEvent. + Submit([this, reason] { HandleSpontaneousAdapterStop(reason); }); + } return; + } + + // Apply the controller's own state mutations synchronously, before this event + // reaches anyone else. Previously this happened in EventHandler running on the + // dispatcher thread, which created a race: the worker could observe stale + // m_state after WaitForAdapterStop returned but before EventHandler had run. + // Doing the mutations here means the broadcast is purely informational to + // external consumers; the controller's own state is already consistent. + ApplyOwnStateForEvent(event); + + // Target-exit / detach also unblock any in-flight WaitForAdapterStop -- the + // engine isn't going to issue a separate AdapterStoppedEvent. We do this AFTER + // ApplyOwnStateForEvent so the worker wakes to a fully-updated m_state. + if (event.type == TargetExitedEventType || event.type == DetachedEventType) + { + bool inWait; + { + std::lock_guard lk(m_adapterStopMutex); + inWait = m_inAdapterWait; + if (inWait) + m_adapterStopPending = ProcessExited; + } + if (inWait) + m_adapterStopCv.notify_all(); + // Fall through: still goes through the public dispatcher queue. + } auto pending = std::make_shared(); pending->event = event; @@ -2026,6 +2267,87 @@ void DebuggerController::PostDebuggerEvent(const DebuggerEvent& event) } +DebugStopReason DebuggerController::WaitForAdapterStop() +{ + std::unique_lock lk(m_adapterStopMutex); + m_adapterStopCv.wait(lk, [this] { + return m_adapterStopPending.has_value() || m_workerShouldExit; + }); + if (m_workerShouldExit && !m_adapterStopPending.has_value()) + return InternalError; + DebugStopReason reason = *m_adapterStopPending; + m_adapterStopPending = std::nullopt; + return reason; +} + + +bool DebuggerController::ShouldSilentResumeAfterStop() +{ + // Only breakpoint stops are candidates for silent resume on a false condition. + // Step operations always surface, even if they land on a breakpoint. + bool isStepOperation = (m_lastOperation == DebugAdapterStepInto) + || (m_lastOperation == DebugAdapterStepOver) + || (m_lastOperation == DebugAdapterStepReturn) + || (m_lastOperation == DebugAdapterStepIntoReverse) + || (m_lastOperation == DebugAdapterStepOverReverse) + || (m_lastOperation == DebugAdapterStepReturnReverse); + if (isStepOperation) + return false; + + m_state->SetConnectionStatus(DebugAdapterConnectedStatus); + m_state->SetExecutionStatus(DebugAdapterPausedStatus); + m_state->MarkDirty(); + m_state->UpdateCaches(); + AddRegisterValuesToExpressionParser(); + AddModuleValuesToExpressionParser(); + + uint64_t ip = m_state->IP(); + if (!m_state->GetBreakpoints()->ContainsAbsolute(ip)) + return false; + if (EvaluateBreakpointCondition(ip)) + return false; + + return true; +} + + +void DebuggerController::HandleSpontaneousAdapterStop(DebugStopReason reason) +{ + // The adapter reported a stop with no controller op in flight. This is the + // case the dispatcher previously synthesized a TargetStoppedEvent for at + // `debuggercontroller.cpp:2279` in the pre-refactor code. + m_state->SetConnectionStatus(DebugAdapterConnectedStatus); + m_state->SetExecutionStatus(DebugAdapterPausedStatus); + m_state->MarkDirty(); + m_state->UpdateCaches(); + AddRegisterValuesToExpressionParser(); + AddModuleValuesToExpressionParser(); + NotifyStopped(reason); +} + + +void DebuggerController::RequestInterrupt() +{ + // Purely out-of-band: signal the engine to break on the caller's thread. The + // in-flight worker op (blocked in WaitForAdapterStop) wakes up via the adapter- + // stop channel; its OnWorker wrapper then calls NotifyStopped. We do not call + // NotifyStopped here and we do not queue any work for the worker. + // + // Snapshot the adapter pointer once so the null check and the call see the same + // value. The pointed-to object is kept alive by the caller's DbgRef on the + // controller (the adapter is destroyed only inside ~DebuggerState, which runs + // inside ~DebuggerController after both worker threads have joined). The adapter- + // access lock serializes BreakInto against in-flight resume requests and UI memory + // reads; it is safe to acquire even while the target is running because the worker + // holds the lock only around the resume request, not across the run-wait. + if (DebugAdapter* adapter = m_adapter) + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + adapter->BreakInto(); + } +} + + void DebuggerController::DebuggerMainThread() { m_shouldExit = false; @@ -2049,49 +2371,11 @@ void DebuggerController::DebuggerMainThread() callbackLock.unlock(); auto event = current->event; - if (event.type == AdapterStoppedEventType) - m_lastAdapterStopEventConsumed = false; - if (event.type == AdapterStoppedEventType && - event.data.targetStoppedData.reason == Breakpoint) - { - // update the caches so registers are available for condition evaluation - m_state->SetConnectionStatus(DebugAdapterConnectedStatus); - m_state->SetExecutionStatus(DebugAdapterPausedStatus); - m_state->MarkDirty(); - m_state->UpdateCaches(); - AddRegisterValuesToExpressionParser(); - AddModuleValuesToExpressionParser(); - - // skip conditional breakpoint evaluation for step operations - when the user explicitly - // steps onto a breakpoint, they expect to stop there regardless of the condition. - bool isStepOperation = (m_lastOperation == DebugAdapterStepInto) - || (m_lastOperation == DebugAdapterStepOver) - || (m_lastOperation == DebugAdapterStepReturn) - || (m_lastOperation == DebugAdapterStepIntoReverse) - || (m_lastOperation == DebugAdapterStepOverReverse) - || (m_lastOperation == DebugAdapterStepReturnReverse); - - if (uint64_t ip = m_state->IP(); - !isStepOperation && m_state->GetBreakpoints()->ContainsAbsolute(ip)) - { - if (!EvaluateBreakpointCondition(ip) && !m_userRequestedBreak) - { - m_lastAdapterStopEventConsumed = true; - current->done.set_value(); - // Using m_adapter->Go() directly instead of Go() to avoid mutex deadlock - // since we're already inside ExecuteAdapterAndWait's event processing. - // Suppress the ResumeEventType that some adapters post synchronously inside - // Go() — the UI already considers the target running, and posting from the - // dispatcher thread would be unexpected. - m_suppressResumeEvent = true; - m_adapter->Go(); - m_suppressResumeEvent = false; - m_state->SetExecutionStatus(DebugAdapterRunningStatus); - continue; - } - } - } + // AdapterStoppedEventType no longer reaches the dispatcher: PostDebuggerEvent + // intercepts it and routes the reason to the worker's adapter-stop channel. + // Conditional-breakpoint silent-resume and spontaneous-stop synthesis now live + // in ExecuteAdapterAndWait / HandleSpontaneousAdapterStop on the worker. DebuggerEvent eventToSend = event; if ((eventToSend.type == TargetStoppedEventType) && !m_initialBreakpointSeen) @@ -2110,29 +2394,6 @@ void DebuggerController::DebuggerMainThread() cb.function(eventToSend); } - // If the current event is an AdapterStoppedEvent, and it is not consumed by any callback, then the adapter - // stop is not caused by the debugger core. This can happen when the user run a "ni" command directly. - // Notify a target stop reason in this case. - if (event.type == AdapterStoppedEventType && !m_lastAdapterStopEventConsumed) - { - DebuggerEvent stopEvent = event; - stopEvent.type = TargetStoppedEventType; - if (!m_initialBreakpointSeen) - { - m_initialBreakpointSeen = true; - stopEvent.data.targetStoppedData.reason = InitialBreakpoint; - } - for (const DebuggerEventCallback& cb : eventCallbacks) - { - std::unique_lock callbackLock2(m_callbackMutex); - if (m_disabledCallbacks.find(cb.index) != m_disabledCallbacks.end()) - continue; - - callbackLock2.unlock(); - cb.function(stopEvent); - } - } - CleanUpDisabledEvent(); current->done.set_value(); } @@ -2675,54 +2936,33 @@ DebugStopReason DebuggerController::StopReason() const DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOperation operation) { - // Due to the nature of the wait, this mutex should NOT be allowed to be locked recursively. - // If this is a pause operation, do not try to lock the mutex -- it is mostly likely held by another thread - if ((operation != DebugAdapterPause) && (operation != DebugAdapterQuit) && (operation != DebugAdapterDetach)) + // Invariant: ExecuteAdapterAndWait only ever runs on m_workerThread. The worker queue + // serializes all adapter operations, so the previous m_adapterMutex / m_adapterMutex2 + // pair (which guarded against concurrent adapter access from multiple spawned threads) + // is no longer needed. The new Pause path bypasses this method entirely and calls + // m_adapter->BreakInto() out-of-band; everything else funnels through here on the worker. + BN_RELEASE_ASSERT(t_controllerOnWorker == this); + + // Claim the adapter-stop channel for the duration of this call. Any AdapterStoppedEvent + // posted by the adapter from now until we clear m_inAdapterWait is delivered to + // WaitForAdapterStop below, not treated as spontaneous. We hold this across the + // entire silent-resume loop so that an adapter stop between iterations (after we + // kick off m_adapter->Go() for a false breakpoint condition) is still consumed + // by us, not synthesized as a spontaneous stop. { - if (!m_adapterMutex.try_lock()) - { - LogWarn("Cannot obtain mutex1 for debug adapter, operation: %d", operation); - return InternalError; - } - } - else - { - if (!m_adapterMutex2.try_lock()) - { - LogWarn("Cannot obtain mutex2 for debug adapter, operation: %d", operation); - return InternalError; - } + std::lock_guard lk(m_adapterStopMutex); + m_inAdapterWait = true; + m_adapterStopPending = std::nullopt; } - Semaphore sem; - DebugStopReason reason = UnknownReason; - size_t callback = RegisterEventCallback( - [&](const DebuggerEvent& event) { - switch (event.type) - { - case AdapterStoppedEventType: - reason = event.data.targetStoppedData.reason; - sem.Release(); - break; - // It is a little awkward to add two cases for these events, but we must take them into account, - // since after we resume the target, the target can either or exit. - case TargetExitedEventType: - case DetachedEventType: - // There is no DebugStopReason for "detach", so we use ProcessExited for now - reason = ProcessExited; - sem.Release(); - break; - default: - break; - } - m_lastAdapterStopEventConsumed = true; - }, - "WaitForAdapterStop"); - m_lastOperation = operation; bool resumeOK = false; bool operationRequested = false; + // Hold the adapter-access lock only around the resume REQUEST (these calls return + // promptly; the actual wait for the stop happens below, lock-free, so Pause/BreakInto + // can acquire the same lock while the target runs). + std::unique_lock adapterLock(m_state->AdapterAccessMutex()); switch (operation) { case DebugAdapterGo: @@ -2770,6 +3010,8 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper default: break; } + // Resume request issued; drop the lock so the run-wait below is lock-free. + adapterLock.unlock(); bool ok = false; if ((operation == DebugAdapterGo) || (operation == DebugAdapterStepInto) || (operation == DebugAdapterStepOver) @@ -2789,16 +3031,45 @@ DebugStopReason DebuggerController::ExecuteAdapterAndWait(const DebugAdapterOper ok = true; } - if (ok) - sem.Wait(); - else + DebugStopReason reason = UnknownReason; + if (!ok) + { reason = InternalError; - - RemoveEventCallback(callback); - if ((operation != DebugAdapterPause) && (operation != DebugAdapterQuit) && (operation != DebugAdapterDetach)) - m_adapterMutex.unlock(); + } else - m_adapterMutex2.unlock(); + { + // Loop: wait for the adapter to stop. If the stop is a breakpoint whose + // condition evaluates to false (and the user didn't explicitly step or + // request a break), silently resume and wait again. Otherwise return. + while (true) + { + reason = WaitForAdapterStop(); + if (reason == ProcessExited || reason == InternalError) + break; + if (reason == Breakpoint && ShouldSilentResumeAfterStop()) + { + m_state->SetExecutionStatus(DebugAdapterRunningStatus); + bool resumed; + { + std::lock_guard resumeLock(m_state->AdapterAccessMutex()); + resumed = m_adapter && m_adapter->Go(); + } + if (!resumed) + { + reason = InternalError; + break; + } + continue; + } + break; + } + } + + { + std::lock_guard lk(m_adapterStopMutex); + m_inAdapterWait = false; + m_adapterStopPending = std::nullopt; + } return reason; } diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index f298d902..ea5c286a 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -16,12 +16,14 @@ limitations under the License. #pragma once #include "binaryninjaapi.h" +#include "base/assertions.h" #include "debuggerstate.h" #include "debuggerevent.h" #include #include #include #include +#include #include #include "ffi_global.h" #include "refcountobject.h" @@ -30,6 +32,13 @@ limitations under the License. DECLARE_DEBUGGER_API_OBJECT(BNDebuggerController, DebuggerController); namespace BinaryNinjaDebugger { + class DebuggerController; + + // Set to the controller pointer when running on that controller's worker thread, + // nullptr otherwise. Used by DebuggerController::Submit to detect re-entrant calls + // without needing to synchronize a thread::id field across threads. + extern thread_local DebuggerController* t_controllerOnWorker; + struct DebuggerEventCallback { std::function function; @@ -76,6 +85,17 @@ namespace BinaryNinjaDebugger { }; private: + // m_adapter is the active debug adapter for this controller. Written exclusively + // from the worker thread (in CreateDebugAdapter); read both from the worker + // (the bulk of references inside ExecuteAdapterAndWait and adapter ops) and + // from arbitrary caller threads (RequestInterrupt's out-of-band BreakInto). + // + // The pointed-to adapter object's lifetime is guaranteed externally: it is + // destroyed only inside ~DebuggerState, which runs inside ~DebuggerController + // after both worker threads have joined. Cross-thread callers hold a DbgRef + // on the controller during their call, so the adapter cannot be destroyed + // out from under them. See the "Refcount the DebugAdapter" follow-up issue + // for the structural fix that would make this guarantee enforced by the type. DebugAdapter* m_adapter; DebuggerState* m_state; FileMetadataRef m_file; @@ -98,17 +118,11 @@ namespace BinaryNinjaDebugger { std::mutex m_callbackMutex; std::set m_disabledCallbacks; - // m_adapterMutex is a low-level mutex that protects the adapter access. It cannot be locked recursively. // m_targetControlMutex is a high-level mutex that prevents two threads from controlling the debugger at the - // same time - std::mutex m_adapterMutex; + // same time. With the worker queue serializing adapter operations on a single thread, this is largely + // redundant; kept in place for now to minimize behavioral churn. std::recursive_mutex m_targetControlMutex; - // m_adapterMutex2 is similar to m_adapterMutex, but it is used to protect only the Pause/Quit/Detach operation - // These operations cannot be protected by m_adapterMutex, since if the user resume the target and it remains - // running, we need the ability to pause or kill the target - std::mutex m_adapterMutex2; - uint64_t m_lastIP = 0; uint64_t m_currentIP = 0; @@ -116,14 +130,29 @@ namespace BinaryNinjaDebugger { // status before returning the value uint32_t m_exitCode = 0; - bool m_userRequestedBreak = false; DebugAdapterOperation m_lastOperation = DebugAdapterGo; - bool m_lastAdapterStopEventConsumed = true; - - // When true, ResumeEventType events are suppressed in PostDebuggerEvent. - // Used during conditional breakpoint auto-resume to avoid posting events from the dispatcher thread. - bool m_suppressResumeEvent = false; + // Adapter-stop channel: internal signal from the adapter thread to the worker. + // AdapterStoppedEventType posted via PostDebuggerEvent is intercepted and routed + // here rather than dispatched through the public event queue. WaitForAdapterStop + // blocks on m_adapterStopCv until either an adapter stop arrives or shutdown is + // requested. m_inAdapterWait is true for the entire duration of an in-flight + // ExecuteAdapterAndWait call (including the silent-resume loop between iterations + // for conditional breakpoints) so that any stop during that window is consumed + // by WaitForAdapterStop and not treated as spontaneous. + std::mutex m_adapterStopMutex; + std::condition_variable m_adapterStopCv; + std::optional m_adapterStopPending; + bool m_inAdapterWait = false; + DebugStopReason WaitForAdapterStop(); + void HandleSpontaneousAdapterStop(DebugStopReason reason); + bool ShouldSilentResumeAfterStop(); + + // Out-of-band: signal the engine to break and mark the break as user-requested. + // Called from Pause/Restart/Quit/Detach on the caller's thread before queueing + // the actual operation, so the worker's in-flight resume op (if any) gets + // interrupted and the queued task can proceed. + void RequestInterrupt(); bool m_inputFileLoaded = false; bool m_initialBreakpointSeen = false; @@ -135,7 +164,11 @@ namespace BinaryNinjaDebugger { bool m_shouldAnnotateStackVariable = false; - void EventHandler(const DebuggerEvent& event); + // Apply the controller's own state mutations for each event type. Called inline + // from PostDebuggerEvent before the event is enqueued for the dispatcher, so + // m_state is consistent before any external consumer (or the worker waking from + // WaitForAdapterStop) observes the change. + void ApplyOwnStateForEvent(const DebuggerEvent& event); void UpdateStackVariables(); void AddRegisterValuesToExpressionParser(); void AddModuleValuesToExpressionParser(); @@ -168,6 +201,30 @@ namespace BinaryNinjaDebugger { DebugStopReason RunToAndWaitInternal(const std::vector &remoteAddresses); DebugStopReason RunToReverseAndWaitInternal(const std::vector &remoteAddresses); + // Worker-thread bodies. Each runs on m_workerThread (via Submit) and performs the + // existing lock-Internal-notify wrapper. The public `XxxAndWait(timeout)` methods + // below submit one of these and wait on the resulting future. + DebugStopReason LaunchAndWaitOnWorker(); + DebugStopReason AttachAndWaitOnWorker(); + DebugStopReason ConnectAndWaitOnWorker(); + DebugStopReason GoAndWaitOnWorker(); + DebugStopReason GoReverseAndWaitOnWorker(); + DebugStopReason StepIntoAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepIntoReverseAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepOverAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepOverReverseAndWaitOnWorker(BNFunctionGraphType il); + DebugStopReason StepReturnAndWaitOnWorker(); + DebugStopReason StepReturnReverseAndWaitOnWorker(); + DebugStopReason RunToAndWaitOnWorker(const std::vector& remoteAddresses); + DebugStopReason RunToReverseAndWaitOnWorker(const std::vector& remoteAddresses); + DebugStopReason RestartAndWaitOnWorker(); + void DetachAndWaitOnWorker(); + void QuitAndWaitOnWorker(); + // Pause has no *OnWorker variant: it's out-of-band by design. See Pause() / + // PauseAndWait() -- they call RequestInterrupt directly on the caller's thread + // rather than queueing work, because the worker is blocked inside an in-flight + // resume op when Pause is needed. + // Whether we can start debugging, e.g., launch/attach/connec to a target bool CanStartDebgging(); // Whether we can resume the execution of the target, including stepping. @@ -201,6 +258,74 @@ namespace BinaryNinjaDebugger { std::thread m_debuggerEventThread; void DebuggerMainThread(); + // Worker queue: serializes all controller operations on a single thread. + // Replaces the per-op `std::thread(...).detach()` pattern. Tasks submitted from any + // thread run in order on m_workerThread; lifetime is owned and joined in the destructor. + // If Submit is called from the worker thread itself, the task runs inline to avoid + // deadlock when an operation needs to invoke another (e.g. Restart calls Quit + Launch). + std::thread m_workerThread; + std::mutex m_workQueueMutex; + std::condition_variable m_workQueueCv; + std::queue> m_workQueue; + std::atomic_bool m_workerShouldExit; + void WorkerThreadMain(); + + // Submit work onto the controller's worker thread. Strictly for external callers + // (UI / FFI / plugins / adapter event threads). Worker code chains compound + // operations as direct calls to the *OnWorker / *Internal helpers -- it does NOT + // re-enter the queue. The assert below catches accidental misuse, which the queue + // model would silently mishandle (deferred execution in unexpected order, or in + // SubmitAndWait's case a self-deadlock). + template + auto Submit(F&& f) -> std::future> + { + BN_RELEASE_ASSERT(t_controllerOnWorker != this); + using R = std::invoke_result_t; + auto task = std::make_shared>(std::forward(f)); + auto future = task->get_future(); + + { + std::lock_guard lock(m_workQueueMutex); + if (m_workerShouldExit) + return future; // future is left unset; caller's get() will throw broken_promise + m_workQueue.push([task]() { (*task)(); }); + } + m_workQueueCv.notify_one(); + return future; + } + + // Submit a worker task and block the caller until the task completes (or the timeout + // elapses, in which case the engine is signaled to break and we still wait for the + // in-flight op to settle before returning). A timeout of milliseconds::max() means + // "wait forever" and bypasses wait_for entirely (avoids overflow inside the stdlib). + // + // This is the synchronous public-API path: it must NOT be called from the worker + // thread itself. Worker-side code that needs to chain operations should call the + // matching *OnWorker / *Internal helper directly (e.g. RestartAndWaitOnWorker calls + // QuitAndWaitOnWorker / LaunchAndWaitOnWorker, QuitAndWaitOnWorker uses + // PauseAndWaitInternal). Calling SubmitAndWait from the worker would silently + // reduce to an inline execution via Submit's re-entry path; the assert below + // catches that mistake rather than letting it hide. + template + auto SubmitAndWait(F&& f, std::chrono::milliseconds timeout) + -> std::invoke_result_t + { + BN_RELEASE_ASSERT(t_controllerOnWorker != this); + auto fut = Submit(std::forward(f)); + if (timeout != std::chrono::milliseconds::max()) + { + if (fut.wait_for(timeout) != std::future_status::ready) + { + // Snapshot the pointer once so the null check and the call see the + // same value. Lifetime of the pointee is guaranteed by the caller's + // DbgRef on the controller. + if (DebugAdapter* adapter = m_adapter) + adapter->BreakInto(); + } + } + return fut.get(); + } + std::unique_ptr m_uiCallbacks; uint64_t m_oldViewBase, m_newViewBase; @@ -351,23 +476,44 @@ namespace BinaryNinjaDebugger { DebugStopReason ExecuteAdapterAndWait(const DebugAdapterOperation operation); // Synchronous APIs - DebugStopReason LaunchAndWait(); - DebugStopReason GoAndWait(); - DebugStopReason GoReverseAndWait(); - DebugStopReason AttachAndWait(); - DebugStopReason RestartAndWait(); - DebugStopReason ConnectAndWait(); - DebugStopReason StepIntoAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepOverAndWait(BNFunctionGraphType il = NormalFunctionGraph); - DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il); - DebugStopReason StepReturnAndWait(); - DebugStopReason StepReturnReverseAndWait(); - DebugStopReason RunToAndWait(const std::vector& remoteAddresses); - DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses); - DebugStopReason PauseAndWait(); - void DetachAndWait(); - void QuitAndWait(); + // Synchronous APIs. They submit the operation to the worker thread and block the + // caller until it completes (or the optional timeout elapses, in which case the + // engine is signaled to break and the call returns once the in-flight op settles). + // Default timeout is "wait forever" so existing callers do not need to change. + DebugStopReason LaunchAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason GoAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason GoReverseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason AttachAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RestartAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason ConnectAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepIntoAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepOverAndWait(BNFunctionGraphType il = NormalFunctionGraph, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepReturnAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason StepReturnReverseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RunToAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses, + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + DebugStopReason PauseAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + void DetachAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); + void QuitAndWait( + std::chrono::milliseconds timeout = std::chrono::milliseconds::max()); // getters DebugAdapter* GetAdapter() { return m_adapter; } diff --git a/core/debuggerstate.cpp b/core/debuggerstate.cpp index 77619fa5..ba958894 100644 --- a/core/debuggerstate.cpp +++ b/core/debuggerstate.cpp @@ -55,7 +55,10 @@ void DebuggerRegisters::Update() return; std::unique_lock lock(m_registersMutex); - m_registerCache = adapter->ReadAllRegisters(); + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + m_registerCache = adapter->ReadAllRegisters(); + } m_dirty = false; } @@ -84,7 +87,11 @@ bool DebuggerRegisters::SetRegisterValue(const std::string& name, intx::uint512 if (iter == cachedRegs.end()) return false; - bool ok = adapter->WriteRegister(name, value); + bool ok; + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + ok = adapter->WriteRegister(name, value); + } if (!ok) return false; @@ -233,12 +240,27 @@ void DebuggerThreads::Update() return; std::unique_lock lock(m_threadsMutex); - m_frames.clear(); - std::vector newThreads = adapter->GetThreadList(); + // Read raw thread/frame data from the adapter under the adapter-access lock, then + // RELEASE it before symbolizing. SymbolizeFrames calls into BN core + // (GetAnalysisFunctionsContainingAddress), which takes BN's analysis lock -- and BN + // core, under that lock, calls back into the debugger's ReadMemory (which wants the + // adapter lock) from its file-accessor read callback. Holding the adapter lock across + // SymbolizeFrames inverts that order and deadlocks. Never hold the adapter lock across + // a call into BN core. + std::vector newThreads; + std::map> newFrames; + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + newThreads = adapter->GetThreadList(); + for (auto& thread : newThreads) + newFrames[thread.m_tid] = adapter->GetFramesOfThread(thread.m_tid); + } + + m_frames.clear(); for (auto thread = newThreads.begin(); thread != newThreads.end(); thread++) { - auto frames = adapter->GetFramesOfThread(thread->m_tid); + auto& frames = newFrames[thread->m_tid]; SymbolizeFrames(frames); m_frames[thread->m_tid] = frames; @@ -407,7 +429,10 @@ void DebuggerModules::Update() return; std::unique_lock lock(m_modulesMutex); - m_modules = adapter->GetModuleList(); + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + m_modules = adapter->GetModuleList(); + } m_dirty = false; } @@ -1260,7 +1285,11 @@ DataBuffer DebuggerMemory::ReadBlock(uint64_t block) if (!m_state->IsRunning()) { // The cache is old and the target is stopped, try to update the cache value - DataBuffer buffer = m_state->GetAdapter()->ReadMemory(block, 0x100); + DataBuffer buffer; + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + buffer = m_state->GetAdapter()->ReadMemory(block, 0x100); + } BN_RELEASE_ASSERT(buffer.GetLength() <= 0x100); if (buffer.GetLength() > 0) { @@ -1343,8 +1372,11 @@ bool DebuggerMemory::WriteMemory(std::uintptr_t address, const DataBuffer& buffe if (!adapter) return false; - if (!adapter->WriteMemory(address, buffer)) - return false; + { + std::lock_guard adapterLock(m_state->AdapterAccessMutex()); + if (!adapter->WriteMemory(address, buffer)) + return false; + } // TODO: Assume any memory change invalidates memory cache (suboptimal, may not be necessary) MarkDirty(); diff --git a/core/debuggerstate.h b/core/debuggerstate.h index c36fa7df..57e58004 100644 --- a/core/debuggerstate.h +++ b/core/debuggerstate.h @@ -242,6 +242,13 @@ namespace BinaryNinjaDebugger { bool m_connectedToDebugServer = false; + // Serializes every live call into the adapter, regardless of which thread makes it + // (worker control ops, worker cache refresh, UI memory/register reads, UI writes). + // Held ONLY around an individual adapter call -- never across the run-wait -- so + // Pause/BreakInto can still acquire it while the target is running. Recursive to + // tolerate any synchronous re-entrant adapter call during a callback. + std::recursive_mutex m_adapterAccessMutex; + std::string GetBestAdapter(BinaryViewRef data); public: @@ -249,6 +256,7 @@ namespace BinaryNinjaDebugger { ~DebuggerState(); DebugAdapter* GetAdapter() const { return m_adapter; } + std::recursive_mutex& AdapterAccessMutex() { return m_adapterAccessMutex; } DebuggerController* GetController() const { return m_controller; } DebuggerModules* GetModules() const { return m_modules; } diff --git a/test/debugger_test.py b/test/debugger_test.py index 530bd198..42795f39 100644 --- a/test/debugger_test.py +++ b/test/debugger_test.py @@ -45,12 +45,10 @@ def is_wow64(fpath): def sleep_and_go(dbg): - time.sleep(0.1) return dbg.go_and_wait() def sleep_and_step_into(dbg): - time.sleep(0.1) return dbg.step_into_and_wait() @@ -386,17 +384,14 @@ def test_restart(self): dbg = self.create_debugger(bv) self.assertNotIn(dbg.launch_and_wait(), [DebugStopReason.ProcessExited, DebugStopReason.InternalError]) - time.sleep(0.1) dbg.go() time.sleep(1) dbg.pause_and_wait() self.assertGreater(len(dbg.threads), 1) - time.sleep(0.1) ret = dbg.restart_and_wait() self.assertNotIn(ret, [DebugStopReason.ProcessExited, DebugStopReason.InternalError]) - time.sleep(0.1) dbg.go() time.sleep(1) ret = dbg.restart_and_wait()