Skip to content

fix(cdb): confirm remote thread context before automatic triage - #127

Closed
xiaozhu1337 wants to merge 2 commits into
svnscha:developfrom
xiaozhu1337:fix/remote-startup-context
Closed

xiaozhu1337 wants to merge 2 commits into
svnscha:developfrom
xiaozhu1337:fix/remote-startup-context

Conversation

@xiaozhu1337

Copy link
Copy Markdown

Summary

Remote startup currently accepts a bare .echo handshake as readiness for !peb. The server may already have resumed its target; the historical transcript shows ?:???>, no current process/thread warnings, then a successful marker. The first !peb later runs after break-in, but its open already failed.

  • Preserve the existing open-and-triage API: request break-in and confirm current registers before automatic remote triage.
  • Use one startup deadline for connection and context probes. Startup probes reuse the marker wait without normal-command timeout recovery's additional 3–10 second wait.
  • Clean up the client on failed startup; preserve the original exception.
  • Document the intentional pause, shared-server ownership and the fact that client cleanup/close does not promise to resume the server's target. Never blindly send g on failure.

Verification

python -m pytest src/mcp_windbg/tests/test_remote_startup.py -q: 7 passed. Fake process tests cover running and stopped targets, missing context, break failure, empty register output, never-replying handshake and a missing probe marker. No native debugger or live remote server was launched. These tests do not verify Windows signal forwarding or every architecture's real register format; native validation should be supervised.

Scope

Related to #125; the undisclosed-session rollback is tracked independently in #126. This fixes a source-level readiness error and does not claim to reproduce or eliminate the observed NPFS/IRP kernel-pool exhaustion.

@svnscha

svnscha commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Thank you for the investigation behind this one. The underlying problem is real and I want it fixed, but I cannot take this implementation, because when I tested it against a real debug server it does not fix the case in its own title and it regresses another one.

It does not fix the running-target case

I built the scenario the PR describes: a real cdb debug server over a named pipe, started -c "g" so the target is genuinely running, with waitfor.exe as the target.

develop:  "Debugger initialization timed out" after the full timeout
this PR:  "Debugger initialization timed out" after the full timeout

With a truly running target the .echo handshake itself never completes, and this branch requests the break-in only after that handshake returns. So it never reaches the code that would help. Sending CTRL+BREAK during the handshake does work: on unmodified develop the session then opened in about 2 seconds and !peb and r both worked. That is where a fix has to go.

And it regresses the no-context case

A target with no current process or thread, which develop tolerates, now fails the open after the full timeout and kills the client. That is the ?:???> state from your own transcript, so the configuration that motivated the report is the one that breaks.

The register probe passes by accident

_REGISTER_CONTEXT = re.compile(r"^\s*(?:eax|rax|r0|x0|pc|ip|eip|rip|iip)\s*=\s*[0-9a-f]+", re.I)

The first line of real r output is prompt-prefixed, because the prompt carries no trailing newline. Captured from a real remote client:

'0:000> rax=000000000000005b rbx=0000000000000003 rcx=0000000000000003\n'
'rdx=000000087fafec48 rsi=0000000000000000 rdi=0000000000000003\n'

So rax, eax, r0 and x0 never match, and the probe only succeeds because a later line happens to start with rip=. On x86 it would rely on eip= and on arm64 on pc=, neither of which I could verify. Any truncated or filtered r makes the open fail after the full timeout. The fake in the tests emits an unprefixed rax= line, which is why this is invisible to the suite.

Where this leaves it

Two design points I want to settle before any readiness fix lands, and I would value your view on both:

  1. Break-in on connect should be opt-in, not the default. This branch sends CTRL+BREAK on every remote open, including to a stopped target, with no parameter and nothing in the tool description or the open result to say so. On a shared debug server that silently stops someone else's process. Connecting should return the session id with a "target running, not yet analysed" advisory, and stopping it should be an explicit choice.
  2. Connect-then-fail must not strand a target. As it stands, a failed or closed open can leave a shared server's target stopped until a human types g.

I am deferring this to 1.6 rather than rushing it into 1.5, because testing it properly needs a live running remote target and there is currently no automated coverage of that path at all. Every live scenario in the suite opens a stopped target, which is exactly why both this and the consolidated branch shipped a defect there. Adding that scenario is part of the 1.6 work.

Two things from this PR I am keeping: your correction that closing a client does not promise to resume the server's target is accurate, I measured it in both trees, and it will land as a docs change on its own. The "opening stops the target" wording will not, since it is not true for running targets.

The readiness half of #125 stays open to track this. Closing the PR, not the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants