Typed disassembly: operands, flow, and the unwind region - #145
Conversation
… function `Instruction` gains `mnemonic`, `operands` and `flow`. The engine has no structured disassembly, so the third column was a string and every caller wanting an immediate or a branch target re-parsed it downstream — twice over, in windbg-mcp's case, once for the walk and once for the recipe. The parse belongs at the seam that produces the rendering. Two rules the shapes are chosen for. An operand this does not recognise is `Other` with its text, never forced into one of the other four. And every destination in `Flow` is an `Option`, because "the engine printed a resolvable address" and "this is indirect" are different facts; a caller reading `None` as no edge stays sound. Gated on `InstructionSet`. x86 and x64 are read; ARM64 reports its mnemonic and `Flow::Unknown`, not the `Fallthrough` most instructions happen to be — an unread `b.eq` called a fall-through hands a walk one edge of two. Measured rather than composed. `examples/typed_disassembly.rs` runs the reading over a whole real dispatch routine: 376 instructions of `mountmgr!MountMgrDeviceControl` on a 26100 image, zero unrecognised operands, zero unknown flows, and its eleven control-code compares recovered as values. It found both defects now pinned by tests — literals must be `u64`, since that routine renders `8000000000000000h` and `0FFFFFFFFFFFFFFFFh`, and registers must be matched before literals, since `ah`, `bh`, `ch` and `dh` are both. `function_extent` reads the `.pdata` entry for an address. It is deliberately named a region: MSVC splits a function across several, and this answers `0x14750..0x147a3` for that routine — 83 bytes, which `.fnent` confirms — while its compare chain lives past `0x147dd`. Bounding a walk with it recovered zero control codes where following the flow recovered twelve. The x64 entry is three `u32` RVAs rather than the 64-bit addresses the API's name suggests, and reading them as `u64` reports no entry for a function that plainly has one. `symbol_for` exposes the symbol lookup that was already there privately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced 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: 1f67db3528
ℹ️ 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".
| /// They are also relative, so they are rebased here against the module that holds the address | ||
| /// — what `.fnent` prints, at the RVAs it prints them. | ||
| pub fn function_extent(&self, address: u64) -> Result<Option<(u64, u64)>, DbgEngError> { | ||
| let mut entry = [0u32; 3]; |
There was a problem hiding this comment.
When function_extent is called on an ARM64 target—which the crate advertises and the new example permits—the function entry is not three u32 RVAs: its second word is packed unwind data or an .xdata RVA, not an end address. Interpreting entry[1] as EndAddress can therefore return a huge bogus region or None for valid ARM64 code; branch on the processor type and decode its function length, or explicitly reject non-x64 targets.
Useful? React with 👍 / 👎.
| "jmp" | "jmpf" => Flow::Jmp(destination()), | ||
| "ret" | "retf" | "retn" | "iret" | "iretd" | "iretq" | "sysret" | "sysexit" => Flow::Return, | ||
| "ud0" | "ud1" | "ud2" | "hlt" => Flow::Trap, | ||
| "int" | "int1" | "int3" | "into" => Flow::Trap, |
There was a problem hiding this comment.
On 32-bit Windows code that issues a returning software interrupt, notably the int 2eh system-call path, classifying every int as Trap makes falls_through() false and causes a reachability walk to discard all instructions after the syscall. Only non-returning trap forms such as int 29h and int3 should stop the walk; classify int using its operand rather than its mnemonic alone.
Useful? React with 👍 / 👎.
| .is_err() | ||
| { | ||
| // No entry for this address is the ordinary answer for a leaf or for data. | ||
| return Ok(None); |
There was a problem hiding this comment.
If GetFunctionEntryByOffset fails because the debugger is in an invalid state, the target cannot be read, or another COM error occurs, this branch converts it to Ok(None), making a failed query indistinguishable from a leaf function with no unwind entry despite the Result<Option<_>> return type. Map only the documented no-entry result to None and return other failures with context.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
…ring All three findings on #145 were correct. Two of them turned on facts the API's documentation does not state, so `examples/function_entry_probe.rs` measures them rather than arguing: which failure means "no entry", and what the record looks like off x64. `function_extent` answers a three-state `FunctionExtent` instead of an `Option`, because the two non-answers were different facts collapsed into one. `NoEntry` is now reported for the single measured failure that means it -- `E_NOINTERFACE`, which a real dbgeng 10.x gives for address zero, for a module's header page, and for every x86 address, 32-bit Windows having no unwind table -- and every other failure is returned as an error rather than read as a leaf. `Unsupported` is every instruction set but x64, and refusing rather than decoding is the choice here. ARM64's record is two words whose second is packed unwind data or an `.xdata` RVA: measured on an ARM64 kernel dump, `nt!KeBugCheckEx` fills `needed = 8` with `[0x0025df60, 0x0005f218]`. Read as an end address that is a bogus region, and for any function whose `BeginAddress` is below the `.xdata` RVA it is a bogus region that contains the address asked about, so it passes every sanity check in the function. A wrong region that looks right is worse than no region. Decoding ARM64's packed unwind length is the other option the finding offered and buys nothing yet: the operand reading already refuses that architecture, so nothing downstream could use the bound. The engine's own `needed` is now checked against the x64 shape as well, so the layout is a check rather than an assumption. A software interrupt is classified by its vector rather than its mnemonic. `int 2eh` is the 32-bit system-call path and returns; classifying every `int` as a trap made `falls_through()` false and discarded every instruction after a syscall. `int 29h` and `int 3` still stop a walk -- the second by vector as well as by the `int3` spelling, since that is how the engine renders `0xcc`. Mutation-verified: restoring the blanket `int` trap fails the new vector test and nothing else. The two engine-dependent fixes are verified by the probe against a real ARM64 dump and a real x64 one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e01a4716a
ℹ️ 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".
| match self.processor_type() { | ||
| Ok(machine) => InstructionSet::from_processor_type(machine), |
There was a problem hiding this comment.
When the debugger's effective machine differs from the physical target—such as a WOW64 session, emulated x64 code on ARM64, or after .effmach—Disassemble renders the effective instruction set, while processor_type() calls GetActualProcessorType. This can expose the wrong InstructionSet, suppress otherwise supported operand parsing, and make function_extent select the wrong unwind-entry layout. Query GetEffectiveProcessorType for the rendering instead, and avoid using the physical processor as the code-layout discriminator.
Useful? React with 👍 / 👎.
| "fword" => 6, | ||
| "qword" => 8, | ||
| "tbyte" => 10, | ||
| "mmword" | "xmmword" => 16, |
There was a problem hiding this comment.
When DbgEng renders an MMX memory operand such as mmword ptr [rax], this branch reports size: Some(16), but an MMWORD is 64 bits (8 bytes); XMMWORD is the 16-byte form. Any consumer using the typed operand to determine access width will therefore double the size of MMX accesses, so mmword and xmmword need separate mappings.
Useful? React with 👍 / 👎.
Both findings on the second round were correct. `instruction_set` read `GetActualProcessorType`, which is the machine, while `Disassemble` renders with the *effective* one. They diverge wherever one machine runs another's code -- a WOW64 process, x64 emulated on ARM64, or any target after `.effmach` -- and discriminating a rendering by the machine underneath it reads x86 output with x64 rules and picks the wrong unwind record shape in `function_extent`. No fixture here has that divergence, so it was forced rather than left argued: `.effmach x86` on an x64 kernel dump moves the effective type to `0x14c` with the physical still `0x8664`, and the reading now follows it -- `nt!KeBugCheckEx` answers "not decoded for X86" instead of decoding an x64 entry against x86 output. `effective_processor_type` is a new method rather than a change to `processor_type`, because the pool and heap walkers read that one for pointer width, and a pointer's width is a fact about the machine rather than about a rendering. `mmword` was folded in with `xmmword` at sixteen bytes. An MMX operand is eight; the fold doubled the width reported for every MMX access, with nothing about the rendering to show for it. Every `ptr` width now has a test asserting the width its name means, including the two non-powers of two. Mutation-verified: restoring `mmword` to sixteen fails the new width test and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fda435b46
ℹ️ 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".
| /// against that shape rather than assumed, since it is the field that says ARM64's is | ||
| /// different. | ||
| pub fn function_extent(&self, address: u64) -> Result<FunctionExtent, DbgEngError> { | ||
| let set = self.instruction_set(); |
There was a problem hiding this comment.
When no target is active or GetEffectiveProcessorType otherwise fails, instruction_set() converts the error to Other(0), so this method returns Ok(FunctionExtent::Unsupported(Other(0))) without attempting the entry query. That makes an engine failure indistinguishable from a valid but unsupported architecture despite this method returning Result; fresh evidence in the current tree is that the newly added processor preflight still swallows failures even though GetFunctionEntryByOffset errors now propagate.
AGENTS.md reference: AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
| ',' if depth <= 0 => { | ||
| out.push(&rest[start..index]); | ||
| start = index + 1; |
There was a problem hiding this comment.
When DbgEng renders a call or branch to a C++ template specialization such as module!foo<int,int> (00007ff612345678), the comma is outside brackets and parentheses here, so the single target is split into two operands. The first fragment then has no parenthesized address, causing classify_flowto produceCall(None)orBranch(None)` and making reachability or call-target analysis silently lose the direct edge.
Useful? React with 👍 / 👎.
| _ if is_conditional_branch(mnemonic) => Flow::Branch(destination()), | ||
| _ => Flow::Fallthrough, |
There was a problem hiding this comment.
For binaries using Intel TSX, xbegin has both a fallthrough path when the transaction starts and a relative branch target used when it aborts or cannot start. Falling into the default Flow::Fallthrough arm means a reachability walk never schedules the fallback path, which is commonly where the lock-based implementation lives, so analyses can omit a substantial portion of the routine.
Useful? React with 👍 / 👎.
Three findings on the third round, all correct. A control transfer's operand text is no longer split on commas. It takes exactly one operand -- a far jump renders its segment with a colon -- and a demangled C++ name carries commas of its own, so `call module!std::map<int,int>::insert (…)` was severed at the template argument, leaving a first fragment with no parenthesised address. That reports `Call(None)`, which a reachability walk reads as an indirect call and drops: a direct edge lost with nothing to show for it. Tracking angle-bracket depth is the other way to fix that and is not taken. `operator<<` and `operator<` leave it unbalanced, and an unbalanced opener swallows every later operand into one -- which on a `cmp` would take the immediate with it and lose exactly the control codes this reading exists to recover. Not splitting where there is nothing to split needs no such judgement. The cost is that a comma inside a symbol still severs it on a non-transfer instruction, where no edge is at stake. `xbegin` is a conditional branch. It falls through into the transaction and takes its operand on an abort or a failure to start, and the abort path is usually where the lock-based fallback lives, so classifying it as a plain fall-through misses a whole implementation of the routine. `function_extent` asks the processor directly instead of through `instruction_set`, which folds a failed query into `Other(0)`. That fold is right where a rendering still exists to hand back with its operands unread; here there is no partial answer, so a failed query is an error and `Unsupported` keeps meaning "a real architecture this does not decode". Both sides now say why they differ. Two of these landed on `classify_flow`, which is a hand-maintained table, so its doc now states what a missing entry costs rather than implying the table is complete: the default arm reads an unlisted transfer as a fall-through, which loses an edge and never invents one, so a gap degrades along the "reachable is sound, not-reachable is best-effort" boundary a walk over this already documents. Decoding ARM64's packed unwind length is now tracked as #146 and cited from the code. Mutation-verified one at a time: always splitting fails the decorated-symbol test and nothing else, and removing the `xbegin` arm fails its test and nothing else. The real-target measurement is unchanged at 376 instructions, zero unread operands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de4a0db96d
ℹ️ 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".
| if set != InstructionSet::Amd64 { | ||
| return Ok(FunctionExtent::Unsupported(set)); |
There was a problem hiding this comment.
When the effective target is x86, this blanket branch returns Unsupported(X86), but the new public FunctionExtent::NoEntry documentation explicitly promises that every x86 address produces NoEntry because 32-bit Windows has no unwind table. Callers that distinguish an absent entry from an unsupported layout therefore receive the wrong state for every x86 input; special-case X86 as NoEntry while retaining Unsupported for architectures whose entry layout actually needs decoding.
Useful? React with 👍 / 👎.
| let (name, address) = match text.split_once('(') { | ||
| Some((name, tail)) => (name.trim(), tail.trim_end_matches(')').trim().to_string()), | ||
| None => (text.trim(), String::new()), |
There was a problem hiding this comment.
When DbgEng resolves a control transfer to a symbol whose name itself contains parentheses, such as module!Functor::operator() (00007ff612345678), split_once('(')splits at the operator name rather than the appended address. The address parse then fails, the symbol is truncated, and the instruction becomesCall(None)orBranch(None)`, dropping the direct edge. The current comma fix leaves this decorated-symbol form exposed; split from the final address suffix instead.
Useful? React with 👍 / 👎.
| "xbegin" => Flow::Branch(destination()), | ||
| _ if is_conditional_branch(mnemonic) => Flow::Branch(destination()), | ||
| _ => Flow::Fallthrough, |
There was a problem hiding this comment.
Whenever a reachability walk encounters xabort, the default arm classifies it as Fallthrough. Inside an RTM transaction it aborts and resumes at the fallback specified by the corresponding xbegin, while outside a transaction it faults, so execution never continues at the next sequential instruction. This classification causes callers to mark dead instructions after an abort as reachable; represent xabort with a non-fallthrough flow.
Useful? React with 👍 / 👎.
| if let Some(target) = instruction.flow.target() { | ||
| // A call leaves this function; every other edge stays in it. | ||
| if !matches!(instruction.flow, Flow::Call(_)) { | ||
| queue.push(target); | ||
| } |
There was a problem hiding this comment.
When the selected routine ends with a tail jmp to another function in the same module, the module bounds accept the destination and this branch enqueues it because every non-call target is assumed to remain in the function. The probe then walks that callee and potentially its transitive tail-call graph, so its instruction counts, unread-operand totals, IOCTL candidates, and call list no longer describe the requested routine and may eventually hit the 20,000-address cap. Use symbol/function ownership to reject cross-function jump targets rather than relying only on module bounds.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
… costs Round four. Three taken, one declined. A destination is split from the **last** parenthesis, and only when what follows it parses as an address. `module!Functor::operator()` is a demangled call operator, and splitting at the first parenthesis took the name apart at `operator`, left `) (…` to parse as an address, and reported `Call(None)` -- the same direct edge the comma case lost, reached through a different character. Requiring the tail to parse is what keeps a symbol whose last parenthesis is its own intact. `function_extent`'s doc promised `NoEntry` for x86 while the gate has always answered `Unsupported`. Both readings are defensible and only one can be true, so the doc moves rather than the code: 32-bit Windows has no unwind table and every x86 address measured here answers `E_NOINTERFACE`, but that is a fact about a platform, and returning `NoEntry` would assert it about an address this never asked about. x86's record is also `IMAGE_FUNCTION_ENTRY`, whose fields are addresses rather than the RVAs rebased here, and nothing has measured one. One rule everywhere: decode the layout that has been measured, refuse the rest by name. The probe no longer follows a tail `jmp` out of the routine it was asked about. A neighbour is inside the module, so module bounds alone let the walk wander into that function and its own tail calls, and every number the probe prints would then describe more than the routine. Edges are kept inside the entry's symbol, with the module bound as the fallback where there is no symbol, and the run says which. Measured on `mountmgr!MountMgrDeviceControl`: zero cross-function jumps declined, 376 instructions and eleven control-code compares unchanged -- so the figures quoted for this branch were already describing the right routine. **Declined: `xabort` as non-fallthrough.** The finding's premise is that execution never continues at the next instruction, and outside a transaction that is not so -- the SDM specifies `IF RTM_ACTIVE = 0 THEN treat as NOP`, so it falls straight through. Inside one it resumes at the outer `xbegin`'s fallback, and no static reading tells the two apart. Classifying it as a transfer would drop every instruction after it wherever RTM is inactive, which on current parts is nearly everywhere. Listed explicitly as a fall-through so the decision is visible rather than left to the default arm. That finding did show up a sentence of mine that was wrong. `classify_flow`'s doc claimed an unlisted mnemonic "never invents an edge", which holds for a missing *conditional* transfer and not for an unconditional one, whose fall-through does not exist. The doc now separates the two and states that the table is complete for the second kind. Mutation-verified: splitting from the first parenthesis fails the new parentheses test and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5810f9f573
ℹ️ 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".
| matches!( | ||
| self, | ||
| Self::Fallthrough | Self::Call(_) | Self::Branch(_) | Self::Unknown | ||
| ) |
There was a problem hiding this comment.
When DbgEng renders ??? because code pages are unavailable, read_operation assigns Flow::Unknown, and this match makes falls_through() return true. The new typed_disassembly reachability walk therefore queues subsequent addresses through bytes it never decoded—potentially until its 20,000-address cap—rather than stopping at the unreadable region. Distinguish unreadable renderings from unsupported instruction sets so only the latter receive the intentional best-effort fallthrough.
Useful? React with 👍 / 👎.
| if text.contains('[') { | ||
| if let Some(memory) = read_memory_operand(text) { | ||
| return Operand::Memory(memory); |
There was a problem hiding this comment.
When a direct call or jump resolves to a C++ overload such as module!Vector::operator[] (00007ff612345678), this test treats the brackets in the symbol name as a memory expression before the !-based target case runs. read_memory_operandconsequently returnsMemory, classify_flowproducesCall(None)orJmp(None)`, and reachability loses the resolvable direct edge. Detect actual memory-expression brackets without consuming brackets that are part of a decorated symbol.
Useful? React with 👍 / 👎.
| "call" | "callf" => Flow::Call(destination()), | ||
| "jmp" | "jmpf" => Flow::Jmp(destination()), | ||
| "ret" | "retf" | "retn" | "iret" | "iretd" | "iretq" | "sysret" | "sysexit" => Flow::Return, | ||
| "ud0" | "ud1" | "ud2" | "hlt" => Flow::Trap, |
There was a problem hiding this comment.
For kernel or firmware code containing hlt, execution resumes at the following instruction after an enabled interrupt or NMI wakes the processor. Grouping hlt with undefined-instruction traps makes falls_through() false, so a reachability walk truncates common idle-loop or wait-path code immediately after the halt. Classify hlt separately with a fallthrough edge.
Useful? React with 👍 / 👎.
Five review rounds said this. Three of them found the same defect through a different character -- a comma inside `std::map<int,int>`, a parenthesis inside `operator()`, a bracket inside `operator[]` -- each one severing a direct call edge that a walk then dropped as indirect, each fix locally correct, the next character already waiting. Four rounds added an entry to the mnemonic table: `int` by vector, `xbegin`, `xabort`, `hlt`. Neither list was ever going to end, because both were consequences of one choice: recovering structure from another program's rendering. So the rendering stops being the source. `mnemonic`, `operands` and `flow` now come from decoding `bytes` -- the engine's own read of the instruction, already in hand, so no extra round trip -- with `iced-x86`, default features off, `decoder` and `instr_info` only. No formatter: the engine's rendering is what this crate promises and it stays verbatim in `text`. What that deletes, rather than fixes. Symbols leave the picture: a destination is an address and naming it is `symbol_for`'s job, so no symbol's spelling can take an operand apart again -- `Operand::Target` and `MemoryOperand` lose their symbol halves. The mnemonic table is gone; flow control comes from the decoder and is complete by construction, leaving two decisions that are about semantics rather than spelling (a software interrupt's vector, and `xabort`), both keyed on `Code` and both carrying the reasoning that was established under review. Roughly 300 lines of parser and every special case in it go with them. `decode_range` is the other half: one memory read for a whole span instead of one engine call per instruction, which is what a bounded traversal over a hundred functions needs. Those instructions carry no `text`, nothing having rendered them, and the doc says to ask `disassemble` for the few a caller displays rather than filling the field from a second formatter -- two renderings of one instruction in one type is worse than none. `Flow::Unreadable` splits off `Unknown`, which round five caught. An instruction set this does not decode still has an instruction there and falls through; a `???` rendering has none, and a walk that fell through one would step through bytes, one address at a time, to its own cap. Checked before the instruction set, because unreadable bytes are unreadable on every architecture. `hlt` falls through, also from round five, and now for free: the decoder calls it `Next`, which is right -- a halted processor resumes when an interrupt wakes it, and grouping it with the undefined-instruction traps truncated every idle loop at the halt. Behaviour-preserving where it counts, measured rather than asserted. On `mountmgr!MountMgrDeviceControl`: 376 instructions, zero unreadable operands, zero unknown flows, the same eleven control-code compares at the same addresses, the same forty calls with the same import names -- identical before and after. The example now also compares the two decode paths over one region: 23 range-decoded, 23 compared, 0 disagreements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66eb5edbbf
ℹ️ 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".
| if decoded.is_invalid() { | ||
| return (String::new(), Vec::new(), Flow::Unreadable); |
There was a problem hiding this comment.
When DbgEng successfully renders a valid x86 instruction that iced-x86 1.21 does not recognize, such as an instruction from a newer extension, this path labels it Unreadable even though its bytes and rendering were read successfully. Since Unreadable::falls_through() is false, reachability walks stop and discard the remainder of the routine; reserve Unreadable for missing bytes/??? and represent a decoder-version disagreement as Unknown.
Useful? React with 👍 / 👎.
| } else if base == iced_x86::Register::None && index == iced_x86::Register::None { | ||
| Some(decoded.memory_displacement64()) |
There was a problem hiding this comment.
For an operand such as the documented kernel access gs:[188h], both base and index are None, so this branch reports address: Some(0x188). The actual linear address depends on the runtime GS segment base and therefore cannot be computed from the instruction alone; consumers may consequently read or symbolize an invalid low address. Require the absence of an effective segment override before exposing a static address.
Useful? React with 👍 / 👎.
| base: (base != iced_x86::Register::None).then(|| register_name(base)), | ||
| index: (index != iced_x86::Register::None).then(|| register_name(index)), | ||
| scale: decoded.memory_index_scale() as u8, | ||
| displacement: decoded.memory_displacement64() as i64, |
There was a problem hiding this comment.
For RIP/EIP-relative operands, iced-x86 exposes the normalized absolute target through memory_displacement64(), so assigning that value to displacement duplicates address rather than returning the signed encoded displacement promised by this field. For example, call [rip+0xffa] at 0x1000 reports a displacement of 0x2000, breaking consumers that inspect or reconstruct the addressing expression; derive the displacement from the target and the instruction's next IP instead.
Useful? React with 👍 / 👎.
Round six, all three taken, and none of them about symbol punctuation or the mnemonic table -- those stopped existing with the decoder. These are edges in the new code. Bytes that were read and did not decode are `Unknown`, not `Unreadable`. The line between the two is whether there are bytes at all: a `???` rendering has none, and a walk must stop rather than step through them one address at a time; an encoding this decoder does not know -- an extension newer than the pinned version, or a span entered mid-instruction -- has an instruction there, and stopping would discard the rest of a routine over a version skew. The same correction applies in `decode_range`, where a linear read runs into data as a matter of course. A segment override rules out a static address. `gs:[188h]` has no base and no index, so it was reported as address `0x188`; it is the KPCR, its linear address is the segment base plus that displacement, and the base is a runtime fact. A consumer taking the old value would read or symbolise a low address that means nothing. A RIP-relative operand keeps the displacement it encodes. The decoder normalises that displacement to its target, so taking it verbatim reported `[rip+0xffa]` at `0x1000` as `0x2000` -- a second copy of `address`, where the addressing expression should be. Derived back against the instruction's own end, which is what the displacement is relative to. Mutation-verified one at a time: each of the three, reverted, fails exactly one test and no other. Re-measured on `mountmgr!MountMgrDeviceControl` unchanged at 376 instructions, eleven control-code compares, and 23 of 23 instructions agreeing between the two decode paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86649e23a0
ℹ️ 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".
| let displacement = if rip_relative { | ||
| (decoded.ip_rel_memory_address() as i64).wrapping_sub(decoded.next_ip() as i64) | ||
| } else { | ||
| decoded.memory_displacement64() as i64 |
There was a problem hiding this comment.
For x86 addressing forms, iced-x86 preserves the 32-bit effective-address representation in memory_displacement64(), so a negative displacement such as [ebp-8] is returned as 0xfffffff8; this direct i64 cast reports 4294967288 rather than the signed -8 promised by MemoryOperand::displacement. The same issue affects address-size-overridden x64 instructions, so based displacements need sign extension according to their effective address/displacement width.
Useful? React with 👍 / 👎.
| /// The image has no unwind entry covering the address — a leaf function, or an address that | ||
| /// is not code. Every x86 address answers this, 32-bit Windows having no unwind table. | ||
| NoEntry, |
There was a problem hiding this comment.
On every x86 target, function_extent returns Unsupported(X86) before querying an entry, so this public variant documentation still promises an outcome callers can never receive. Fresh evidence in the final tree is that the method-level documentation now explicitly chooses Unsupported for x86 while this enum-level contract still says every x86 address returns NoEntry; callers matching these states can therefore implement the wrong fallback.
Useful? React with 👍 / 👎.
Round seven, both taken. The decoder keeps a narrow effective address in its own width, so `[ebp-8]` comes back as `0xfffffff8` and the widening cast reported the commonest local-variable reference there is as 4,294,967,288 -- against a field whose whole promise is a *signed* displacement. Measured before fixing: the test went in asserting -8 and printed 4294967288. Sign-extended from the width the address registers are, that being what the addressing form is computed in, and an absolute reference with no register left unsigned, because a 32-bit `[0xfffff000]` is a high address and not a negative offset. The 64-bit form was already right, and is now pinned beside the 32-bit one so the two cannot drift. And the `FunctionExtent::NoEntry` variant still promised that every x86 address answers it, which the gate has never done and which the *method* doc was corrected about last round. Fixing one site and leaving the other is how a contract ends up saying two things; both now say x86 answers `Unsupported`, and why. Mutation-verified: dropping the sign extension fails the new displacement test and nothing else. Re-measured on `mountmgr!MountMgrDeviceControl` unchanged -- 376 instructions, eleven control-code compares, 23 of 23 agreeing across the two decode paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfc3e64822
ℹ️ 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".
| match decoded.op_kind(index) { | ||
| OpKind::Register => Operand::Register(register_name(decoded.op_register(index))), | ||
| OpKind::NearBranch16 | OpKind::NearBranch32 | OpKind::NearBranch64 => { | ||
| Operand::Target(decoded.near_branch_target()) |
There was a problem hiding this comment.
On an x86 target where bit 31 is set—such as a 32-bit kernel or a large-address-aware user process—DbgEng exposes offsets as sign-extended u64 values, while iced-x86's NearBranch32 target is zero-extended. The instruction can therefore be at 0xffffffff80001000 while this operand and the corresponding Flow::target() report 0x80002000; the module-bound walk in typed_disassembly rejects such direct edges, and other consumers read or symbolize the wrong address. Canonicalize decoded x86 targets to DbgEng's sign-extended address form, including the value returned by near_target().
Useful? React with 👍 / 👎.
Round eight, one finding, taken. Measured before fixing: at `ffffffff`80001000` with a 32-bit effective machine, `call +0xffb` reported its destination as `0x80002000` rather than `ffffffff`80002000`. Decoding 32-bit code computes a 32-bit target, so an instruction whose own address carries a high half named a destination in a different address space from itself -- which a module-bounds walk rejects as out of range, and which anything reading or symbolising follows to the wrong place. `.effmach x86` over a 64-bit kernel target is a reachable way to be there, and this branch already knows that mode exists. The high half is inherited from the instruction rather than sign-extended. Both would fix the reported case; inheriting takes the address form from the caller's own value instead of assuming which convention the engine uses, and this code has no business deciding that on the engine's behalf. 64-bit decoding is deliberately untouched, and that is not just "it needs no help": a `rel32` reaches two gigabytes either way, so it can legitimately cross a 4 GB boundary, and masking there would drag a correct target back by four gigabytes. Both directions are asserted, and each of the two mutations -- dropping the canonicalization, and applying it to 64-bit as well -- fails the test. The one case this does not get right is the mirror image, a 32-bit branch wrapping across its own sign boundary, which keeps the high half it started in. Said in the doc rather than left to be discovered. Re-measured on `mountmgr!MountMgrDeviceControl` unchanged: 376 instructions, eleven control-code compares, 23 of 23 agreeing across the two decode paths. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1fae017db
ℹ️ 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".
| } else if rip_relative { | ||
| Some(decoded.ip_rel_memory_address()) | ||
| } else if base == iced_x86::Register::None && index == iced_x86::Register::None { | ||
| Some(decoded.memory_displacement64()) |
There was a problem hiding this comment.
On 32-bit targets where DbgEng exposes high addresses in sign-extended form, an absolute operand such as [0x80002000] is decoded by iced-x86 as the zero-extended 0x80002000, so this branch reports an address outside the debugger's 0xffffffff80000000 address space. Absolute global and import-slot references are common in x86 kernel code, and consumers using this documented field to read or symbolize the operand will access the wrong address; canonicalize it against the instruction address just as direct x86 branch targets are.
Useful? React with 👍 / 👎.
| } else if index != iced_x86::Register::None { | ||
| index.size() * 8 |
There was a problem hiding this comment.
When decoding a 32-bit or address-size-overridden VSIB gather/scatter operand with no GPR base, the index is an XMM/YMM/ZMM register, so index.size() * 8 produces at least 128 rather than the effective address width. sign_extend() consequently becomes a no-op, and a negative displacement such as 0xfffffff8 is exposed as positive 4294967288; derive the width from the instruction's effective address size rather than the vector index register.
Useful? React with 👍 / 👎.
… from the field Round nine, both taken, and both are the previous two rounds' fixes not carried to their siblings. An absolute memory address was left in the decoder's 32-bit space while a branch target was canonicalised into the instruction's. `[0x80002000]` under an instruction at `ffffffff`80001000` is an address in a different space from the code reading it, and absolute globals and import slots are ordinary in x86 kernel code, so this is the field a consumer follows. Same rule, same helper, same reason 64-bit is exempt. And a displacement's signed width now comes from the displacement field rather than from the address registers. The register width was the first answer and is wrong for a VSIB gather, whose index is an `xmm`/`ymm`/`zmm`: `index.size() * 8` is 128 or more, `sign_extend` becomes a no-op, and a negative displacement comes back as four billion -- exactly the defect the sign extension was added to fix, surviving in the one addressing form whose index is not a general-purpose register. The encoded field is the right width by construction, a `disp8` of `0xf8` being `-8` whatever computes the address, and it sidesteps the address-size override too. Mutation-verified one at a time: each reverted fix fails exactly one test. The VSIB case is a real encoding rather than a shape argued about -- `c4 e2 79 92 0c 95 f8 ff ff ff`, and the test asserts the index really is a vector register before asserting the displacement, so it cannot pass by decoding something else. Re-measured on `mountmgr!MountMgrDeviceControl` unchanged: 376 instructions, eleven control-code compares, 23 of 23 agreeing across the two decode paths, and its import thunks still naming themselves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2e35e2710
ℹ️ 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".
| // An absolute reference with no register stays unsigned, because a 32-bit `[0xfffff000]` is a | ||
| // high address rather than a negative offset. | ||
| let displacement = if rip_relative { | ||
| (decoded.ip_rel_memory_address() as i64).wrapping_sub(decoded.next_ip() as i64) |
There was a problem hiding this comment.
When 64-bit code uses an address-size override, the base is EIP: ip_rel_memory_address() is then a zero-extended 32-bit target, while next_ip() retains the instruction's high 64-bit address. Subtracting them directly reports an enormous negative displacement instead of the encoded value, so consumers cannot reconstruct operands such as 67 48 8b 05 fa 0f 00 00 at a high address. Fresh evidence after the earlier RIP/EIP fix is that this new subtraction does not distinguish EIP from RIP; perform the EIP calculation modulo 32 bits.
Useful? React with 👍 / 👎.
| sign_extend( | ||
| decoded.memory_displacement64(), | ||
| decoded.memory_displ_size() as usize * 8, |
There was a problem hiding this comment.
For EVEX instructions using compressed disp8, iced-x86 exposes memory_displacement64() after applying the tuple scale while memory_displ_size() still describes the one encoded byte. Sign-extending that scaled value from eight bits corrupts valid offsets once the scaled magnitude crosses 0x7f; for example, a ZMM full-vector operand with encoded disp8 2 represents +128 but becomes -128 here. Fresh evidence after the earlier displacement-width fix is the new use of the encoded field width for an already-expanded value; derive signedness from the effective address width instead.
Useful? React with 👍 / 👎.
…t to it
Round ten, both taken, and both measured before fixing: the EVEX case printed
-128 for a real +128, and the address-size-overridden relative operand printed
4294971386 for an encoded 0xffa -- four gigabytes out, exactly as claimed.
The width to sign-extend a displacement from is the **effective address
width**. Two simpler readings of that have now each been tried and each is
wrong in one addressing form:
* the index register's width breaks a VSIB gather, whose index is a vector
register, so the extension is a no-op and a negative displacement comes
back as four billion;
* the encoded field's width breaks EVEX, whose `disp8` is compressed -- the
decoder returns it already multiplied by the tuple scale while the field
is still one byte, so extending the expanded value from eight bits turns a
real `+128` into `-128`.
So: the address registers give the width where there are any, an address-size
override being precisely what makes them narrow; and where there is no
general-purpose register -- a pure VSIB form -- the encoding makes the
displacement field the address width, and compression cannot arise there
because a compressed `disp8` needs a base. All three readings are pinned by
mutation, including the two wrong ones, so the next round finds a failing test
rather than a plausible-looking alternative.
And an `EIP`-relative operand computes its displacement at 32 bits. An
address-size override on a 64-bit instruction leaves the decoder wrapping the
target to 32 while the instruction's own next address stays 64, and
subtracting across that puts the answer 2^32 out.
Re-measured on `mountmgr!MountMgrDeviceControl` unchanged: 376 instructions,
eleven control-code compares, 23 of 23 agreeing across the two decode paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2a2feb3d2
ℹ️ 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".
| if decoded.code_size() == iced_x86::CodeSize::Code64 { | ||
| return value; | ||
| } | ||
| (decoded.ip() & 0xffff_ffff_0000_0000) | (value & 0xffff_ffff) |
There was a problem hiding this comment.
When a 32-bit instruction and its target lie on opposite sides of bit 31, this copies the instruction's upper half rather than canonicalizing the decoded target itself. For example, a low-address instruction referencing absolute [0x80002000] still returns 0x0000000080002000 instead of DbgEng's sign-extended offset, and a relative branch crossing the boundary has the same problem, so memory reads, symbol lookup, and flow edges use the wrong address. Fresh evidence in the final tree is that canonical_address now explicitly performs this source-half OR, while the added tests cover only instructions and targets that already occupy the same half.
Useful? React with 👍 / 👎.
Round eleven, one finding, **declined** -- and measured rather than argued, because it asks to reverse round eight's choice and the whole thing turns on one fact nobody had checked: does the engine hand out high x86 addresses in sign-extended form? It does not. Measured on this build against a 32-bit user target (`cppthrow-fastfail-x86.dmp`): `? 80002000` evaluates to `80002000`, and `.formats` prints `Hex: 80002000` -- eight digits, unextended. Sign-extending would therefore invent `ffffffff80002000` for an address the engine itself calls `80002000`, and on a 32-bit kernel, where most code lives above bit 31, it would do that for nearly every address. Inheriting the instruction's own half reproduces the measurement exactly, a genuine 32-bit target's instructions having a zero high half. The finding is right that the tests covered only same-half cases, so the straddling one it names is now asserted: a low instruction reaching a high absolute keeps the low half. That is the residual cost of this choice and it is the narrower of the two exposures -- the sign-extending rule would be wrong for every high address rather than for the ones that straddle. Mutation-verified in the direction that matters here: swapping the inherit for a sign-extension fails the new test. So the alternative is no longer a plausible-looking suggestion, it is a red build with the measurement in the test's name. What would reopen it is a measurement from a 32-bit **kernel** target, which no fixture in this repo has. Said in the doc rather than left implicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ec420cea8
ℹ️ 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".
| return false; | ||
| } | ||
| // `NTSTATUS` severity lives in the top two bits; a defined control code has none set. | ||
| if value & 0x8000_0000 != 0 { |
There was a problem hiding this comment.
When a driver uses a vendor-defined device type in the 0x8000..=0xffff range, its valid CTL_CODE has bit 31 set as the Common bit, so this check discards every such IOCTL as though it were an NTSTATUS. This makes the probe silently omit private control codes—the ones most relevant to third-party driver analysis—so status filtering must not categorically reject the Common bit.
Useful? React with 👍 / 👎.
…ring All three findings on #145 were correct. Two of them turned on facts the API's documentation does not state, so `examples/function_entry_probe.rs` measures them rather than arguing: which failure means "no entry", and what the record looks like off x64. `function_extent` answers a three-state `FunctionExtent` instead of an `Option`, because the two non-answers were different facts collapsed into one. `NoEntry` is now reported for the single measured failure that means it -- `E_NOINTERFACE`, which a real dbgeng 10.x gives for address zero, for a module's header page, and for every x86 address, 32-bit Windows having no unwind table -- and every other failure is returned as an error rather than read as a leaf. `Unsupported` is every instruction set but x64, and refusing rather than decoding is the choice here. ARM64's record is two words whose second is packed unwind data or an `.xdata` RVA: measured on an ARM64 kernel dump, `nt!KeBugCheckEx` fills `needed = 8` with `[0x0025df60, 0x0005f218]`. Read as an end address that is a bogus region, and for any function whose `BeginAddress` is below the `.xdata` RVA it is a bogus region that contains the address asked about, so it passes every sanity check in the function. A wrong region that looks right is worse than no region. Decoding ARM64's packed unwind length is the other option the finding offered and buys nothing yet: the operand reading already refuses that architecture, so nothing downstream could use the bound. The engine's own `needed` is now checked against the x64 shape as well, so the layout is a check rather than an assumption. A software interrupt is classified by its vector rather than its mnemonic. `int 2eh` is the 32-bit system-call path and returns; classifying every `int` as a trap made `falls_through()` false and discarded every instruction after a syscall. `int 29h` and `int 3` still stop a walk -- the second by vector as well as by the `int3` spelling, since that is how the engine renders `0xcc`. Mutation-verified: restoring the blanket `int` trap fails the new vector test and nothing else. The two engine-dependent fixes are verified by the probe against a real ARM64 dump and a real x64 one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
1 participant
Instructiongainsmnemonic,operandsandflowbeside thetextit already had.The engine has no structured disassembly, so the third column was a string and every caller wanting an immediate or a branch target re-parsed it downstream — twice over in
windbg-mcp, once for the reachability walk and once for the path recipe. The parse belongs at the seam that produces the rendering.The two rules the shapes are chosen for
Otherwith its text, never forced into one of the other four.Flowis anOption. "The engine printed a resolvable address" and "this is indirect" are different facts, and a caller readingNoneas no edge stays sound.Reading is gated on
InstructionSet. x86 and x64 are read; ARM64 reports its mnemonic, no operands andFlow::Unknown— not theFallthroughmost instructions happen to be, because an unreadb.eqcalled a fall-through hands a walk one edge of two.Measured, not composed
The unit tests are written against renderings copied by hand, which pins the rules and does not prove the parser: a hand-copied line is chosen from the shapes its author already knew about.
examples/typed_disassembly.rsruns the reading over a whole real dispatch routine instead.mountmgr!MountMgrDeviceControl, 26100 imageOtherFlow::UnknownIt found both defects this PR pins with tests:
u64. That routine rendersmov rax,8000000000000000handmov qword ptr [rbp+0A8h],0FFFFFFFFFFFFFFFFh; a signed parse dropped all three of the routine's unread operands on the floor.ah,bh,chanddhare registers and well-formedh-suffixed hexadecimal, somov ah,5reported a destination of0xa. Swapping those two arms passes every other test in the file, which is why it is pinned on its own.function_extentReads the
.pdataentry for an address, rebased. Named a region deliberately: MSVC splits a function across several entries, and this answers0x14750..0x147a3for that routine — 83 bytes, which.fnentconfirms exactly — while its compare chain lives past0x147dd. Bounding a walk with it recovered zero control codes where following the flow recovered twelve. So it is a sanity bound, not a function's extent, and the doc comment says so.The x64 entry is three
u32RVAs rather than the 64-bit addresses the API's name suggests. Reading them asu64reports "no entry" for a function that plainly has one, which is how this was first written.symbol_forexposes the symbol lookup that was already there privately — what names a call's destination once a walk has one.Checks
cargo fmt --all -- --checkclean,cargo clippy --all-targetsadds no warning,cargo test --lib200 passed / 0 failed.What this is for
The consumer is a
windbg-mcpchange that ports the Driver Buddy Revolutions Ghidra script's analyses natively — a static IOCTL map, a device-security gate and a hazard scan — and reworksreachable_from_dispatchoff itsuf-text parse onto these fields.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf