feat: exception events as values, and the crash stack without .ecxr - #144
Conversation
Three typed reads that `windbg-mcp`'s explorer walkthrough had to do by hand through the `execute` text hatch, three times in one evening: `.exr -1`, `.ecxr`, and the stack walk that follows one. `last_event` returns a `DebugEvent` — kind, engine process and thread, and an `ExceptionRecord` when the event carried one. It is the user-mode counterpart to `bug_check`. `parameters` is cut to the record's own `NumberParameters` and clamped to the fifteen slots there are, because that count is the field the two shapes of a `0xc0000409` are told apart by: one parameter is the CRT's `abort`, three is WIL's, whose second is the HRESULT. A leftover read as a parameter would answer the question rather than merely look untidy. `stored_event` returns the event a dump was written for, with the register context it was written with. Unlike `last_event` it does not move, so it still answers on a session somebody has already been working in. `Ok(None)` where there is no stored event, read off the engine's own refusal rather than probed for. `stack_frames_from` walks that context — `.ecxr; k` without the `.ecxr`, so the caller's selected thread and frame are left where they were. Measured in `examples/stored_event_probe.rs`, which caught the one that would otherwise have shipped: unlike `GetScope`, `GetStoredEventInformation` does not refuse a context buffer that is too small. It truncates and returns success — offered 716 bytes for an x64 dump it writes 716 and reports 716 — and the damage surfaces three calls later, when `GetContextStackTrace` rejects the result with `E_INVALIDARG` against a caller that did nothing wrong. So this call gets a ladder of its own that starts above every real `CONTEXT` instead of below it. Two negative results are recorded there too, because both narrow the claim. A kernel crash dump has no stored event at all, so this and `bug_check` do not overlap and neither is a fallback for the other. And what makes the two stack walks differ is the selected thread and only that: after `~1s` on a two-thread fail-fast dump, `stack_frames` returns the parked thread's six frames while `stack_frames_from` still returns the crash's twelve — while `.frame`, `.cxr` and `.ecxr` move neither, since they change the symbol scope and `GetStackTrace` walks from the thread's registers. A single-threaded dump agrees with itself in every state it can be put in, which nearly recorded "no difference" as the answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8edd418b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(DebugEvent { | ||
| kind, |
There was a problem hiding this comment.
When a dump has been passed to open_dump but has not yet been pumped with wait_for_event, GetLastEventInformation succeeds with kind 0 and DEBUG_ANY_ID identifiers, as the new stored_event_probe itself records. This unconditional construction therefore returns a seemingly valid DebugEvent, contrary to this method's guarantee that an engine before its first wait fails; callers can mistake “target not loaded yet” for an actual future/unknown event kind. Detect this sentinel and return an error instead.
Useful? React with 👍 / 👎.
| Some(context.as_bytes().as_ptr().cast()), | ||
| context.len() as u32, |
There was a problem hiding this comment.
If a caller retains this context, ends the session, opens another same-architecture target, and then calls stack_frames_from, the method passes the old register bytes directly to the new target. Those register and stack addresses are target-specific, so this can unwind or symbolize unrelated memory instead of returning the crash stack; unlike Scope, ThreadContext carries no target identity to detect the misuse. Store the originating target identity with the context and reject it when it no longer matches before calling GetContextStackTrace.
Useful? React with 👍 / 👎.
Both findings from Codex's review of dbgscope#144, and both were right about the fact. The remedy differs from the one suggested in the first case. **"No event" was claimed to be a failure and is not.** `last_event`'s doc said the call fails on an engine that has seen no event. Measured -- on an engine holding no target, and on a dump `open_dump` has named but nothing has pumped -- it succeeds: `S_OK`, kind `0`, `DEBUG_ANY_ID` for both ids. So the call was returning a `DebugEvent` whose `kind` is not a `DEBUG_EVENT_*` value at all, and a caller could read "the target is not loaded yet" as an event kind this build does not recognise. Codex proposed detecting the sentinel and erroring. It is `Ok(None)` instead, which is this crate's shape for exactly this -- `bug_check` and `stored_event` both use it -- and which an error would lose: "nothing has happened yet" and "the call failed" are different answers, and only one of them is a reason to stop. Two new probe arms, `empty` and `unwaited`, are where that was measured. The wrong sentence came from `last_event_process`, which has carried it since it was written, so it is corrected there too. It costs that call's one caller nothing -- `record_where_it_stopped` looks the id up in the session listing and `DEBUG_ANY_ID` is in no listing -- but a caller that believed the doc would be joining on a sentinel. **A `ThreadContext` now carries the target it was read from**, and `stack_frames_from` refuses one from a target the engine no longer holds, as `set_scope` already refuses a stale `Scope`. That precedent is the argument: it is the same saved register state with the same hazard, and this is the sharper half of it. A stale scope points the session somewhere visibly wrong; a stale context comes back as *frames*, which is an answer a caller cannot tell from the right one. Both new rules are mutation-verified: removing the kind-zero check fails the test written for it, and so does removing the identity check. The first was initially only visible through the probe, so it has a unit test now -- an engine holding nothing reaches that state without a file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 201d5429e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The check is cheap and the failure it prevents is not detectable afterwards: a context | ||
| // from a released target unwinds whatever its addresses reach in the new one and comes | ||
| // back as a plausible stack. | ||
| if context.target != self.target_identity() { |
There was a problem hiding this comment.
Although the follow-up now protects the end_session path, this comparison still accepts a stale context when a caller loads dump A, saves its context, and then replaces it with dump B through open_dump (or either kernel-attach opener): those paths call forget_the_previous_session, which explicitly treats the old session as replaced, but never call reissue_identity. The fresh evidence beyond the earlier comment is this repository-controlled replacement path, where context.target therefore remains equal and the old registers can again be unwound against the new target; reissue the client identity as part of successful session replacement.
Useful? React with 👍 / 👎.
Round two of Codex's review on dbgscope#144, and it is right: the identity check the previous commit added was worth nothing on the path that matters most. Only `end_session` reissued the target identity. The three openers that replace a session -- `open_dump`, which `open_trace` delegates to, and both kernel attaches -- call `forget_the_previous_session`, which says in its own name and doc that the old session is gone, and then left the identity alone. So a caller who opened dump A, saved a `Scope` or a `ThreadContext`, and opened dump B through the same engine got the same identity back, and the stale registers were accepted against the new target. The reissue goes in `forget_the_previous_session` rather than in the three openers. Codex proposed the openers; that would be a fourth thing for the next opener to remember, and the function that already means "this session is being replaced" is where the invalidation belongs. `test_an_opener_that_replaces_the_session_invalidates_the_old_contexts` pins it, and **writes its own dump** with `.dump /m` on a launched target rather than wanting a fixture -- which is what makes it a test rather than a probe. Backing the reissue out fails it on the assertion written for it. Two things that came out of adding it, both mine. The two engine-building tests from the previous commit took no `one_debuggee()` guard, because neither opens a target and the guard reads as being about targets. It is not: `DebugEngine::drop` ends the process's debuggee session, so an engine built and dropped beside another test's live target ends *that* target's session. It surfaced three tests away as `test_an_abandoned_launch_does_not_hand_its_process_to_the_next_one` failing a pump with `E_UNEXPECTED`, with nothing pointing back here, and only under the default harness -- single-threaded the suite was green, which is the wrong thing to conclude from. Both take the guard now; three consecutive parallel runs are clean. And neither carried `#[cfg(not(miri))]`, which every other engine test here does. Miri does not run on pull requests in this repo, so that would have been green here and red on the merge to `main` -- the exact gap this repo's own notes warn about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Three typed reads that
windbg-mcp's explorer walkthrough had to perform by hand through theexecutetext hatch, three times in one evening —.exr -1,.ecxr, and the stack walk that follows one. Its §9 makes the case for them; this is the primitive half.What lands
DebugEngine::last_event() -> DebugEvent— kind, engine process and thread, and anExceptionRecord(code, flags, faulting address, parameters, nested pointer) when the event carried one. The user-mode counterpart tobug_check.parametersarrives cut to the record's ownNumberParameters, and clamped to the fifteen slots there are. That count is the field the two shapes of a0xc0000409are told apart by — one parameter is the CRT'sabort, three is WIL's, whose second is theHRESULT— so a leftover read as a parameter would answer the question rather than merely look untidy.DebugEngine::stored_event() -> Option<DebugEvent>— the event a dump was written for, with the register context it was written with, as an opaqueThreadContext. Unlikelast_eventit does not move, so it still answers on a session somebody has already been working in.Ok(None)where there is no stored event, read off the engine's own refusal (E_UNEXPECTED) rather than probed for, so a genuine failure still reaches the caller as one.DebugEngine::stack_frames_from(&ThreadContext, max)—.ecxr; kwithout the.ecxr: the caller's selected thread and frame are left where they were, so a triage built on it is still a read of the target.The measurement that changed the design
examples/stored_event_probe.rscaught the bug that would otherwise have shipped.GetScoperefuses a context buffer below the target'sCONTEXTsize — which is whySCOPE_CONTEXT_SIZESclimbs from the smallest.GetStoredEventInformationdoes not: offered 716 bytes for an x64 dump it writes 716, reports 716, and returns success. The damage surfaces three calls later, whenGetContextStackTracerejects the truncated context withE_INVALIDARGagainst a caller that did nothing wrong.So this call gets a ladder of its own that starts above every real
CONTEXTand grows only on the one signal the call gives that there was more to write. Offered 4,096 the same dump reports 1,232.Two negative results, because both narrow the claim
STATUS_BREAKPOINTwith zero parameters, innt!KeBugCheck2. So this andbug_checkdo not overlap and neither is a fallback for the other.~1son a two-thread fail-fast dump,stack_framesreturns the parked thread's six frames whilestack_frames_fromstill returns the crash's twelve..frame,.cxrand.ecxrmove neither — they change the symbol scope, andGetStackTracewalks from the thread's registers. A single-threaded dump agrees with itself in every state it can be put in, which nearly recorded "no difference" as the answer.Verification
cargo test --lib: 192 passed, up from 189 — three new unit tests, over synthetic records rather than an engine, so they run under Miri.used < SIZEcheck and dropping the parameter clamp each fail the test written for them.cargo fmt --all -- --checkclean;cargo clippy --all-targetsunchanged at 10 warnings, none in this diff.examples/stored_event_probe.rsrun on all three target shapes: a livecmd.exe, a kernel crash dump, and user-mode fail-fast dumps (one thread and two).E_UNEXPECTEDwas pinned by flippingno_stored_eventto|_| falsefor one run and back for the next.Miri does not run on PRs here; nothing in this diff adds an
unsafeshape that Miri would have something new to say about (oneread_unalignedout of a#[repr(C, align(8))]byte buffer), but it is worth aworkflow_dispatchbefore merge if you would rather.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf