morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #2704] [MERGED] fix(dotnet): name the truncated instruction when an operand leaves the method body #5328

New issue

Closed

opened 2026-10-31 01:05:46 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2704
Author: @YunzhouLi-hub
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/dotnet-2677-operand-diagnostic

📝 Commits (4)

1e9af71 fix(dotnet): name the truncated instruction when an operand leaves the method body
a4e87ee chore: merge captured main updates
953877f test(dotnet): consolidate truncated operand coverage
53f7bde Merge branch 'main' into fix/dotnet-2677-operand-diagnostic

📊 Changes

2 files changed (+288 additions, -88 deletions)

View changed files

📝 src/dotnet/ManagedCilDecoding.test.ts (+205 -38)
📝 src/dotnet/ManagedMemberInstructionDecoder.ts (+83 -50)

📄 Description

Summary

decodeInstructions checked the operand extent only after reading the operand value. Every fixed reader and the token path call Node Buffer readers that throw a RangeError once the operand runs past the end of the IL body. That RangeError was caught and its raw runtime message became body.issue, so a truncated method body reported "Attempt to access memory outside buffer bounds" instead of naming the instruction.

This bounds the operand extent before reading, so the existing Instruction <name> at IL offset <n> leaves method body diagnostic wins.

Problem and expected behavior

Reporter's case — buildManagedPeFixture({ ilBody: Buffer.from([0x06, 0x20]) }) (ldloc.0; ldc.i4 with no operand) through inspectManagedMembersBytes / managedPeFixtureTarget:

Actual: "issue": "Attempt to access memory outside buffer bounds"
Expected: "issue": "Instruction ldc.i4 at IL offset 0 leaves method body"

Verified on the merge base:

  {
    "decoded_instruction_count": 0,
    "header_format": "tiny",
    "il_size": 1,
-   "issue": "Instruction ldc.i4 at IL offset 0 leaves method body",
+   "issue": "Attempt to access memory outside buffer bounds",
    "normalized_il_sha256": null,
    "status": "malformed"
  }

status: "malformed", coverage: "partial" and normalized_il_sha256: null are unchanged — only the diagnostic text improves.

Change and scope

src/dotnet/ManagedMemberInstructionDecoder.ts
- New operandExtent(il, offset, kind) computes the operand width without reading it: fixed widths from the table, 4 + count * 4 for switch (returning null when the count field itself is unreadable), and 4 for token kinds.
- FIXED_OPERAND_READERS becomes Map<string, FixedOperand> = { width, read }, so each reader's byte width sits beside the read that uses it. The width and the reader can no longer drift apart.
- decodeInstructions now checks the extent before calling readOperand, and the old post-read operand.next < offset || operand.next > il.length guard is deleted. next < offset was dead — every reader returns offset + non-negative.
- readOperand's unknown-kind branch returns without reading (it previously read 4 bytes yet reported extent 0). The branch is unreachable from the closed OPCODES table, so behaviour is unchanged; the inconsistency is removed rather than left as a trap.
- TOKEN_OPERAND_KINDS moved above its consumers to avoid use-before-declaration.
src/dotnet/ManagedCilDecoding.test.ts — new describe("managed operand extent diagnostics"), 39 tests.

No change to ManagedMethodBodyReader.ts, the public CLI/MCP contracts, or any output shape.

Contract and boundary impact

The fix is at the producing boundary — the place that decides an operand is out of range — rather than a downstream repair of the message. body.issue remains decoded.issue ?? malformed-regions ?? truncated-fallback at src/dotnet/ManagedMethodBodyReader.ts:175, so status: "malformed", coverage: "partial", decoded_instruction_count and the null normalized_il_sha256 are all preserved exactly as before.

Evidence and regression coverage

Coverage is in both directions, because a width that is wrong in either direction reintroduces the raw buffer error or wrongly rejects a valid operand:

Direction	What it catches
Zero operand bytes	A width declared as `0`
One byte short of the real width	A width declared too small
Exactly the declared width, with a known value	A width declared too large

Mutation-checked rather than assumed:

Mutation	Result
`ldc.i4` width 4 → 3	one-byte-short case fails
`ldc.i4` width 4 → 5	exact-width case fails
`ldc.r4` width 4 → 8	exact-width case fails
`TOKEN_OPERAND_WIDTH` 4 → 2	8 cases fail (both directions, all 4 token kinds)

With the decoder reverted, 16 of the 19 original zero-byte tests fail and all 27 pre-existing tests pass either way. The 3 that pass both ways are the two controls plus the readable-count switch case, which the old operand.next guard already handled.

The new tests assert the instruction name, offset and decoded value, not just that the message changed, so no generic message would satisfy them.

Validation performed

Command	Result
`npm run test:local -- src/dotnet/ManagedCilDecoding.test.ts`	66/66 pass
`npm run test:local --` (all 11 `src/dotnet/*.test.ts`)	222/222 pass
`npm run check:fast`	2/2 tasks successful
`npm run verify:test-discovery`	all 1055 test files discovered exactly once
`npx oxfmt --check` on both changed files	correct format

Node 24.15.0 / npm 11.12.1, Windows x64. npm run docs:check passes 4/5 tasks here; artifacts:managed-evidence fails with EPERM ... mkdtemp on this host — I confirmed it fails identically on a clean checkout, so it is environmental, not caused by this change.

Not verified: no real managed .exe/.dll fixture and no real-provider run — this is a diagnostic-wording fix validated entirely on the synthetic ManagedPe.fixture.ts fixtures.

Compatibility, safety, and release

Output-shape compatible: only the text of an existing diagnostic changes. Consumers matching on the exact old "Attempt to access memory outside buffer bounds" string would need to relax that match — unlikely, and the new text is the more useful contract.
No execution, no filesystem writes, no provider process, no network. Pure in-process decoding of an already-read buffer.
Patch-level fix.

Before merge

