X2WinRpcAdapter: remote Windows debugging over FlatBuffers RPC - #1174
Open
Weitao-Sun wants to merge 26 commits into
Open
X2WinRpcAdapter: remote Windows debugging over FlatBuffers RPC#1174Weitao-Sun wants to merge 26 commits into
Weitao-Sun wants to merge 26 commits into
Conversation
Introduces a new cross-platform X2WIN_RPC debug adapter that will talk to a Windows-side stub (x2winstub, WIN32-only, scaffolded but not yet implemented) over a custom TCP RPC protocol, to support debugging Windows targets from macOS/Linux without depending on lldb-server's immature Windows support or DbgEng's Windows-only client library. Lifecycle (Attach/Connect/Execute/Detach/Quit) and GetTargetArchitecture are implemented against the wire protocol; the remaining DebugAdapter methods are placeholder stubs to keep the class concrete while the protocol and stub are built out incrementally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes the connect/reconnect crash risk (ConnectSocket now no-ops if already connected instead of reassigning a live thread), reads the stub address from adapter settings instead of a hardcoded value, and adds the attach.pid setting the built-in Attach-to-Process flow relies on internally to carry the selected pid. Also wires up the TargetStopped event end-to-end: Detach/Quit now post DetachedEventType/TargetExitedEventType so DebuggerController's connection-state tracking and WaitForAdapterStop() don't get stuck, and ReaderLoop() decodes the stub's stop-reason byte into a real DebugStopReason instead of dropping Event frames on the floor. Verified end-to-end against a throwaway Python stub: connect, list fake processes, attach, receive the stopped notification, detach, and attach again all work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the hand-rolled frame format (manual FrameType/MethodId enums and byte-packing helpers) with a single protobuf Envelope message using a oneof to distinguish requests/responses/events, defined in protocol/x2win.proto (replacing the empty placeholder). This removes an entire class of manual encode/decode bugs and gives the not-yet-written Windows stub an unambiguous schema to implement against instead of reverse-engineering byte offsets. Protobuf is wired into core/CMakeLists.txt the same way LLDB already is: an externally-built dependency located via a PROTOBUF_PATH environment variable with a platform-appropriate default, not vendored or fetched by the build. build.md documents building it from source as a static lib (so debuggercore doesn't pick up a runtime dependency on a system-installed Protobuf); the CMAKE_CXX_STANDARD=20 flag in those instructions is required to avoid an Abseil ABI mismatch between its installed headers and compiled binaries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Go() and both AddBreakpoint() overloads were still stub returns that never touched the wire; Resume and SetBreakpoint requests silently did nothing. ConnectToDebugServer() was unimplemented entirely. All three now round-trip through CallSync() the same way Attach()/Detach() already did. ReaderLoop() also stashes the reason/address from each TargetStoppedEvent into new atomic members so StopReason()/GetInstructionOffset() can report real values instead of hardcoded UnknownReason/0 -- needed for DebuggerController's stop-reason-driven resume logic to behave correctly. AddBreakpoint(ModuleNameAndOffset&) needed ResolveModuleAddress(), which was declared but never defined; added it following LldbAdapter's pattern. protocol/x2win.proto gains the corresponding ConnectServerRequest/Response, GoRequest/Response, SetBreakpointRequest/Response + BreakpointType, and an address field on TargetStoppedEvent plus STOP_REASON_INITIAL_BREAKPOINT. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Protocol: - Finish the switch from protobuf to flatbuffers (vendor/flatbuffers submodule, protocol/x2win.fbs replaces x2win.proto) and bring x2winstub's local mirror fully up to date (main.cpp, net/, debug/WindowsDebugEngine port, x2win_session). - Add StepIntoRequest/StepOverRequest/BreakIntoRequest/RemoveBreakpointRequest message pairs, and a size field on ModuleEntry. core/adapters/x2winrpcadapter.cpp: - Wire StepInto()/StepOver()/BreakInto()/RemoveBreakpoint() over the new RPCs, same CallSync pattern as Go(). - GetBreakpointList() now serves from a locally-maintained cache (kept in sync by AddBreakpoint()/RemoveBreakpoint()) instead of a live RPC, since the base class declares it const and CallSync() can't be called from a const method. - GetModuleList() extracts the module basename itself (recognizing both '/' and '\\') before storing it as short_name -- DebugModule::GetPathBaseName() only recognizes '\\' when compiled for Windows, which broke module-name matching (and therefore auto-rebase) since X2WinRpcAdapter is the first adapter where BN core can run on a different OS than the Windows debug target. - common.inputFile is now auto-populated from the BinaryView's file path (GenerateDefaultAdapterSettings, same convention as every other adapter), fixing the same rebase-matching path from the other side. - Go()/StepInto()/StepOver() now post ResumeEventType/StepIntoEventType/ StepOverEventType on success, which is what actually drives DebuggerState::IsRunning() -- previously always false for this adapter, which also meant CanResumeTarget() never blocked a second Go/Step while one was already in flight. core/debuggercontroller.cpp: - ApplyOwnStateForEvent: add StepOverEventType alongside Resume/StepIntoEventType so it also flips execution status to Running (additive only -- no existing adapter ever posts this event, so no behavior change for anyone else). x2winstub/CMakeLists.txt: - Add NOMINMAX/WIN32_LEAN_AND_MEAN so <Windows.h>'s max/min macros stop mangling flatbuffers' std::numeric_limits<T>::max() calls -- this was only surfacing on a genuinely clean build; incremental builds had been silently reusing stale .obj files for main.cpp/net/connection.cpp/x2win_session.cpp across several rounds of protocol changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cAdapter Register read/write: - protocol/x2win.fbs gains ReadAllRegistersRequest/Response, ReadRegisterRequest/Response, WriteRegisterRequest/Response, and a RegisterEntry table (name/value/width/register_index). Values are uint64 -- X2Win only ever targets x86/x64 Windows. - X2WinRpcAdapter::ReadAllRegisters()/ReadRegister()/WriteRegister() were stub returns; now round-trip through CallSync() like the other RPCs. Breakpoint resync after (re)connect: - DebuggerBreakpoints::Apply() replays every known breakpoint from CreateDebugAdapter(), which runs before Attach()/ExecuteWithArgs()/ Connect() has actually opened the socket -- AddBreakpoint() used to just fail silently in that window, so breakpoints never made it to a freshly (re)connected stub. AddBreakpoint(ModuleNameAndOffset&) now stages into m_pendingBreakpoints when not yet connected (or when the module isn't resolvable yet), and the new ApplyBreakPoints() flushes it once connected and again on every TargetStoppedEvent -- same shape as LldbAdapter::ApplyBreakpoints()'s pending-breakpoint handling. - RemoveBreakpoint() now also checks m_pendingBreakpoints first, so removing a breakpoint that hadn't been flushed yet doesn't silently no-op and then reappear on the next flush. - TeardownConnection() now clears m_breakpoints -- entries from a dead connection aren't trustworthy after a reconnect (fresh stub session, or a resend from DebuggerBreakpoints::Apply() racing a stale cached entry into a duplicate/ghost breakpoint). GetProcessList() no longer self-connects: - It used to call ConnectFromSettings() itself, independent of the controller's Launch/Attach/Connect/ConnectToDebugServer lifecycle. In target mode this could open a connection to a stub that immediately pushes an unsolicited TargetStoppedEvent on accept, which could drive DetectLoadedModule()/autoRebase through a path that never ran CreateDebuggerBinaryView() -- crashing on a null memory accessor. Now it just checks m_connected, matching GdbAdapter (unimplemented) and LldbAdapter (only ever queries an already-live backend session). Launch/Restart: - launch.executablePath/workingDirectory/commandLineArguments were never registered as adapter settings, so DebuggerState::GetExecutablePath() always returned "" and any Launch (including Restart's Quit-then-Launch) sent an empty path to the stub. Settings added, deliberately without a local file-picker uiSelectionAction since the path is a remote Windows path, not a local one. - ExecuteWithArgs() now refuses immediately (before touching the network) when the last successful connection was via Connect() (the target-mode entry point, UI: "Connect to Remote Process") -- a target-mode stub only ever owns the one debuggee it was started with, same as plain gdbserver vs gdbserver --multi. Without this, Restart in target mode would Quit the debuggee (causing the stub to exit, per its reconnect-loop design) and then hang trying to reconnect to a stub that no longer exists. Also drops x2winstub/engine_port_task.md and read_memory_task.md, superseded by the x2winstub/instruction_note/ task-doc workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SupportFeature() always returned false, so DebuggerController's StepOverAndWaitInternal() never used the already-wired native StepOver RPC and instead fell back to software step-over emulation. Report StepOver and Modules as supported since both are implemented over RPC; StepReturn, StepOverReverse, Threads, and TTD remain false since the stub doesn't support them yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
protocol/x2win.fbs gains WriteMemoryRequest/WriteMemoryResponse, mirroring ReadMemoryRequest/Response's synchronous request/response shape (address + byte vector in, success bool out, no separate async event). X2WinRpcAdapter::WriteMemory() was a stub returning false; now round-trips through CallSync() like ReadMemory()/WriteRegister(). This is what backs DebuggerFileAccessor::Write() (core/debuggerfileaccessor.cpp), i.e. editing bytes in the hex view or bv.write() against the live process view during a debug session. Verified end-to-end against the stub (write + read-back). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…worker deadlock in X2WinRpcAdapter Thread management: - protocol/x2win.fbs gains GetThreadListRequest/Response (ThreadEntry: tid/rip/is_frozen), GetActiveThreadIdRequest/Response, SetActiveThreadIdRequest/Response, SuspendThreadRequest/Response, and ResumeThreadRequest/Response. - X2WinRpcAdapter::GetThreadList()/GetActiveThread()/GetActiveThreadId()/ SetActiveThread()/SetActiveThreadId()/SuspendThread()/ResumeThread() were stub returns; now round-trip through CallSync(). GetActiveThread() derives rip from GetInstructionOffset() (the last reported stop) rather than a separate RPC, since BN only ever stops the whole process, never a single thread. - SupportFeature() now reports DebugAdapterSupportThreads. Hardware breakpoints: - protocol/x2win.fbs gains SetHardwareBreakpointRequest/Response and RemoveHardwareBreakpointRequest/Response (address/type/size triple, not an allocated id -- mirrors a debug register slot's own identity rule). - The 4 AddHardwareBreakpoint()/RemoveHardwareBreakpoint() overloads (absolute address and ModuleNameAndOffset) always returned false; now wire through CallSync(), reusing core's PendingHardwareBreakpoint to stage before the adapter is connected -- DebuggerBreakpoints::Apply() calls these unconditionally from CreateDebugAdapter(), same pre-connect timing problem AddBreakpoint(ModuleNameAndOffset&) already had to solve. ApplyBreakPoints() now flushes both the software and hardware pending lists. Report process exit over the wire (fixes a worker-thread deadlock): - StopReason gains EXITED, and TargetStoppedEvent gains exit_code. Stub-side process exit was previously invisible to BN core entirely -- the stub detects it (WindowsDebugEngine posts an internal TargetExited event) but nothing on the wire ever reported it, so DebuggerController:: WaitForAdapterStop() (an untimed condition_variable::wait) would block forever after a Go() whose target ran to completion on its own, and the real Detach()/Quit() RPC -- queued behind that stuck worker op -- would never even reach the stub. Only the out-of-band RequestInterrupt() -> BreakInto() (fired once per Detach/Quit click, on its own thread) made it onto the wire, uselessly, since the process was already gone. - ReaderLoop() now branches on StopReason_EXITED: caches the exit code, sets m_lastStopReason to ProcessExited, and posts TargetExitedEventType instead of AdapterStoppedEventType (skipping the ApplyBreakPoints() resync -- nothing to resend to). ExitCode() now returns the cached value instead of a hardcoded 0. - BreakInto() skips the RPC round trip entirely when m_lastStopReason is already ProcessExited, instead of logging a "stub reported failure" that isn't telling us anything new (RequestInterrupt() calls it unconditionally before every Detach()/Quit(), regardless of whether the target is still running). Also strips a stray trailing "\n" from one LogWarn call (Log already appends its own newline). Corresponding stub-side changes (x2win_session.cpp HandleRequest cases for the new thread/hardware-breakpoint RPCs, and OnEngineEvent forwarding TargetExited) delivered separately via x2winstub/instruction_note/ task docs, per the BN-core/stub split -- see x2winrpcadapter-task-doc-workflow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Call stacks:
- protocol/x2win.fbs gains FrameEntry (index/pc/sp/fp/function_name/
function_start/module -- mirrors BN's DebugFrame) and
GetFramesOfThreadRequest/Response.
- X2WinRpcAdapter::GetFramesOfThread() fell back to DebugAdapter's default
(always {}), so the Stack Trace sidebar was always empty; now round-trips
through CallSync() like GetThreadList(). WindowsDebugEngine::
GetFramesOfThread() (StackWalk64-based, ported from WindowsNativeAdapter)
already did the actual unwinding, just wasn't wired through the proto
surface.
StepReturn:
- protocol/x2win.fbs gains StepReturnRequest/Response (no fields, mirrors
StepIntoRequest/StepOverRequest's shape).
- X2WinRpcAdapter::StepReturn() was unimplemented (same default-false
fallback), now wired the same way. WindowsDebugEngine::StepReturn()
already existed and uses the newly-wired GetFramesOfThread() internally
(direct C++ call, not a second RPC round trip) to find the caller's
return address and set a temporary breakpoint there.
- SupportFeature() now reports DebugAdapterSupportStepReturn.
Verified end-to-end against a multi-threaded test binary: call stacks
correctly unwind through user code -> CRT startup -> kernel32/ntdll thread
trampolines for every thread, and StepReturn correctly stops at the return
address in the caller rather than single-stepping.
Corresponding stub-side changes (x2win_session.cpp HandleRequest cases for
the two new RPCs) delivered separately via x2winstub/instruction_note/ task
docs, per the BN-core/stub split -- see x2winrpcadapter-task-doc-workflow.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GetStackPointer:
- X2WinRpcAdapter didn't override this, so it fell back to DebugAdapter's
default (always 0). No new RPC needed -- same trick as
GdbMiAdapter::GetStackPointer(): reuse the already-wired ReadRegister()
and read rsp/esp (X2Win only ever targets x86/x64 Windows, so no need for
GdbMiAdapter's fuller architecture-name switch).
GetMemoryMap:
- protocol/x2win.fbs gains MemoryRegionEntry (start/size/name/read/write/
execute/shared -- mirrors BN's DebugMemoryRegion) and
GetMemoryMapRequest/Response.
- X2WinRpcAdapter::GetMemoryMap() fell back to DebugAdapter's default
(always {}), so the Memory Map sidebar was always empty; now round-trips
through CallSync() like GetModuleList(). WindowsDebugEngine::
GetMemoryMap() (ported from WindowsNativeAdapter) already did the actual
region enumeration, just wasn't wired through the proto surface.
Verified end-to-end: SP now shows a real value in the register view instead
of 0, and the Memory Map sidebar populates with the target's regions.
Corresponding stub-side change (x2win_session.cpp's Body_GetMemoryMapRequest
case) delivered separately via x2winstub/instruction_note/ task docs, per
the BN-core/stub split -- see x2winrpcadapter-task-doc-workflow.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…in X2WinRpcAdapter - DisconnectDebugServer(): send QuitRequest and tear down the connection, mirroring the Server-mode counterpart to ConnectToDebugServer. - Detach()/Quit(): only fully TeardownConnection() for target-mode connections; for server-mode, reset session state instead so the underlying socket connection to the stub survives (it can still be reused for a subsequent Launch()/Attach()). - Factor the breakpoint/stop-state clearing out of TeardownConnection() into a shared ResetSessionState(), and extend it to also clear pending (hardware) breakpoints and last-stop/exit-code state.
This monorepo's x2winstub/ mirror had fallen behind the actual X2WinStub
repo checked out on the remote Windows box (10.42.4.10), which is where
it's actually built/run/debugged and carries its own git history. Pulled
the current tracked state of that repo (branch
wire-get-memory-map-and-fix-arg-parsing, 75fec60) into this mirror, i.e.
everything its own .gitignore doesn't exclude (build/, clangdLoc/,
.claude/, .vscode/, instruction_note/) and skipping testBinaries/ (also
untracked there) and vendor/flatbuffers (a real git submodule there,
building standalone; this mirror instead reuses this repo's own
vendor/flatbuffers via the nested add_subdirectory(x2winstub) path, so it
doesn't need its own copy -- see x2winstub/CMakeLists.txt's
`if(NOT TARGET x2win_fbs)` guard).
Covers remote's last several commits, wiring up over RPC: StepInto/
StepOver, BreakInto/RemoveBreakpoint, GetProcessList (+ restricting
Attach to Server mode), registers, WriteMemory, thread management,
hardware breakpoints/watchpoints, TargetExited forwarding,
GetFramesOfThread/GetMemoryMap/StepReturn, and a target-mode
reconnect/--ip/--port argument-parsing fix -- matching the BN-core side
already wired in this repo's own recent commits.
Also pulled over KNOWN_ISSUES.md (untracked on remote, not yet committed
there either) and debug/debug_loop.{cpp,h}.superseded, the pre-port
WinAPI debug loop kept there for reference (superseded by
windows_debug_engine.cpp).
Verified: debuggercore still builds clean locally (x2winstub itself is
Windows-only and can't be built on this machine).
Fix build.md: describe FlatBuffers, not stale Protobuf/Abseil wording. The wire protocol switched from Protobuf to FlatBuffers a while back (see protocol/x2win.fbs, vendor/flatbuffers), but this doc's build instructions never got updated to match -- it still described a two-submodule Protobuf+Abseil setup. Found while sweeping the repo for leftover protobuf references (everything else -- PROTOBUF_PATH, find_package(Protobuf), .proto/.pb.h/.pb.cc, vendor/protobuf submodule entries -- was already clean). Rename x2winstub/KNOWN_ISSUES.md to x2winstub/STATUS.md and expand it: - Add a top-level summary of what X2WinRpcAdapter/x2winstub currently supports and doesn't. - Note that build/run against the remote Windows dev box is confirmed, but passing this repo's Jenkins CI build is not yet confirmed. - Add known issue: X2WinRpcAdapter::Go() never posts a ResumeEventType, so the Binary Ninja UI doesn't reflect the target running until the next stop event arrives. - Drop the --ip/--port known issue (fixed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
|
Go() sent GoRequest and returned whether the stub accepted the resume, but never posted a ResumeEventType DebuggerEvent on success. That event is what DebuggerController::ApplyOwnStateForEvent() uses to flip execution status to DebugAdapterRunningStatus, and what the status bar / widget refresh handlers key off -- without it, the UI kept showing the last stopped state until the next TargetStoppedEvent/TargetExitedEventType arrived, with no indication the target was running in between. GdbAdapter::Go() posts this before sending its resume request; here it's posted after CallSync() returns and only once the stub reports success, so a rejected resume doesn't show "Running" -- at the cost of the UI update lagging the actual resume by one round trip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pulls in Vector35/X2WinStub@2e995e5 (branch wire-get-memory-map-and-fix-arg-parsing), fixing three issues tracked in STATUS.md: - Reset() now fully clears m_breakpoints/m_pendingBreakpoints (and the hardware breakpoint equivalents) instead of only marking entries inactive, so breakpoints from a previous target can no longer carry over to an unrelated process on a reused stub connection. - DebugLoop() drains any other threads' pending debug events before calling DebugActiveProcessStop() on detach, so detaching a multi-threaded target stopped on a breakpoint shared by more than one thread no longer tears the whole process down. - FlushInstructionCache() is now called after every INT3 write/restore (ApplyBreakpoint, RemoveBreakpoint, the temp breakpoint helpers) and after WriteMemory(), so a breakpoint set on an already-running target can no longer silently fail to trigger. STATUS.md updated to mark all three as fixed (plus the ResumeEventType fix from the previous commit), rather than "fix identified, not yet implemented". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pter
- core/adapters/x2winrpcadapter.cpp: X2WinRpcAdapter::ConnectSocket() called
inet_pton(), which isn't declared by the legacy <winsock.h> this codebase
includes on Windows (see core/adapters/socket.h) -- fails to compile with
C3861 "identifier not found". Switch to inet_addr(), matching how every
other adapter in this repo (esrevenadapter.cpp, corelliumadapter.cpp,
gdbadapter.cpp) already converts a string IP into sin_addr.
- ReaderLoop() is the sole thread that reads RPC responses off the socket.
On a TargetStoppedEvent it called ApplyBreakPoints() inline, which can call
AddBreakpoint()/AddHardwareBreakpoint() -> CallSync() for any breakpoint
that was staged in m_pendingBreakpoints/m_pendingHardwareBreakpoints
(module+offset breakpoints not yet resolvable, e.g. right after
Launch/reconnect before the module list is populated). CallSync() blocks
on future.get() until ReaderLoop() reads the matching response -- called
from ReaderLoop() itself, that response can never be read, since this
thread is off in CallSync() instead of back at the top of its read loop.
Reproducible self-deadlock whenever a stop event arrives with a non-empty
pending list.
Fix: dispatch the flush to a separate thread (guarded by
m_applyingBreakpoints so concurrent stop events don't race two flushes),
so ReaderLoop() gets straight back to reading frames -- including the one
that flush is waiting on. Also:
- Added m_pendingBreakpointsMutex: m_pendingBreakpoints/
m_pendingHardwareBreakpoints were previously read/written from
whatever thread calls Add/RemoveBreakpoint() *and* from ReaderLoop()
with no synchronization at all.
- ReaderLoop() now breaks every still-outstanding promise in
m_pendingRequests with an empty envelope before returning, so a
CallSync() (including the newly-detached flush thread's) blocked on a
response that will never arrive because the connection just died
doesn't hang forever either. Existing callers already treat an empty
envelope as a normal rejected/failed call.
Confirmed via test/x2winrpc_test.py: test_module_list, test_register_read_write,
and test_thread_list_suspend_resume all hung indefinitely before this fix
(even run in isolation) and pass cleanly after it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…stub Spawns the real x2winstub.exe as a local subprocess and drives it through DebuggerController exactly like a normal debug session -- no mocking of the adapter or the wire protocol. See the file header for why this lives apart from debugger_test.py's main suite (X2WinRpcAdapter is still unmerged, draft-PR-only work) and the STATUS.md cross-references in individual test docstrings for which regression each one covers. Two assertions were wrong as received and are fixed here, both confirmed by disassembling the actual test binaries rather than assumption: - test_software_breakpoint re-added a breakpoint at the address the target was already stopped at (entry) and asserted go_and_wait() would hit it again immediately. WindowsDebugEngine::Go() correctly steps over a breakpoint sitting at the current IP before resuming (standard debugger semantics -- otherwise `continue` from your own breakpoint could never make progress), and entry executes exactly once, so that breakpoint could never fire a second time. ProcessExited is the correct outcome; only the assertion was wrong. Trimmed to what's actually left to verify once _launch_and_stop_at_entry() already covers add-then-hit: that delete actually takes effect. - test_breakpoint_set_on_running_target_triggers assumed helloworld_loop.exe's *entry point* sits on its own repeatedly-executed loop path (its own docstring flagged this as "not verified by disassembly here"). It isn't: entry is just the one-shot CRT startup thunk that jmp's away and never returns, so a breakpoint there can never retrigger once the target has moved past it, regardless of whether the underlying "write a breakpoint into a running process" mechanism works. Now samples a real in-loop address by breaking into the already-running target once, then resumes and arms the breakpoint on that address while the target is live -- preserving the actual STATUS.md #4 regression scenario. (This one still fails after the fix -- see next steps; the address was the immediate bug, not the only one.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t open issues The address it wrote a breakpoint to was wrong twice over (see the docstring for the disassembly-backed detail): first assumed entry was inside helloworld_loop.exe's repeating loop (it's the one-shot CRT startup thunk), then a "sample the live IP via pause" attempt that also missed -- every thread of the process turns out to sit inside ntdll at pause time, not the target's own code, and writing an INT3 there turned Quit()'s cleanup into a multi-minute stall. Now uses a statically-verified in-module address instead. Still fails after that fix: log capture (binaryninja.log_to_file) shows the SetBreakpointRequest genuinely reaching the stub and getting rejected -- ApplyBreakpoint()'s ReadProcessMemory fails with ERROR_PARTIAL_COPY. Ruled out address hotness, delay length (0.1s-8s), launch vs attach, and target binary as variables; all reproduce the same rejection every time. This contradicts a reported manual GUI repro of the equivalent sequence that doesn't hit it, which needs sorting out before chasing this further -- not something to guess at blind, so debug/windows_debug_engine.cpp was deliberately left unmodified this session per explicit direction. Also flagged test_step_return's still-unconfirmed InternalError-on-second-call hypothesis (StackWalk64 unreliable on asmtest.exe's real-prologue-free functions). Full writeup, what was ruled out, and remaining test coverage gaps: x2winstub/TEST_RESULTS.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Weitao-Sun
marked this pull request as ready for review
September 10, 2026 20:05
Weitao-Sun
changed the title
xusheng6
self-requested a review
September 10, 2026 20:11
New tests: exit codes, exception stops (access violation / divide by zero), StepOver, Restart, conditional-breakpoint condition round-trip, SetActiveThread, module+offset hardware breakpoints, shared-library module-list update, and three negative-path cases (duplicate connect, invalid-pid attach). Real bugs found and fixed: - Attach()/ExecuteWithArgs()/Connect() never posted LaunchFailureEventType on failure, so DebuggerController's optimistic "running" status (set before calling into the adapter) was never corrected back -- a failed attach left dbg.running stuck true forever. Added X2WinRpcAdapter::PostLaunchFailure() and call it from every failure path. - protocol/x2win.fbs's StopReason enum had no values for exception-driven stops, so AccessViolation/Calculation/IllegalInstruction all collapsed to UnknownReason on the wire. Added ACCESS_VIOLATION/CALCULATION/ ILLEGAL_INSTRUCTION and wired them through x2win_session.cpp and X2WinRpcAdapter::ReaderLoop()'s reverse mapping. Regenerated x2win_generated.h; debuggercore.dll and x2winstub.exe must ship together now that the wire format changed. - Restart() silently dropped every breakpoint it replayed: DebuggerBreakpoints::Apply() (via CreateDebugAdapter()'s adapter-reuse path) resolves breakpoints against the stub's module list before the restart's own Launch() has run, while the stub is between debuggees -- resolution "succeeds" against the just-terminated process's stale module info, the resulting SetBreakpointRequest is rejected, and nothing re-staged it for a second try. Fixed by re-staging on that rejection too in AddBreakpoint(ModuleNameAndOffset&), same as the existing module-not-yet-resolvable case. Also documented (not fixed, per prior direction not to modify windows_debug_engine.cpp and to keep chasing scoped): a newly-measured ~1-minute cleanup stall specific to test_breakpoint_set_on_running_target_triggers (confirmed unrelated to the Restart fix above), and a separate, apparently-pre-existing pathological slowness in conditional-breakpoint runtime evaluation (ShouldSilentResumeAfterStop() path) discovered while writing test_conditional_breakpoint -- both written up in x2winstub/TEST_RESULTS.md with what's been ruled out so far. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TEST_RESULTS.md was written as a session narrative (session/follow-up framing, repeated "confirmed fixed" phrasing, full investigation trails and log excerpts) -- rewrote it as a concise reference: what's fixed (one paragraph each), what's still broken and why, coverage gaps. Same information, roughly a third the length. STATUS.md #3 said "Fixed locally, not yet committed" -- that fix has been in the repo for multiple commits now; updated. Also noted exception stop-reason reporting (access violation / divide-by-zero / illegal instruction) in the supported-features list. No code changes; also verified no debug scaffolding (temporary logging, stray log files) was left behind from this session's investigation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both STATUS.md and TEST_RESULTS.md previously narrated how issues were found and fixed (which test caught what, which commit fixed it, "this session" / "follow-up session" framing). Neither file needs that to be useful going forward -- rewrote both to state only the current situation: what's supported, what's currently open (symptom + root cause knowledge, no history), and current test pass/fail status. STATUS.md's "Known issues" now lists only issues that are actually still open; previously-fixed ones are simply reflected in the feature coverage list instead of being kept as resolved history entries. No code changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Manually confirmed (same build, same exact address, GUI vs the automated test) that address selection and code path are not what explains the discrepancy -- GUI's breakpoint toggle goes through the identical AddBreakpoint(uint64_t) path when connected. Updated the issue description accordingly; the underlying cause is still unknown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Manually confirmed in Binary Ninja's GUI: asmtest.exe, launch, Resume with no breakpoints -- process runs and hits an access violation, but Resume/Step stop responding afterward (Detach/Kill still work). Scripting the identical sequence does not reproduce it -- the controller correctly reports AccessViolation and dbg.running flips back to false. Cause not identified; recorded as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
StackWalk64 can guess a second frame from whatever the frame-pointer register happens to hold when there's no real unwind info (.pdata) to key off of, rather than failing outright. That guess isn't reliably right or wrong, which let a bogus frames[1].m_pc through undetected. GetReturnAddress()'s fallback had the same issue: it trusted *RSP unconditionally, which is only actually the return address at the exact instant a function is entered, before its prologue runs. Add IsPlausibleReturnAddress() (inside a known module, immediately preceded by a call instruction whose length lands exactly on it) and use it to validate the StackWalk64 result before trusting it, and to drive a stack scan in GetReturnAddress() instead of a single blind read. Also flush stderr after each log line -- MSVC's CRT buffers stderr once it's redirected to a file/pipe (unlike glibc), so a warning/error could sit unseen until the process exited cleanly. Rewrite STATUS.md/TEST_RESULTS.md to match current, verified behavior: 26/27 automated tests pass. test_breakpoint_set_on_running_target_triggers is excluded from automation -- confirmed by manual testing that the bug is in the test script, not the stub. test_step_return still fails, but now for a test-script reason (missing a step_into before its second step_return_and_wait() call) rather than an engine bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
.cpp.superseded/.h.superseded were never part of the build (CMakeLists.txt lists sources explicitly; windows_debug_engine.cpp replaced this design already) and existed only for historical reference. The reference is kept in comments elsewhere (main.cpp, CMakeLists.txt) that explain design decisions relative to the old approach. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
X2WinRpcAdapter(BN-core) +x2winstub(remote Windows stub), talking overa FlatBuffers RPC protocol, for debugging Windows targets on a separate box.
What's here
Supported: Server-mode and Target-mode connect, launch/attach/detach/quit,
full execution control (go/step into/over/return/break-into), software +
hardware breakpoints (including setting a breakpoint on an already-running
target), memory read/write + memory map, register read/write, thread
list/switch/suspend/resume, module list, call stack, target arch.
Not supported: reverse step-over, Time Travel Debugging (TTD).
Test status
26/27 automated tests pass (
test/x2winrpc_test.py). See TEST_RESULTS.md fordetail on the one failing test (a bug in the test script itself, not the
adapter/stub) and coverage gaps.
Known issues (see STATUS.md for details)
(
DebuggerController::ShouldSilentResumeAfterStop()), not X2Win-specific.StepReturn()can pick an unrelated return address if called while thethread isn't actually inside a called function's body, on code with no
real function prologues for
StackWalk64to unwind.Restart()can race a caller that resumesimmediately.
script, so the adapter/controller-level handling is confirmed correct --
whatever's wrong is specific to the interactive GUI path.
Build status
Builds/runs against the remote Windows dev box. Not yet confirmed to pass
this repo's Jenkins CI build.
🤖 Generated with Claude Code