From 4ff2922909451420c7e711269eaf12b875eaa828 Mon Sep 17 00:00:00 2001 From: Xusheng Date: Wed, 20 May 2026 15:02:44 -0400 Subject: [PATCH] Refactor debugger execution and event model --- api/debuggerapi.h | 20 + api/debuggercontroller.cpp | 122 ++++ api/ffi.h | 40 +- api/python/debuggercontroller.py | 131 +++- core/adapters/windowsnativeadapter.cpp | 76 ++- core/debuggercontroller.cpp | 898 ++++++++++++++++--------- core/debuggercontroller.h | 248 ++++++- core/debuggerstate.cpp | 50 +- core/debuggerstate.h | 8 + core/ffi.cpp | 132 ++++ test/debugger_test.py | 16 +- ui/controlswidget.cpp | 11 +- 12 files changed, 1351 insertions(+), 401 deletions(-) diff --git a/api/debuggerapi.h b/api/debuggerapi.h index 6fbaf67b..7ec2aecf 100644 --- a/api/debuggerapi.h +++ b/api/debuggerapi.h @@ -693,20 +693,26 @@ namespace BinaryNinjaDebuggerAPI { // target control bool Launch(); BNDebugStopReason LaunchAndWait(); + BNDebugStopReason LaunchAndWait(uint64_t timeoutMs); bool Execute(); void Restart(); void Quit(); void QuitAndWait(); + void QuitAndWait(uint64_t timeoutMs); bool Connect(); DebugStopReason ConnectAndWait(); + DebugStopReason ConnectAndWait(uint64_t timeoutMs); bool ConnectToDebugServer(); bool DisconnectDebugServer(); void Detach(); + void DetachAndWait(); + void DetachAndWait(uint64_t timeoutMs); // Convenience function, either launch the target process or connect to a remote, depending on the selected // adapter void LaunchOrConnect(); bool Attach(); DebugStopReason AttachAndWait(); + DebugStopReason AttachAndWait(uint64_t timeoutMs); bool Go(); bool GoReverse(); @@ -724,19 +730,33 @@ namespace BinaryNinjaDebuggerAPI { void Pause(); DebugStopReason GoAndWait(); + DebugStopReason GoAndWait(uint64_t timeoutMs); DebugStopReason GoReverseAndWait(); + DebugStopReason GoReverseAndWait(uint64_t timeoutMs); DebugStopReason StepIntoAndWait(BNFunctionGraphType il = NormalFunctionGraph); + DebugStopReason StepIntoAndWait(BNFunctionGraphType il, uint64_t timeoutMs); DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il = NormalFunctionGraph); + DebugStopReason StepIntoReverseAndWait(BNFunctionGraphType il, uint64_t timeoutMs); DebugStopReason StepOverAndWait(BNFunctionGraphType il = NormalFunctionGraph); + DebugStopReason StepOverAndWait(BNFunctionGraphType il, uint64_t timeoutMs); DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il); + DebugStopReason StepOverReverseAndWait(BNFunctionGraphType il, uint64_t timeoutMs); DebugStopReason StepReturnAndWait(); + DebugStopReason StepReturnAndWait(uint64_t timeoutMs); DebugStopReason StepReturnReverseAndWait(); + DebugStopReason StepReturnReverseAndWait(uint64_t timeoutMs); DebugStopReason RunToAndWait(uint64_t remoteAddresses); + DebugStopReason RunToAndWait(uint64_t remoteAddresses, uint64_t timeoutMs); DebugStopReason RunToAndWait(const std::vector& remoteAddresses); + DebugStopReason RunToAndWait(const std::vector& remoteAddresses, uint64_t timeoutMs); DebugStopReason RunToReverseAndWait(uint64_t remoteAddresses); + DebugStopReason RunToReverseAndWait(uint64_t remoteAddresses, uint64_t timeoutMs); DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses); + DebugStopReason RunToReverseAndWait(const std::vector& remoteAddresses, uint64_t timeoutMs); DebugStopReason PauseAndWait(); + DebugStopReason PauseAndWait(uint64_t timeoutMs); DebugStopReason RestartAndWait(); + DebugStopReason RestartAndWait(uint64_t timeoutMs); std::string GetAdapterType(); void SetAdapterType(const std::string& adapter); diff --git a/api/debuggercontroller.cpp b/api/debuggercontroller.cpp index e5e9eb8c..dec0bf76 100644 --- a/api/debuggercontroller.cpp +++ b/api/debuggercontroller.cpp @@ -331,12 +331,24 @@ DebugStopReason DebuggerController::GoAndWait() } +DebugStopReason DebuggerController::GoAndWait(uint64_t timeoutMs) +{ + return BNDebuggerGoAndWaitWithTimeout(m_object, timeoutMs); +} + + DebugStopReason DebuggerController::GoReverseAndWait() { return BNDebuggerGoReverseAndWait(m_object); } +DebugStopReason DebuggerController::GoReverseAndWait(uint64_t timeoutMs) +{ + return BNDebuggerGoReverseAndWaitWithTimeout(m_object, timeoutMs); +} + + bool DebuggerController::Launch() { return BNDebuggerLaunch(m_object); @@ -349,6 +361,12 @@ DebugStopReason DebuggerController::LaunchAndWait() } +DebugStopReason DebuggerController::LaunchAndWait(uint64_t timeoutMs) +{ + return BNDebuggerLaunchAndWaitWithTimeout(m_object, timeoutMs); +} + + bool DebuggerController::Execute() { return BNDebuggerExecute(m_object); @@ -373,6 +391,12 @@ void DebuggerController::QuitAndWait() } +void DebuggerController::QuitAndWait(uint64_t timeoutMs) +{ + BNDebuggerQuitAndWaitWithTimeout(m_object, timeoutMs); +} + + bool DebuggerController::Connect() { return BNDebuggerConnect(m_object); @@ -385,6 +409,12 @@ DebugStopReason DebuggerController::ConnectAndWait() } +DebugStopReason DebuggerController::ConnectAndWait(uint64_t timeoutMs) +{ + return BNDebuggerConnectAndWaitWithTimeout(m_object, timeoutMs); +} + + bool DebuggerController::ConnectToDebugServer() { return BNDebuggerConnectToDebugServer(m_object); @@ -403,6 +433,18 @@ void DebuggerController::Detach() } +void DebuggerController::DetachAndWait() +{ + BNDebuggerDetachAndWait(m_object); +} + + +void DebuggerController::DetachAndWait(uint64_t timeoutMs) +{ + BNDebuggerDetachAndWaitWithTimeout(m_object, timeoutMs); +} + + void DebuggerController::Pause() { BNDebuggerPause(m_object); @@ -428,6 +470,12 @@ DebugStopReason DebuggerController::AttachAndWait() } +DebugStopReason DebuggerController::AttachAndWait(uint64_t timeoutMs) +{ + return BNDebuggerAttachAndWaitWithTimeout(m_object, timeoutMs); +} + + bool DebuggerController::StepInto(BNFunctionGraphType il) { @@ -495,71 +543,145 @@ DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il) } +DebugStopReason DebuggerController::StepIntoAndWait(BNFunctionGraphType il, uint64_t timeoutMs) +{ + return BNDebuggerStepIntoAndWaitWithTimeout(m_object, il, timeoutMs); +} + + DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il) { return BNDebuggerStepIntoReverseAndWait(m_object, il); } +DebugStopReason DebuggerController::StepIntoReverseAndWait(BNFunctionGraphType il, uint64_t timeoutMs) +{ + return BNDebuggerStepIntoReverseAndWaitWithTimeout(m_object, il, timeoutMs); +} + + DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il) { return BNDebuggerStepOverAndWait(m_object, il); } +DebugStopReason DebuggerController::StepOverAndWait(BNFunctionGraphType il, uint64_t timeoutMs) +{ + return BNDebuggerStepOverAndWaitWithTimeout(m_object, il, timeoutMs); +} + + DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il) { return BNDebuggerStepOverReverseAndWait(m_object, il); } +DebugStopReason DebuggerController::StepOverReverseAndWait(BNFunctionGraphType il, uint64_t timeoutMs) +{ + return BNDebuggerStepOverReverseAndWaitWithTimeout(m_object, il, timeoutMs); +} + + DebugStopReason DebuggerController::StepReturnAndWait() { return BNDebuggerStepReturnAndWait(m_object); } + +DebugStopReason DebuggerController::StepReturnAndWait(uint64_t timeoutMs) +{ + return BNDebuggerStepReturnAndWaitWithTimeout(m_object, timeoutMs); +} + DebugStopReason DebuggerController::StepReturnReverseAndWait() { return BNDebuggerStepReturnReverseAndWait(m_object); } +DebugStopReason DebuggerController::StepReturnReverseAndWait(uint64_t timeoutMs) +{ + return BNDebuggerStepReturnReverseAndWaitWithTimeout(m_object, timeoutMs); +} + + DebugStopReason DebuggerController::RunToAndWait(uint64_t remoteAddresses) { return RunToAndWait(std::vector {remoteAddresses}); } +DebugStopReason DebuggerController::RunToAndWait(uint64_t remoteAddresses, uint64_t timeoutMs) +{ + return RunToAndWait(std::vector {remoteAddresses}, timeoutMs); +} + + DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses) { return BNDebuggerRunToAndWait(m_object, remoteAddresses.data(), remoteAddresses.size()); } +DebugStopReason DebuggerController::RunToAndWait(const std::vector& remoteAddresses, uint64_t timeoutMs) +{ + return BNDebuggerRunToAndWaitWithTimeout(m_object, remoteAddresses.data(), remoteAddresses.size(), timeoutMs); +} + + DebugStopReason DebuggerController::RunToReverseAndWait(uint64_t remoteAddresses) { return RunToReverseAndWait(std::vector {remoteAddresses}); } +DebugStopReason DebuggerController::RunToReverseAndWait(uint64_t remoteAddresses, uint64_t timeoutMs) +{ + return RunToReverseAndWait(std::vector {remoteAddresses}, timeoutMs); +} + + DebugStopReason DebuggerController::RunToReverseAndWait(const std::vector& remoteAddresses) { return BNDebuggerRunToReverseAndWait(m_object, remoteAddresses.data(), remoteAddresses.size()); } +DebugStopReason DebuggerController::RunToReverseAndWait( + const std::vector& remoteAddresses, uint64_t timeoutMs) +{ + return BNDebuggerRunToReverseAndWaitWithTimeout( + m_object, remoteAddresses.data(), remoteAddresses.size(), timeoutMs); +} + + DebugStopReason DebuggerController::PauseAndWait() { return BNDebuggerPauseAndWait(m_object); } +DebugStopReason DebuggerController::PauseAndWait(uint64_t timeoutMs) +{ + return BNDebuggerPauseAndWaitWithTimeout(m_object, timeoutMs); +} + + DebugStopReason DebuggerController::RestartAndWait() { return BNDebuggerRestartAndWait(m_object); } +DebugStopReason DebuggerController::RestartAndWait(uint64_t timeoutMs) +{ + return BNDebuggerRestartAndWaitWithTimeout(m_object, timeoutMs); +} + + std::string DebuggerController::GetAdapterType() { char* adapter = BNDebuggerGetAdapterType(m_object); diff --git a/api/ffi.h b/api/ffi.h index 783d98eb..edd6e0d3 100644 --- a/api/ffi.h +++ b/api/ffi.h @@ -211,7 +211,8 @@ extern "C" UserRequestedBreak, - OperationNotSupported + OperationNotSupported, + TimedOut } BNDebugStopReason; @@ -422,7 +423,7 @@ extern "C" { BNDebuggerTTDEventType type; // Type of event BNDebuggerTTDPosition position; // Position where event occurred - + // Optional child objects - existence depends on event type BNDebuggerTTDModule* module; // For ModuleLoaded/ModuleUnloaded events (NULL if not present) BNDebuggerTTDThread* thread; // For ThreadCreated/ThreadTerminated events (NULL if not present) @@ -528,19 +529,28 @@ extern "C" // target control DEBUGGER_FFI_API bool BNDebuggerLaunch(BNDebuggerController* controller); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerLaunchAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerLaunchAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API bool BNDebuggerExecute(BNDebuggerController* controller); DEBUGGER_FFI_API void BNDebuggerRestart(BNDebuggerController* controller); DEBUGGER_FFI_API void BNDebuggerQuit(BNDebuggerController* controller); DEBUGGER_FFI_API void BNDebuggerQuitAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API void BNDebuggerQuitAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API bool BNDebuggerConnect(BNDebuggerController* controller); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerConnectAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerConnectAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API bool BNDebuggerConnectToDebugServer(BNDebuggerController* controller); DEBUGGER_FFI_API bool BNDebuggerDisconnectDebugServer(BNDebuggerController* controller); DEBUGGER_FFI_API void BNDebuggerDetach(BNDebuggerController* controller); + DEBUGGER_FFI_API void BNDebuggerDetachAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API void BNDebuggerDetachAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs); // Convenience function, either launch the target process or connect to a remote, depending on the selected adapter DEBUGGER_FFI_API void BNDebuggerLaunchOrConnect(BNDebuggerController* controller); DEBUGGER_FFI_API bool BNDebuggerAttach(BNDebuggerController* controller); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerAttachAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerAttachAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API bool BNDebuggerGo(BNDebuggerController* controller); DEBUGGER_FFI_API bool BNDebuggerGoReverse(BNDebuggerController* controller); @@ -558,24 +568,48 @@ extern "C" DEBUGGER_FFI_API void BNDebuggerPause(BNDebuggerController* controller); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerGoAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerGoAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerGoReverseAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerGoReverseAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepIntoAndWait( BNDebuggerController* controller, BNFunctionGraphType il); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepIntoAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepIntoReverseAndWait( BNDebuggerController* controller, BNFunctionGraphType il); - + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepIntoReverseAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepOverAndWait( BNDebuggerController* controller, BNFunctionGraphType il); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepOverAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepOverReverseAndWait( BNDebuggerController* controller, BNFunctionGraphType il); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepOverReverseAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepReturnAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepReturnAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepReturnReverseAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerStepReturnReverseAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRunToAndWait( BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRunToAndWaitWithTimeout( + BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRunToReverseAndWait( BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRunToReverseAndWaitWithTimeout( + BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerPauseAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerPauseAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRestartAndWait(BNDebuggerController* controller); + DEBUGGER_FFI_API BNDebugStopReason BNDebuggerRestartAndWaitWithTimeout( + BNDebuggerController* controller, uint64_t timeoutMs); DEBUGGER_FFI_API char* BNDebuggerGetAdapterType(BNDebuggerController* controller); DEBUGGER_FFI_API void BNDebuggerSetAdapterType(BNDebuggerController* controller, const char* adapter); diff --git a/api/python/debuggercontroller.py b/api/python/debuggercontroller.py index b3a4f511..000f3c5a 100644 --- a/api/python/debuggercontroller.py +++ b/api/python/debuggercontroller.py @@ -1528,10 +1528,14 @@ def launch(self) -> bool: """ return dbgcore.BNDebuggerLaunch(self.handle) - def launch_and_wait(self) -> DebugStopReason: + def launch_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Launch the target and wait for all debugger events to be processed + + :param timeout: optional timeout in milliseconds """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerLaunchAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerLaunchAndWait(self.handle)) def restart(self) -> None: @@ -1546,10 +1550,15 @@ def quit(self) -> None: """ dbgcore.BNDebuggerQuit(self.handle) - def quit_and_wait(self) -> None: + def quit_and_wait(self, timeout: Optional[int] = None) -> None: """ Terminate the target, and wait for all callback to be called + + :param timeout: optional timeout in milliseconds """ + if timeout is not None: + dbgcore.BNDebuggerQuitAndWaitWithTimeout(self.handle, timeout) + return dbgcore.BNDebuggerQuitAndWait(self.handle) def connect(self) -> bool: @@ -1560,12 +1569,16 @@ def connect(self) -> bool: """ return dbgcore.BNDebuggerConnect(self.handle) - def connect_and_wait(self) -> DebugStopReason: + def connect_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Connect to a remote target (process) and wait for all debugger events to be processed The host and port of the remote target must first be specified by setting `remote_host` and `remote_port` + + :param timeout: optional timeout in milliseconds """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerConnectAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerConnectAndWait(self.handle)) def connect_to_debug_server(self) -> bool: @@ -1588,6 +1601,17 @@ def detach(self) -> None: """ dbgcore.BNDebuggerDetach(self.handle) + def detach_and_wait(self, timeout: Optional[int] = None) -> None: + """ + Detach the target, and wait for all callback to be called. + + :param timeout: optional timeout in milliseconds + """ + if timeout is not None: + dbgcore.BNDebuggerDetachAndWaitWithTimeout(self.handle, timeout) + return + dbgcore.BNDebuggerDetachAndWait(self.handle) + def pause(self) -> None: """ Pause a running target @@ -1608,12 +1632,16 @@ def attach(self) -> bool: """ return dbgcore.BNDebuggerAttach(self.handle) - def attach_and_wait(self) -> DebugStopReason: + def attach_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Attach to a running process and wait until all debugger events are processed The PID of the target process must be set via DebuggerState.pid_attach + + :param timeout: optional timeout in milliseconds """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerAttachAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerAttachAndWait(self.handle)) def go(self) -> bool: @@ -1811,28 +1839,36 @@ def run_to_reverse(self, address) -> bool: return dbgcore.BNDebuggerRunToReverse(self.handle, addr_list, len(address)) - def go_and_wait(self) -> DebugStopReason: + def go_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Resume the target. The call is blocking and only returns when the target stops. + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerGoAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerGoAndWait(self.handle)) - def go_reverse_and_wait(self) -> DebugStopReason: + def go_reverse_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Resume the target in reverse. The call is blocking and only returns when the target stops. + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerGoReverseAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerGoReverseAndWait(self.handle)) - def step_into_and_wait(self, il: binaryninja.FunctionGraphType = - binaryninja.FunctionGraphType.NormalFunctionGraph) -> DebugStopReason: + def step_into_and_wait( + self, il: binaryninja.FunctionGraphType = binaryninja.FunctionGraphType.NormalFunctionGraph, + timeout: Optional[int] = None + ) -> DebugStopReason: """ Perform a step into on the target in reverse. @@ -1849,13 +1885,17 @@ def step_into_and_wait(self, il: binaryninja.FunctionGraphType = The call is blocking and only returns when the target stops. :param il: optional IL level to perform the operation at + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepIntoAndWaitWithTimeout(self.handle, il, timeout)) return DebugStopReason(dbgcore.BNDebuggerStepIntoAndWait(self.handle, il)) - - def step_into_reverse_and_wait(self, il: binaryninja.FunctionGraphType = - binaryninja.FunctionGraphType.NormalFunctionGraph) -> DebugStopReason: + def step_into_reverse_and_wait( + self, il: binaryninja.FunctionGraphType = binaryninja.FunctionGraphType.NormalFunctionGraph, + timeout: Optional[int] = None + ) -> DebugStopReason: """ Perform a reverse step into on the target. @@ -1871,12 +1911,17 @@ def step_into_reverse_and_wait(self, il: binaryninja.FunctionGraphType = The call is blocking and only returns when the target stops. :param il: optional IL level to perform the operation at + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepIntoReverseAndWaitWithTimeout(self.handle, il, timeout)) return DebugStopReason(dbgcore.BNDebuggerStepIntoReverseAndWait(self.handle, il)) - def step_over_and_wait(self, il: binaryninja.FunctionGraphType = - binaryninja.FunctionGraphType.NormalFunctionGraph) -> DebugStopReason: + def step_over_and_wait( + self, il: binaryninja.FunctionGraphType = binaryninja.FunctionGraphType.NormalFunctionGraph, + timeout: Optional[int] = None + ) -> DebugStopReason: """ Perform a step over on the target. @@ -1893,12 +1938,17 @@ def step_over_and_wait(self, il: binaryninja.FunctionGraphType = The call is blocking and only returns when the target stops. :param il: optional IL level to perform the operation at + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepOverAndWaitWithTimeout(self.handle, il, timeout)) return DebugStopReason(dbgcore.BNDebuggerStepOverAndWait(self.handle, il)) - def step_over_reverse_and_wait(self, il: binaryninja.FunctionGraphType = - binaryninja.FunctionGraphType.NormalFunctionGraph) -> DebugStopReason: + def step_over_reverse_and_wait( + self, il: binaryninja.FunctionGraphType = binaryninja.FunctionGraphType.NormalFunctionGraph, + timeout: Optional[int] = None + ) -> DebugStopReason: """ Perform a step over on the target in reverse. @@ -1915,11 +1965,14 @@ def step_over_reverse_and_wait(self, il: binaryninja.FunctionGraphType = The call is blocking and only returns when the target stops. :param il: optional IL level to perform the operation at + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepOverReverseAndWaitWithTimeout(self.handle, il, timeout)) return DebugStopReason(dbgcore.BNDebuggerStepOverReverseAndWait(self.handle, il)) - def step_return_and_wait(self) -> DebugStopReason: + def step_return_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Perform a step return on the target. @@ -1935,11 +1988,27 @@ def step_return_and_wait(self) -> DebugStopReason: The call is blocking and only returns when the target stops. + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepReturnAndWaitWithTimeout(self.handle, timeout)) return DebugStopReason(dbgcore.BNDebuggerStepReturnAndWait(self.handle)) - def run_to_and_wait(self, address) -> DebugStopReason: + def step_return_reverse_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: + """ + Perform a step return on the target in reverse. + + The call is blocking and only returns when the target stops. + + :param timeout: optional timeout in milliseconds + :return: the reason for the stop + """ + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerStepReturnReverseAndWaitWithTimeout(self.handle, timeout)) + return DebugStopReason(dbgcore.BNDebuggerStepReturnReverseAndWait(self.handle)) + + def run_to_and_wait(self, address, timeout: Optional[int] = None) -> DebugStopReason: """ Resume the target, and wait for it to break at the given address(es). @@ -1950,6 +2019,7 @@ def run_to_and_wait(self, address) -> DebugStopReason: The call is blocking and only returns when the target stops. + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ if isinstance(address, int): @@ -1962,9 +2032,12 @@ def run_to_and_wait(self, address) -> DebugStopReason: for i in range(len(address)): addr_list[i] = address[i] + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerRunToAndWaitWithTimeout( + self.handle, addr_list, len(address), timeout)) return DebugStopReason(dbgcore.BNDebuggerRunToAndWait(self.handle, addr_list, len(address))) - def run_to_reverse_and_wait(self, address) -> DebugStopReason: + def run_to_reverse_and_wait(self, address, timeout: Optional[int] = None) -> DebugStopReason: """ Resume the target in reverse, and wait for it to break at the given address(es). @@ -1975,6 +2048,7 @@ def run_to_reverse_and_wait(self, address) -> DebugStopReason: The call is blocking and only returns when the target stops. + :param timeout: optional timeout in milliseconds :return: the reason for the stop """ if isinstance(address, int): @@ -1987,23 +2061,36 @@ def run_to_reverse_and_wait(self, address) -> DebugStopReason: for i in range(len(address)): addr_list[i] = address[i] + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerRunToReverseAndWaitWithTimeout( + self.handle, addr_list, len(address), timeout)) return DebugStopReason(dbgcore.BNDebuggerRunToReverseAndWait(self.handle, addr_list, len(address))) - def pause_and_wait(self) -> None: + def pause_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Pause a running target. The call is blocking and only returns when the target stops. + + :param timeout: optional timeout in milliseconds + :return: the reason for the stop """ - dbgcore.BNDebuggerPauseAndWait(self.handle) + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerPauseAndWaitWithTimeout(self.handle, timeout)) + return DebugStopReason(dbgcore.BNDebuggerPauseAndWait(self.handle)) - def restart_and_wait(self) -> None: + def restart_and_wait(self, timeout: Optional[int] = None) -> DebugStopReason: """ Restart a running target. The call is blocking and only returns when the target stops again after the restart. + + :param timeout: optional timeout in milliseconds + :return: the reason for the stop """ - dbgcore.BNDebuggerRestartAndWait(self.handle) + if timeout is not None: + return DebugStopReason(dbgcore.BNDebuggerRestartAndWaitWithTimeout(self.handle, timeout)) + return DebugStopReason(dbgcore.BNDebuggerRestartAndWait(self.handle)) @property def adapter_type(self) -> str: diff --git a/core/adapters/windowsnativeadapter.cpp b/core/adapters/windowsnativeadapter.cpp index dca66f25..f3145521 100644 --- a/core/adapters/windowsnativeadapter.cpp +++ b/core/adapters/windowsnativeadapter.cpp @@ -239,7 +239,15 @@ bool WindowsNativeAdapter::Detach() if (!m_activelyDebugging) return true; - m_shouldStop = true; + // Set the stop flag under m_debugMutex so the DebugLoop's condition_variable wait + // (whose predicate reads m_shouldStop) can't miss the wakeup if it is between evaluating + // the predicate and parking. Modifying the flag without the lock races with that window + // and can lose the notify, hanging the join() below forever even though m_shouldStop is + // atomic. + { + std::lock_guard lock(m_debugMutex); + m_shouldStop = true; + } // Wake up the debug thread if it's waiting m_debugCondition.notify_one(); @@ -247,13 +255,17 @@ bool WindowsNativeAdapter::Detach() if (m_debugThread.joinable()) m_debugThread.join(); - // Close all thread handles - for (auto& [tid, handle] : m_threads) + // Thread handles in m_threads come from debug events (CREATE_PROCESS/CREATE_THREAD); + // Windows closes those automatically when debugging ends, so we must not close them + // here (see HandleExitThread). Doing so raises STATUS_INVALID_HANDLE under a debugger. + m_threads.clear(); + + // The initial thread handle from CreateProcess is owned by us. + if (m_threadHandle) { - if (handle && handle != m_threadHandle) - CloseHandle(handle); + CloseHandle(m_threadHandle); + m_threadHandle = nullptr; } - m_threads.clear(); if (m_processHandle) { @@ -278,7 +290,15 @@ bool WindowsNativeAdapter::Quit() if (!m_activelyDebugging) return true; - m_shouldStop = true; + // Set the stop flag under m_debugMutex so the DebugLoop's condition_variable wait + // (whose predicate reads m_shouldStop) can't miss the wakeup if it is between evaluating + // the predicate and parking. Modifying the flag without the lock races with that window + // and can lose the notify, hanging the join() below forever even though m_shouldStop is + // atomic. + { + std::lock_guard lock(m_debugMutex); + m_shouldStop = true; + } // Wake up the debug thread if it's waiting m_debugCondition.notify_one(); @@ -290,13 +310,17 @@ bool WindowsNativeAdapter::Quit() if (m_debugThread.joinable()) m_debugThread.join(); - // Close all thread handles - for (auto& [tid, handle] : m_threads) + // Thread handles in m_threads come from debug events (CREATE_PROCESS/CREATE_THREAD); + // Windows closes those automatically when debugging ends, so we must not close them + // here (see HandleExitThread). Doing so raises STATUS_INVALID_HANDLE under a debugger. + m_threads.clear(); + + // The initial thread handle from CreateProcess is owned by us. + if (m_threadHandle) { - if (handle) - CloseHandle(handle); + CloseHandle(m_threadHandle); + m_threadHandle = nullptr; } - m_threads.clear(); if (m_processHandle) { @@ -322,13 +346,17 @@ void WindowsNativeAdapter::Reset() if (m_debugThread.joinable()) m_debugThread.join(); - // Close all thread handles - for (auto& [tid, handle] : m_threads) + // Thread handles in m_threads come from debug events (CREATE_PROCESS/CREATE_THREAD); + // Windows closes those automatically when debugging ends, so we must not close them + // here (see HandleExitThread). Doing so raises STATUS_INVALID_HANDLE under a debugger. + m_threads.clear(); + + // The initial thread handle from CreateProcess is owned by us. + if (m_threadHandle) { - if (handle) - CloseHandle(handle); + CloseHandle(m_threadHandle); + m_threadHandle = nullptr; } - m_threads.clear(); // Close process handle if (m_processHandle) @@ -2732,7 +2760,12 @@ bool WindowsNativeAdapter::Go() // During this continue, all threads should run normally. If another thread hits a breakpoint, // that's expected behavior (the debugger stops). Only StepInto() uses scheduler-locking. - m_targetRunning = true; + // Publish the resume under m_debugMutex so the parked DebugLoop predicate observes it and + // the notify can't be lost (see Quit for the race detail). + { + std::lock_guard lock(m_debugMutex); + m_targetRunning = true; + } m_debugCondition.notify_one(); // Notify that the target has resumed @@ -2836,7 +2869,12 @@ bool WindowsNativeAdapter::StepInto() } } - m_targetRunning = true; + // Publish the resume under m_debugMutex so the parked DebugLoop predicate observes it and + // the notify can't be lost (see Quit for the race detail). + { + std::lock_guard lock(m_debugMutex); + m_targetRunning = true; + } m_debugCondition.notify_one(); // Notify that the target has resumed diff --git a/core/debuggercontroller.cpp b/core/debuggercontroller.cpp index d876a4d1..c478fc29 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,45 @@ 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(); }); + + m_interruptThread = std::thread([this] { InterruptThreadMain(); }); } 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(); + + // Stop the interrupt thread before the adapter/state are torn down below: it touches + // m_adapter and m_state->AdapterAccessMutex() in BreakInto, both of which outlive this + // join but not the delete m_state further down. + { + std::lock_guard interruptLock(m_interruptMutex); + m_interruptShouldExit = true; + } + m_interruptCv.notify_all(); + if (m_interruptThread.joinable()) + m_interruptThread.join(); + m_shouldExit = true; m_cv.notify_all(); if (m_debuggerEventThread.joinable()) @@ -60,6 +96,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,15 +348,13 @@ bool DebuggerController::Launch() if (!CanStartDebgging()) return false; - DbgRef self = this; - std::thread([self]() { self->LaunchAndWait(); }).detach(); + Submit([this] { LaunchAndWaitOnWorker(); }); return true; } DebugStopReason DebuggerController::LaunchAndWaitInternal() { - m_userRequestedBreak = false; if (Settings::Instance()->Get("debugger.safeMode")) { @@ -330,32 +386,33 @@ 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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = LaunchAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +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; - DbgRef self = this; - std::thread([self]() { self->AttachAndWait(); }).detach(); + Submit([this] { AttachAndWaitOnWorker(); }); return true; } @@ -363,7 +420,6 @@ bool DebuggerController::Attach() DebugStopReason DebuggerController::AttachAndWaitInternal() { m_firstAttach = false; - m_userRequestedBreak = false; DebuggerEvent event; event.type = LaunchEventType; @@ -382,32 +438,33 @@ 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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = AttachAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +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; - DbgRef self = this; - std::thread([self]() { self->ConnectAndWait(); }).detach(); + Submit([this] { ConnectAndWaitOnWorker(); }); return true; } @@ -415,7 +472,6 @@ bool DebuggerController::Connect() DebugStopReason DebuggerController::ConnectAndWaitInternal() { m_firstConnect = false; - m_userRequestedBreak = false; DebuggerEvent event; event.type = LaunchEventType; @@ -434,28 +490,28 @@ 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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = ConnectAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } -bool DebuggerController::Execute() +DebugStopReason DebuggerController::ConnectAndWait(std::chrono::milliseconds timeout) { - std::unique_lock lock(m_targetControlMutex); + return SubmitAndWait([this] { return ConnectAndWaitOnWorker(); }, timeout); +} + +bool DebuggerController::Execute() +{ std::string filePath = m_state->GetExecutablePath(); bool requestTerminal = m_state->GetRequestTerminalEmulator(); LaunchConfigurations configs = {requestTerminal, m_state->GetInputFile(), m_state->IsConnectedToDebugServer()}; @@ -546,8 +602,7 @@ bool DebuggerController::Go() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->GoAndWait(); }).detach(); + Submit([this] { GoAndWaitOnWorker(); }); return true; } @@ -558,48 +613,52 @@ bool DebuggerController::GoReverse() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - 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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = GoReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +DebugStopReason DebuggerController::GoReverseAndWait(std::chrono::milliseconds timeout) +{ + return SubmitAndWait([this] { return GoReverseAndWaitOnWorker(); }, timeout); +} + + DebugStopReason DebuggerController::StepIntoIL(BNFunctionGraphType il) { switch (il) @@ -812,7 +871,6 @@ DebugStopReason DebuggerController::StepIntoReverseIL(BNFunctionGraphType il) DebugStopReason DebuggerController::StepIntoReverseAndWaitInternal() { - m_userRequestedBreak = false; // TODO: check if StepInto() succeeds return ExecuteAdapterAndWait(DebugAdapterStepIntoReverse); } @@ -822,8 +880,7 @@ bool DebuggerController::StepInto(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepIntoAndWait(il); }).detach(); + Submit([this, il] { StepIntoAndWaitOnWorker(il); }); return true; } @@ -833,46 +890,49 @@ bool DebuggerController::StepIntoReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - 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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - 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) @@ -1085,8 +1145,7 @@ bool DebuggerController::StepOver(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->StepOverAndWait(il); }).detach(); + Submit([this, il] { StepOverAndWaitOnWorker(il); }); return true; } @@ -1097,49 +1156,54 @@ bool DebuggerController::StepOverReverse(BNFunctionGraphType il) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, il]() { self->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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = StepOverIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } -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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = StepOverReverseIL(il); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +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(); @@ -1163,7 +1227,6 @@ DebugStopReason DebuggerController::EmulateStepReturnAndWait() DebugStopReason DebuggerController::StepReturnAndWaitInternal() { - m_userRequestedBreak = false; if (true /* StepReturnAvailable() */) { @@ -1179,7 +1242,6 @@ DebugStopReason DebuggerController::StepReturnAndWaitInternal() DebugStopReason DebuggerController::StepReturnReverseAndWaitInternal() { - m_userRequestedBreak = false; if (true /* StepReturnReverseAvailable() */) { @@ -1193,8 +1255,7 @@ bool DebuggerController::StepReturn() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->StepReturnAndWait(); }).detach(); + Submit([this] { StepReturnAndWaitOnWorker(); }); return true; } @@ -1205,52 +1266,54 @@ bool DebuggerController::StepReturnReverse() if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self]() { self->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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = StepReturnAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } -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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = StepReturnReverseAndWaitInternal(); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +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) { @@ -1276,7 +1339,6 @@ DebugStopReason DebuggerController::RunToAndWaitInternal(const std::vector& remoteAddresses) { - m_userRequestedBreak = false; for (uint64_t remoteAddress : remoteAddresses) { @@ -1306,8 +1368,7 @@ bool DebuggerController::RunTo(const std::vector& remoteAddresses) if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, remoteAddresses]() { self->RunToAndWait(remoteAddresses); }).detach(); + Submit([this, remoteAddresses] { RunToAndWaitOnWorker(remoteAddresses); }); return true; } @@ -1319,49 +1380,56 @@ bool DebuggerController::RunToReverse(const std::vector& remoteAddress if (!CanResumeTarget()) return false; - DbgRef self = this; - std::thread([self, remoteAddresses]() { self->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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = RunToAndWaitInternal(remoteAddresses); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } -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()) return InvalidStatusOrOperation; - if (!m_targetControlMutex.try_lock()) - return InternalError; - auto reason = RunToReverseAndWaitInternal(remoteAddresses); - if (!m_userRequestedBreak && (reason != ProcessExited) && (reason != InternalError)) + if ((reason != ProcessExited) && (reason != InternalError)) NotifyStopped(reason); - m_targetControlMutex.unlock(); return reason; } +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(); @@ -1470,19 +1538,40 @@ bool DebuggerController::Restart() if (!m_state->IsConnected()) return false; - DbgRef self = this; - std::thread([self]() { self->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; + + if (std::this_thread::get_id() == m_dispatcherThreadId) + { + LogError("Synchronous debugger API called from debugger callback thread; use async APIs from callbacks"); + return InternalError; + } + + RequestInterrupt(); + return SubmitAndWait([this] { return RestartAndWaitOnWorker(); }, timeout); } @@ -1531,32 +1620,38 @@ void DebuggerController::Detach() if (!m_state->IsConnected()) return; - DbgRef self = this; - std::thread([self]() { self->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()) - locked = true; - if (!m_state->IsConnected()) - { - if (locked) - m_targetControlMutex.unlock(); return; - } // TODO: return whether the operation is successful ExecuteAdapterAndWait(DebugAdapterDetach); // There is no need to notify a detached event at this point, since the detach event is already processed // by all the callback +} + - if (locked) - m_targetControlMutex.unlock(); +void DebuggerController::DetachAndWait(std::chrono::milliseconds timeout) +{ + if (!m_state->IsConnected()) + return; + + if (std::this_thread::get_id() == m_dispatcherThreadId) + { + LogError("Synchronous debugger API called from debugger callback thread; use async APIs from callbacks"); + return; + } + + RequestInterrupt(); + SubmitAndWait([this] { DetachAndWaitOnWorker(); }, timeout); } @@ -1565,28 +1660,26 @@ void DebuggerController::Quit() if (!m_state->IsConnected()) return; - DbgRef self = this; - std::thread([self]() { self->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()) - locked = true; - if (!m_state->IsConnected()) - { - if (locked) - m_targetControlMutex.unlock(); return; - } 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 @@ -1594,9 +1687,22 @@ void DebuggerController::QuitAndWait() // There is no need to notify a TargetExitedEvent at this point, since the exit event is already processed // by all the callback +} + + +void DebuggerController::QuitAndWait(std::chrono::milliseconds timeout) +{ + if (!m_state->IsConnected()) + return; + + if (std::this_thread::get_id() == m_dispatcherThreadId) + { + LogError("Synchronous debugger API called from debugger callback thread; use async APIs from callbacks"); + return; + } - if (locked) - m_targetControlMutex.unlock(); + RequestInterrupt(); + SubmitAndWait([this] { QuitAndWaitOnWorker(); }, timeout); } @@ -1605,48 +1711,66 @@ bool DebuggerController::Pause() if (!m_state->IsConnected()) return false; - DbgRef self = this; - std::thread([self]() { self->PauseAndWait(); }).detach(); - + // Out-of-band: ask the interrupt thread to break the engine (returns immediately). + // 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; + if (std::this_thread::get_id() == m_dispatcherThreadId) + { + LogError("Synchronous debugger API called from debugger callback thread; use async APIs from callbacks"); + return InternalError; + } + + 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 TimedOut; + } + + 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); } @@ -1662,7 +1786,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); @@ -1705,7 +1833,6 @@ DebugStopReason DebuggerController::EmulateStepOverReverseAndWait() DebugStopReason DebuggerController::StepOverAndWaitInternal() { - m_userRequestedBreak = false; if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOver)) { @@ -1720,7 +1847,6 @@ DebugStopReason DebuggerController::StepOverAndWaitInternal() DebugStopReason DebuggerController::StepOverReverseAndWaitInternal() { - m_userRequestedBreak = false; if (m_adapter && m_adapter->SupportFeature(DebugAdapterSupportStepOverReverse)) { @@ -1859,8 +1985,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,38 +2019,36 @@ void DebuggerController::EventHandler(const DebuggerEvent& event) } case TargetExitedEventType: m_exitCode = (uint32_t)event.data.exitData.exitCode; + [[fallthrough]]; case DetachedEventType: case LaunchFailureEventType: { + // Light, lock-free state only -- safe to run even while the adapter lock is held (it is, + // when this runs synchronously inside a locked ExecuteAdapterAndWait op such as Quit). m_state->SetConnectionStatus(DebugAdapterNotConnectedStatus); m_state->SetExecutionStatus(DebugAdapterInvalidStatus); - m_state->MarkDirty(); m_inputFileLoaded = false; m_initialBreakpointSeen = false; ClearTTDPositionHistory(); - RemoveDebuggerMemoryRegion(); - if (m_oldAnalysisState != HoldState) - { - m_data->SetAnalysisHold(false); - } - 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. - auto* accessor = m_accessor; - m_accessor = nullptr; - std::thread([accessor]() { delete accessor; }).detach(); - } m_lastIP = m_currentIP; m_currentIP = 0; + + // The remaining cleanup (MarkDirty / RemoveDebuggerMemoryRegion / accessor disposal / + // analysis hold) calls into BN core, which takes the file lock. Running it here would mean + // holding the adapter lock across a BN-core call whenever this fires inside a locked op + // (Quit/Detach) -- the AB-BA deadlock with the analysis read path. For target-gone/detach + // it is instead run by ExecuteAdapterAndWait / the worker AFTER the adapter lock is + // released (see FinalizeTargetGoneCleanup). LaunchFailure does not flow through that path + // (and has no live memory region to remove), so finalize it inline. + // + // Note: the accessor MUST be disposed in FinalizeTargetGoneCleanup, AFTER + // RemoveDebuggerMemoryRegion -- the BinaryView's MemoryMap holds a raw pointer to it from + // AddRemoteMemoryRegion, and any in-flight or about-to-fire BinaryView::Read (e.g. a + // LinearView refresh triggered by the very same TargetExited event reaching the UI) would + // otherwise hit a freed accessor. + if (event.type == LaunchFailureEventType) + FinalizeTargetGoneCleanup(); break; } case TargetStoppedEventType: @@ -1952,6 +2094,37 @@ void DebuggerController::EventHandler(const DebuggerEvent& event) } +// Idempotent. Calls into BN core (memory map / analysis), which takes the file lock, so it MUST be +// invoked with no adapter lock held. Safe to call more than once: MarkDirty re-marks an +// already-cleared cache, RemoveMemoryRegion no-ops when the region is already gone, accessor +// disposal is guarded by the nullptr check, and SetAnalysisHold(false) is idempotent -- so the +// redundant case (e.g. both WindowsNativeAdapter::Quit and the debug loop posted TargetExited) is +// harmless. +void DebuggerController::FinalizeTargetGoneCleanup() +{ + m_state->MarkDirty(); + // Remove the region from the BinaryView's MemoryMap BEFORE disposing of m_accessor: the + // MemoryMap holds a raw pointer to it (see AddRemoteMemoryRegion in DebuggerController::Start), + // so freeing the accessor first would leave the map with a dangling pointer that any concurrent + // BinaryView::Read (e.g. a linear-view refresh triggered by TargetExited) would dereference. + RemoveDebuggerMemoryRegion(); + if (m_accessor) + { + // 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(). FinalizeTargetGoneCleanup + // itself runs on the worker (via ExecuteAdapterAndWait or the Submit fallback in + // PostDebuggerEvent), so a synchronous delete on the last ref would self-join and deadlock. + // A detached thread sidesteps that regardless of who called us. + auto* accessor = m_accessor; + m_accessor = nullptr; + std::thread([accessor]() { delete accessor; }).detach(); + } + if (m_oldAnalysisState != HoldState) + m_data->SetAnalysisHold(false); +} + + size_t DebuggerController::RegisterEventCallback( std::function callback, const std::string& name) { @@ -2013,11 +2186,62 @@ 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(); + else + // No controller op is in flight (e.g. the target exited on its own, or an adapter that + // reports exits asynchronously). No ExecuteAdapterAndWait will run the deferred cleanup, + // so queue it on the worker, where no adapter lock is held. Idempotent, so it is fine + // even if a later op also triggers it. + Submit([this] { FinalizeTargetGoneCleanup(); }); + // Fall through: still goes through the public dispatcher queue. + } auto pending = std::make_shared(); pending->event = event; @@ -2043,6 +2267,113 @@ 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, and fully asynchronous: hand the break off to the interrupt + // thread and return immediately. The caller (Pause/Restart/Quit/Detach, possibly on + // the UI thread) never blocks on the adapter call. The in-flight worker op (blocked + // in WaitForAdapterStop) wakes up via the adapter-stop channel once the engine breaks; + // its OnWorker wrapper then calls NotifyStopped. We do not call NotifyStopped here and + // we do not queue any work for the worker. + // + // Multiple requests collapse to one flag: BreakInto is only meaningful per in-flight + // op, and the worker cannot advance to the next queued op until this break unsticks it, + // so a coalesced break can never land on a later operation's target. + { + std::lock_guard lock(m_interruptMutex); + m_interruptRequested = true; + } + m_interruptCv.notify_one(); +} + + +void DebuggerController::InterruptThreadMain() +{ + while (true) + { + { + std::unique_lock lock(m_interruptMutex); + m_interruptCv.wait(lock, [this] { return m_interruptShouldExit || m_interruptRequested; }); + if (m_interruptShouldExit && !m_interruptRequested) + break; + m_interruptRequested = false; + } + + // Snapshot the adapter pointer once so the null check and the call see the same + // value. The pointed-to object outlives this thread: the adapter is destroyed in + // ~DebuggerState, which runs in ~DebuggerController only after this thread has been + // 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; @@ -2066,49 +2397,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) @@ -2127,29 +2420,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,6 +2945,8 @@ std::string DebuggerController::GetStopReasonString(DebugStopReason reason) return "UserRequestedBreak"; case OperationNotSupported: return "OperationNotSupported"; + case TimedOut: + return "TimedOut"; default: return ""; } @@ -2692,54 +2964,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)) - { - if (!m_adapterMutex.try_lock()) - { - LogWarn("Cannot obtain mutex1 for debug adapter, operation: %d", operation); - return InternalError; - } - } - else + // 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_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: @@ -2787,6 +3038,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) @@ -2806,16 +3059,53 @@ 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; + } + + // The target is gone (process exit or detach -- both surface as ProcessExited). Run the + // BN-core cleanup that ApplyOwnStateForEvent deliberately deferred. The adapter lock was + // dropped above (before the run-wait), so we are NOT holding it across these file-lock-taking + // calls -- this is what avoids the AB-BA deadlock with the analysis read path. Running it here, + // on the worker before we return, keeps the caller's post-wait view fully finalized. + if (reason == ProcessExited) + FinalizeTargetGoneCleanup(); return reason; } diff --git a/core/debuggercontroller.h b/core/debuggercontroller.h index f298d902..4d50d4a9 100644 --- a/core/debuggercontroller.h +++ b/core/debuggercontroller.h @@ -16,12 +16,15 @@ 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 #include "ffi_global.h" #include "refcountobject.h" @@ -30,6 +33,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 +86,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 +119,6 @@ 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; - 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 +126,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: ask the interrupt thread to break the engine. Returns immediately + // (does not block the caller on the adapter call). Called from Pause/Restart/Quit/ + // Detach 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 +160,17 @@ 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); + // Idempotent "target is gone" cleanup (memory cache + the "debugger" BinaryView region + + // analysis hold). These call into BN core, which takes the file lock, so this MUST run with + // no adapter lock held -- see ExecuteAdapterAndWait, which invokes it after releasing the + // lock. Deliberately NOT done inline in ApplyOwnStateForEvent, which can run while the + // adapter lock is held (that is the AB-BA deadlock with the analysis read path). + void FinalizeTargetGoneCleanup(); void UpdateStackVariables(); void AddRegisterValuesToExpressionParser(); void AddModuleValuesToExpressionParser(); @@ -168,6 +203,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 (which hands the break to the + // interrupt 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 +260,104 @@ 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(); + + // Interrupt thread: a single owned thread whose only job is to issue out-of-band + // BreakInto() calls. RequestInterrupt() (called from Pause/Restart/Quit/Detach on + // arbitrary caller threads) just sets m_interruptRequested and notifies, then + // returns -- so callers never block on a synchronous adapter call. + // + // The BreakInto must run off the worker thread (the worker is blocked inside + // WaitForAdapterStop on the very op being interrupted) and we don't want it on the + // caller's thread either (the UI thread must not block on an adapter call). This + // thread is the third option: owned and joined like m_workerThread, so it can never + // outlive the controller or touch a destroyed adapter. + std::thread m_interruptThread; + std::mutex m_interruptMutex; + std::condition_variable m_interruptCv; + bool m_interruptRequested = false; + bool m_interruptShouldExit = false; + void InterruptThreadMain(); + + // 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. A timeout of + // milliseconds::max() means "wait forever" and bypasses wait_for entirely (avoids + // overflow inside the stdlib). When the timeout elapses, return a timeout result + // without interrupting the target; the queued worker task keeps running and will + // eventually publish its normal events. + // + // This is the synchronous public-API path: it must NOT be called from the worker + // thread itself or from debugger callbacks. 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). Callback code should use async + // APIs such as Go() / StepInto() so it does not wait on the worker while the worker + // is waiting for event dispatch to complete. + template + auto SubmitAndWait(F&& f, std::chrono::milliseconds timeout) + -> std::invoke_result_t + { + BN_RELEASE_ASSERT(t_controllerOnWorker != this); + using R = std::invoke_result_t; + if (std::this_thread::get_id() == m_dispatcherThreadId) + { + LogError("Synchronous debugger API called from debugger callback thread; use async APIs from callbacks"); + if constexpr (std::is_void_v) + return; + else if constexpr (std::is_same_v) + return InternalError; + else + return R {}; + } + auto fut = Submit(std::forward(f)); + if (timeout != std::chrono::milliseconds::max()) + { + if (fut.wait_for(timeout) != std::future_status::ready) + { + if constexpr (std::is_void_v) + return; + else if constexpr (std::is_same_v) + return TimedOut; + else + return R {}; + } + } + return fut.get(); + } + std::unique_ptr m_uiCallbacks; uint64_t m_oldViewBase, m_newViewBase; @@ -351,23 +508,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/core/ffi.cpp b/core/ffi.cpp index 5b2a7dfd..17c0cdd5 100644 --- a/core/ffi.cpp +++ b/core/ffi.cpp @@ -21,10 +21,16 @@ limitations under the License. #include "debuggercontroller.h" #include "debuggercommon.h" #include "../api/ffi.h" +#include #include using namespace BinaryNinjaDebugger; +static std::chrono::milliseconds TimeoutMs(uint64_t timeoutMs) +{ + return std::chrono::milliseconds(timeoutMs); +} + char* BNDebuggerAllocString(const char* contents) { @@ -399,6 +405,12 @@ BNDebugStopReason BNDebuggerLaunchAndWait(BNDebuggerController* controller) } +BNDebugStopReason BNDebuggerLaunchAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->LaunchAndWait(TimeoutMs(timeoutMs)); +} + + bool BNDebuggerExecute(BNDebuggerController* controller) { return controller->object->Execute(); @@ -424,6 +436,12 @@ void BNDebuggerQuitAndWait(BNDebuggerController* controller) } +void BNDebuggerQuitAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + controller->object->QuitAndWait(TimeoutMs(timeoutMs)); +} + + bool BNDebuggerConnect(BNDebuggerController* controller) { return controller->object->Connect(); @@ -436,6 +454,12 @@ BNDebugStopReason BNDebuggerConnectAndWait(BNDebuggerController* controller) } +BNDebugStopReason BNDebuggerConnectAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->ConnectAndWait(TimeoutMs(timeoutMs)); +} + + bool BNDebuggerConnectToDebugServer(BNDebuggerController* controller) { return controller->object->ConnectToDebugServer(); @@ -454,6 +478,18 @@ void BNDebuggerDetach(BNDebuggerController* controller) } +void BNDebuggerDetachAndWait(BNDebuggerController* controller) +{ + controller->object->DetachAndWait(); +} + + +void BNDebuggerDetachAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + controller->object->DetachAndWait(TimeoutMs(timeoutMs)); +} + + void BNDebuggerPause(BNDebuggerController* controller) { controller->object->Pause(); @@ -479,6 +515,12 @@ BNDebugStopReason BNDebuggerAttachAndWait(BNDebuggerController* controller) } +BNDebugStopReason BNDebuggerAttachAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->AttachAndWait(TimeoutMs(timeoutMs)); +} + + bool BNDebuggerGo(BNDebuggerController* controller) { return controller->object->Go(); @@ -556,47 +598,99 @@ BNDebugStopReason BNDebuggerGoAndWait(BNDebuggerController* controller) } +BNDebugStopReason BNDebuggerGoAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->GoAndWait(TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerGoReverseAndWait(BNDebuggerController* controller) { return controller->object->GoReverseAndWait(); } +BNDebugStopReason BNDebuggerGoReverseAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->GoReverseAndWait(TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerStepIntoAndWait(BNDebuggerController* controller, BNFunctionGraphType il) { return controller->object->StepIntoAndWait(il); } +BNDebugStopReason BNDebuggerStepIntoAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs) +{ + return controller->object->StepIntoAndWait(il, TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerStepIntoReverseAndWait(BNDebuggerController* controller, BNFunctionGraphType il) { return controller->object->StepIntoReverseAndWait(il); } +BNDebugStopReason BNDebuggerStepIntoReverseAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs) +{ + return controller->object->StepIntoReverseAndWait(il, TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerStepOverAndWait(BNDebuggerController* controller, BNFunctionGraphType il) { return controller->object->StepOverAndWait(il); } + +BNDebugStopReason BNDebuggerStepOverAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs) +{ + return controller->object->StepOverAndWait(il, TimeoutMs(timeoutMs)); +} + BNDebugStopReason BNDebuggerStepOverReverseAndWait(BNDebuggerController* controller, BNFunctionGraphType il) { return controller->object->StepOverReverseAndWait(il); } +BNDebugStopReason BNDebuggerStepOverReverseAndWaitWithTimeout( + BNDebuggerController* controller, BNFunctionGraphType il, uint64_t timeoutMs) +{ + return controller->object->StepOverReverseAndWait(il, TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerStepReturnAndWait(BNDebuggerController* controller) { return controller->object->StepReturnAndWait(); } +BNDebugStopReason BNDebuggerStepReturnAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->StepReturnAndWait(TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerStepReturnReverseAndWait(BNDebuggerController* controller) { return controller->object->StepReturnReverseAndWait(); } +BNDebugStopReason BNDebuggerStepReturnReverseAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->StepReturnReverseAndWait(TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerRunToAndWait( BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count) { @@ -610,6 +704,19 @@ BNDebugStopReason BNDebuggerRunToAndWait( } +BNDebugStopReason BNDebuggerRunToAndWaitWithTimeout( + BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count, uint64_t timeoutMs) +{ + std::vector addresses; + addresses.reserve(count); + for (size_t i = 0; i < count; i++) + { + addresses.push_back(remoteAddresses[i]); + } + return controller->object->RunToAndWait(addresses, TimeoutMs(timeoutMs)); +} + + BNDebugStopReason BNDebuggerRunToReverseAndWait( BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count) { @@ -623,18 +730,43 @@ BNDebugStopReason BNDebuggerRunToReverseAndWait( } +BNDebugStopReason BNDebuggerRunToReverseAndWaitWithTimeout( + BNDebuggerController* controller, const uint64_t* remoteAddresses, size_t count, uint64_t timeoutMs) +{ + std::vector addresses; + addresses.reserve(count); + for (size_t i = 0; i < count; i++) + { + addresses.push_back(remoteAddresses[i]); + } + return controller->object->RunToReverseAndWait(addresses, TimeoutMs(timeoutMs)); +} + + DebugStopReason BNDebuggerPauseAndWait(BNDebuggerController* controller) { return controller->object->PauseAndWait(); } +DebugStopReason BNDebuggerPauseAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->PauseAndWait(TimeoutMs(timeoutMs)); +} + + DebugStopReason BNDebuggerRestartAndWait(BNDebuggerController* controller) { return controller->object->RestartAndWait(); } +DebugStopReason BNDebuggerRestartAndWaitWithTimeout(BNDebuggerController* controller, uint64_t timeoutMs) +{ + return controller->object->RestartAndWait(TimeoutMs(timeoutMs)); +} + + char* BNDebuggerGetAdapterType(BNDebuggerController* controller) { if (!controller->object->GetState()) diff --git a/test/debugger_test.py b/test/debugger_test.py index 530bd198..2bd3b5f9 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() @@ -379,6 +377,17 @@ def test_thread(self): self.assertGreater(len(threads), 1) dbg.quit_and_wait() + def test_go_and_wait_timeout(self): + fpath = name_to_fpath('helloworld_thread', self.arch) + bv = load(fpath) + dbg = self.create_debugger(bv) + try: + self.assertNotIn(dbg.launch_and_wait(), [DebugStopReason.ProcessExited, DebugStopReason.InternalError]) + reason = dbg.go_and_wait(5000) + self.assertEqual(reason, DebugStopReason.TimedOut) + finally: + dbg.quit_and_wait() + @unittest.skipIf(platform.system() == 'Windows', 'Skip restart test on Windows for now') def test_restart(self): fpath = name_to_fpath('helloworld_thread', self.arch) @@ -386,17 +395,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() diff --git a/ui/controlswidget.cpp b/ui/controlswidget.cpp index 58ffd980..3b119237 100644 --- a/ui/controlswidget.cpp +++ b/ui/controlswidget.cpp @@ -410,25 +410,28 @@ void DebugControlsWidget::performAttachPID() void DebugControlsWidget::performRestart() { - std::thread([&]() { m_controller->Restart(); }).detach(); + // Restart/Quit/Detach/Pause return immediately: they signal the interrupt thread and + // queue the work onto the controller's worker thread, so no spawned UI-side thread is + // needed (same as performResume/performStepInto below). + m_controller->Restart(); } void DebugControlsWidget::performQuit() { - std::thread([&]() { m_controller->Quit(); }).detach(); + m_controller->Quit(); } void DebugControlsWidget::performDetach() { - std::thread([&]() { m_controller->Detach(); }).detach(); + m_controller->Detach(); } void DebugControlsWidget::performPause() { - std::thread([&]() { m_controller->Pause(); }).detach(); + m_controller->Pause(); }