Confirm the diagnostic wording matches the project's preferred phrasing for instruction-extent failures.
Decide whether the equivalent unguarded reads elsewhere in the CIL pipeline should be swept in the same change or tracked separately. I found none in decodeInstructions' operand paths, but I have not audited ManagedMemberInstructionDecoder.ts's callers beyond ManagedMethodBodyReader.ts.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2704 **Author:** [@YunzhouLi-hub](https://github.com/YunzhouLi-hub) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/dotnet-2677-operand-diagnostic` --- ### 📝 Commits (4) - [`1e9af71`](https://github.com/morluto/rea/commit/1e9af71f0e05b6f303c3227a5d93985a6c1a0972) fix(dotnet): name the truncated instruction when an operand leaves the method body - [`a4e87ee`](https://github.com/morluto/rea/commit/a4e87eea00745b58042f1114fbe0529ae396a3f6) chore: merge captured main updates - [`953877f`](https://github.com/morluto/rea/commit/953877f78585caefa2de2c801ff282a4f7396ca5) test(dotnet): consolidate truncated operand coverage - [`53f7bde`](https://github.com/morluto/rea/commit/53f7bde88449d471b7524adc763883118bdc1ce1) Merge branch 'main' into fix/dotnet-2677-operand-diagnostic ### 📊 Changes **2 files changed** (+288 additions, -88 deletions) <details> <summary>View changed files</summary> 📝 `src/dotnet/ManagedCilDecoding.test.ts` (+205 -38) 📝 `src/dotnet/ManagedMemberInstructionDecoder.ts` (+83 -50) </details> ### 📄 Description ## Summary `decodeInstructions` checked the operand extent only *after* reading the operand value. Every fixed reader and the token path call Node `Buffer` readers that throw a `RangeError` once the operand runs past the end of the IL body. That `RangeError` was caught and its raw runtime message became `body.issue`, so a truncated method body reported `"Attempt to access memory outside buffer bounds"` instead of naming the instruction. This bounds the operand extent *before* reading, so the existing `Instruction <name> at IL offset <n> leaves method body` diagnostic wins. ## Problem and expected behavior Reporter's case — `buildManagedPeFixture({ ilBody: Buffer.from([0x06, 0x20]) })` (`ldloc.0; ldc.i4` with no operand) through `inspectManagedMembersBytes` / `managedPeFixtureTarget`: - Actual: ``"issue": "Attempt to access memory outside buffer bounds"`` - Expected: ``"issue": "Instruction ldc.i4 at IL offset 0 leaves method body"`` Verified on the merge base: ``` { "decoded_instruction_count": 0, "header_format": "tiny", "il_size": 1, - "issue": "Instruction ldc.i4 at IL offset 0 leaves method body", + "issue": "Attempt to access memory outside buffer bounds", "normalized_il_sha256": null, "status": "malformed" } ``` `status: "malformed"`, `coverage: "partial"` and `normalized_il_sha256: null` are unchanged — only the diagnostic text improves. ## Change and scope - `src/dotnet/ManagedMemberInstructionDecoder.ts` - New `operandExtent(il, offset, kind)` computes the operand width without reading it: fixed widths from the table, `4 + count * 4` for `switch` (returning `null` when the count field itself is unreadable), and `4` for token kinds. - `FIXED_OPERAND_READERS` becomes `Map<string, FixedOperand>` = `{ width, read }`, so each reader's byte width sits beside the read that uses it. The width and the reader can no longer drift apart. - `decodeInstructions` now checks the extent before calling `readOperand`, and the old post-read `operand.next < offset || operand.next > il.length` guard is deleted. `next < offset` was dead — every reader returns `offset + non-negative`. - `readOperand`'s unknown-kind branch returns without reading (it previously read 4 bytes yet reported extent 0). The branch is unreachable from the closed `OPCODES` table, so behaviour is unchanged; the inconsistency is removed rather than left as a trap. - `TOKEN_OPERAND_KINDS` moved above its consumers to avoid use-before-declaration. - `src/dotnet/ManagedCilDecoding.test.ts` — new `describe("managed operand extent diagnostics")`, 39 tests. No change to `ManagedMethodBodyReader.ts`, the public CLI/MCP contracts, or any output shape. ## Contract and boundary impact The fix is at the producing boundary — the place that decides an operand is out of range — rather than a downstream repair of the message. `body.issue` remains `decoded.issue ?? malformed-regions ?? truncated-fallback` at `src/dotnet/ManagedMethodBodyReader.ts:175`, so `status: "malformed"`, `coverage: "partial"`, `decoded_instruction_count` and the null `normalized_il_sha256` are all preserved exactly as before. ## Evidence and regression coverage Coverage is in both directions, because a width that is wrong in *either* direction reintroduces the raw buffer error or wrongly rejects a valid operand: | Direction | What it catches | | --- | --- | | Zero operand bytes | A width declared as `0` | | One byte short of the real width | A width declared too small | | Exactly the declared width, with a known value | A width declared too large | Mutation-checked rather than assumed: | Mutation | Result | | --- | --- | | `ldc.i4` width 4 → 3 | one-byte-short case fails | | `ldc.i4` width 4 → 5 | exact-width case fails | | `ldc.r4` width 4 → 8 | exact-width case fails | | `TOKEN_OPERAND_WIDTH` 4 → 2 | 8 cases fail (both directions, all 4 token kinds) | With the decoder reverted, 16 of the 19 original zero-byte tests fail and all 27 pre-existing tests pass either way. The 3 that pass both ways are the two controls plus the readable-count `switch` case, which the old `operand.next` guard already handled. The new tests assert the instruction *name, offset and decoded value*, not just that the message changed, so no generic message would satisfy them. ## Validation performed | Command | Result | | --- | --- | | `npm run test:local -- src/dotnet/ManagedCilDecoding.test.ts` | 66/66 pass | | `npm run test:local --` (all 11 `src/dotnet/*.test.ts`) | 222/222 pass | | `npm run check:fast` | 2/2 tasks successful | | `npm run verify:test-discovery` | all 1055 test files discovered exactly once | | `npx oxfmt --check` on both changed files | correct format | Node 24.15.0 / npm 11.12.1, Windows x64. `npm run docs:check` passes 4/5 tasks here; `artifacts:managed-evidence` fails with `EPERM ... mkdtemp` on this host — I confirmed it fails identically on a clean checkout, so it is environmental, not caused by this change. Not verified: no real managed `.exe`/`.dll` fixture and no real-provider run — this is a diagnostic-wording fix validated entirely on the synthetic `ManagedPe.fixture.ts` fixtures. ## Compatibility, safety, and release - Output-shape compatible: only the text of an existing diagnostic changes. Consumers matching on the exact old `"Attempt to access memory outside buffer bounds"` string would need to relax that match — unlikely, and the new text is the more useful contract. - No execution, no filesystem writes, no provider process, no network. Pure in-process decoding of an already-read buffer. - Patch-level `fix`. ## Before merge - [ ] Confirm the diagnostic wording matches the project's preferred phrasing for instruction-extent failures. - [ ] Decide whether the equivalent unguarded reads elsewhere in the CIL pipeline should be swept in the same change or tracked separately. I found none in `decodeInstructions`' operand paths, but I have not audited `ManagedMemberInstructionDecoder.ts`'s callers beyond `ManagedMethodBodyReader.ts`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:46 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5328

No description provided.

Rows
Columns