morluto/rea

Fork 0

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

[PR #1886] [MERGED] fix(managed): resolve member ownership through pointer tables #4877

New issue

Closed

opened 2026-10-31 00:56:18 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:56:18 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1886
Author: @chqcjq
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/11/2026
Merged by: @morluto

Base: main ← Head: fix/managed-member-pointer-ownership

📝 Commits (1)

ccc72e5 fix(managed): resolve member ownership through pointer tables

📊 Changes

11 files changed (+766 additions, -26 deletions)

View changed files

📝 docs/managed-code-analysis.md (+11 -0)
📝 src/dotnet/ManagedMemberInspectorCore.ts (+14 -10)
➕ src/dotnet/ManagedMemberPointerBudget.test.ts (+96 -0)
➕ src/dotnet/ManagedMemberPointers.test.ts (+275 -0)
➕ src/dotnet/ManagedMemberPointers.ts (+101 -0)
📝 src/dotnet/ManagedMemberRows.ts (+19 -2)
📝 src/dotnet/ManagedMetadataInventory.ts (+1 -1)
📝 src/dotnet/ManagedMetadataInventoryRows.ts (+21 -8)
📝 src/dotnet/ManagedMetadataRowSizes.ts (+1 -1)
📝 src/dotnet/ManagedPe.fixture.ts (+133 -4)
➕ tests/acceptance/managedMemberPointers.test.ts (+94 -0)

📄 Description

Summary

Resolve declaring-type ownership through FieldPtr and MethodPtr for unoptimized managed metadata. Physical Field/MethodDef tokens remain unchanged, while member owners and local custom-attribute constructor types follow the metadata's pointer ordering.

This is the pre-existing defect identified in the #1706 review, which the author explicitly deferred to a separate shared-owner fix. It does not implement type selection or the separate cursor-budget work in #1878.

Problem and expected behavior

REA admits #- metadata but compares physical Field/MethodDef row numbers directly against TypeDef list positions. Those coordinates differ when a pointer table reorders the definitions.

A 4 KiB source-owned fixture has <Module>, Fixture.A, Fixture.B, two fields and two methods, and pointer rows [2,1]. On base fb94d9dd, the first physical field/method is incorrectly attributed to A and the second to B, with complete coverage. A custom attribute whose constructor is physical MethodDef row 1 also receives the wrong type name.

Expected: physical row 1 belongs to B and row 2 to A; their tokens remain 0x04000001/02 and 0x06000001/02. Microsoft's System.Reflection.Metadata/PEReader independently produced that ownership for the same fixture without loading or executing the assembly.

Change and scope

Share a per-layout inverse pointer lookup between member and attribute-constructor inspection. Read list widths from both pointer and definition tables, and use nonempty pointer-table counts for list bounds.
Reserve retained index capacity before creating its containers. Cache the lookup, including malformed-table failures, to avoid repeated scans/charges.
Reject zero, out-of-range and ambiguous pointers as typed coverage issues. Keep readable member names/tokens and custom-attribute parent/constructor/value facts; only an unresolvable owner/type name becomes unknown.
Document that TypeDef list ranges retain list-slot coordinates, which need not be contiguous physical definition rows.

The implementation changes the shared interpretation, rather than patching a selected-type filter. Existing direct-list behavior and malformed TypeDef first-match behavior remain covered. No cursor/CIL/string/projection budget was removed or increased.

Contract and boundary impact

Semantic owner and earliest changed stage: static PE/CLI metadata interpretation in src/dotnet.
CLI/MCP/tool catalog: no schema change. Corrected ownership and preserved partial facts use existing fields.
Provider/format/platform: FieldPtr/MethodPtr support in the portable static reader; no external engine, target execution, or native mutation.
Evidence/provenance: preserve physical member tokens, row offsets, artifact identity and valid attribute payload facts. Pointer list positions are not substituted for definition tokens.
Process/authorization/cleanup: no new process or authority behavior.
Generated metadata/package/installation: no dependency or version change; generated outputs are not committed.

Evidence and regression coverage

Executed baseline: three independent failures for field ownership, method ownership and local attribute-constructor type; identity-pointer and ordinary #~ controls passed.
Nineteen parser regressions cover reordered/identity/absent/empty pointer tables, both tables' zero/out-of-range/duplicate/omitted entries, truncated admitted table extents, and preservation of independent member/attribute facts.
Two budget regressions use a small real fixture: insufficient budget results in zero Uint32Array constructions; enough budget for one lookup permits reuse with no second allocation or charge. These check admission ordering, not a hard bound on all V8 heap use.
A compiled CLI/real stdio MCP acceptance test compares artifact/member results and validates the unchanged fixture digest and physical tokens.
Independent Microsoft metadata-reader check of the same generated PE confirmed the expected reordered ownership. This is a synthetic fixture accepted by an independent reader, not a claim about arbitrary compiler-produced binaries.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity and source provenance remain preserved.
Partial and uncertain outcomes remain visible.

Validation performed

On Windows x64 with the pinned Node 24.18.0 / npm 11.16.0:

npm run test:local -- <all 10 src/dotnet/*.test.ts files> — 158 passed, no failures or skips.
npm run test:focused -- tests/acceptance/managedMemberPointers.test.ts — 1 passed, compiled CLI/stdio MCP parity.
npm run verify:managed — passed, 11 portable conformance scenarios. Optional operator-local manifest and ILSpy oracle were not configured and remain explicitly unsupported in the report.
npm run check — passed; existing and reported lint warnings remain, no errors.
npm run docs:check — passed.
npm run verify:test-discovery — all 845 files discovered exactly once.
git diff --check — passed.
Local codex review --base origin/main with invocation-only compatibility overrides for two unsupported feature-table settings — completed with no actionable regressions reported. Its supplementary tests used the host-default Node 24.9.0; the authoritative test/gate results listed above were separately run on pinned Node 24.18.0.

Compatibility, safety, and release

Breaking changes/migration: none; callers receive corrected ownership for affected metadata.
Scope limits: this change does not add ParamPtr/EventPtr/PropertyPtr interpretation, fix the separate transient cursor-accounting issue, or introduce synchronous decoder cancellation.
Native/OS coverage: no Hopper/Ghidra/Android claims. Local public-boundary verification ran on Windows; no Linux/macOS run is claimed.
No binary fixture, local evidence log, credential or generated artifact is included.

Review checklist

One concrete defect, with the existing report and independent-fix scope linked.
Minimal owning-layer changes and Conventional Commit title.
Executed red/green, malformed-input, provenance and allocation-boundary checks.
Owning documentation and public CLI/MCP evidence included.
Final diff checked for unrelated changes and unsupported claims.

_{🔄 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/1886 **Author:** [@chqcjq](https://github.com/chqcjq) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/managed-member-pointer-ownership` --- ### 📝 Commits (1) - [`ccc72e5`](https://github.com/morluto/rea/commit/ccc72e5f4132c4bbb51ae30c75e54e3c1cc4a497) fix(managed): resolve member ownership through pointer tables ### 📊 Changes **11 files changed** (+766 additions, -26 deletions) <details> <summary>View changed files</summary> 📝 `docs/managed-code-analysis.md` (+11 -0) 📝 `src/dotnet/ManagedMemberInspectorCore.ts` (+14 -10) ➕ `src/dotnet/ManagedMemberPointerBudget.test.ts` (+96 -0) ➕ `src/dotnet/ManagedMemberPointers.test.ts` (+275 -0) ➕ `src/dotnet/ManagedMemberPointers.ts` (+101 -0) 📝 `src/dotnet/ManagedMemberRows.ts` (+19 -2) 📝 `src/dotnet/ManagedMetadataInventory.ts` (+1 -1) 📝 `src/dotnet/ManagedMetadataInventoryRows.ts` (+21 -8) 📝 `src/dotnet/ManagedMetadataRowSizes.ts` (+1 -1) 📝 `src/dotnet/ManagedPe.fixture.ts` (+133 -4) ➕ `tests/acceptance/managedMemberPointers.test.ts` (+94 -0) </details> ### 📄 Description ## Summary Resolve declaring-type ownership through `FieldPtr` and `MethodPtr` for unoptimized managed metadata. Physical Field/MethodDef tokens remain unchanged, while member owners and local custom-attribute constructor types follow the metadata's pointer ordering. This is the pre-existing defect identified in [the #1706 review](https://github.com/morluto/rea/pull/1706#discussion_r4240482341), which [the author explicitly deferred to a separate shared-owner fix](https://github.com/morluto/rea/pull/1706#discussion_r4240489625). It does not implement type selection or the separate cursor-budget work in #1878. ## Problem and expected behavior REA admits `#-` metadata but compares physical Field/MethodDef row numbers directly against TypeDef list positions. Those coordinates differ when a pointer table reorders the definitions. A 4 KiB source-owned fixture has `<Module>`, `Fixture.A`, `Fixture.B`, two fields and two methods, and pointer rows `[2,1]`. On base `fb94d9dd`, the first physical field/method is incorrectly attributed to A and the second to B, with complete coverage. A custom attribute whose constructor is physical MethodDef row 1 also receives the wrong type name. Expected: physical row 1 belongs to B and row 2 to A; their tokens remain `0x04000001/02` and `0x06000001/02`. Microsoft's `System.Reflection.Metadata`/`PEReader` independently produced that ownership for the same fixture without loading or executing the assembly. ## Change and scope - Share a per-layout inverse pointer lookup between member and attribute-constructor inspection. Read list widths from both pointer and definition tables, and use nonempty pointer-table counts for list bounds. - Reserve retained index capacity **before** creating its containers. Cache the lookup, including malformed-table failures, to avoid repeated scans/charges. - Reject zero, out-of-range and ambiguous pointers as typed coverage issues. Keep readable member names/tokens and custom-attribute parent/constructor/value facts; only an unresolvable owner/type name becomes unknown. - Document that TypeDef list ranges retain list-slot coordinates, which need not be contiguous physical definition rows. The implementation changes the shared interpretation, rather than patching a selected-type filter. Existing direct-list behavior and malformed TypeDef first-match behavior remain covered. No cursor/CIL/string/projection budget was removed or increased. ## Contract and boundary impact - Semantic owner and earliest changed stage: static PE/CLI metadata interpretation in `src/dotnet`. - CLI/MCP/tool catalog: no schema change. Corrected ownership and preserved partial facts use existing fields. - Provider/format/platform: FieldPtr/MethodPtr support in the portable static reader; no external engine, target execution, or native mutation. - Evidence/provenance: preserve physical member tokens, row offsets, artifact identity and valid attribute payload facts. Pointer list positions are not substituted for definition tokens. - Process/authorization/cleanup: no new process or authority behavior. - Generated metadata/package/installation: no dependency or version change; generated outputs are not committed. ## Evidence and regression coverage - Executed baseline: three independent failures for field ownership, method ownership and local attribute-constructor type; identity-pointer and ordinary `#~` controls passed. - Nineteen parser regressions cover reordered/identity/absent/empty pointer tables, both tables' zero/out-of-range/duplicate/omitted entries, truncated admitted table extents, and preservation of independent member/attribute facts. - Two budget regressions use a small real fixture: insufficient budget results in **zero** Uint32Array constructions; enough budget for one lookup permits reuse with no second allocation or charge. These check admission ordering, not a hard bound on all V8 heap use. - A compiled CLI/real stdio MCP acceptance test compares artifact/member results and validates the unchanged fixture digest and physical tokens. - Independent Microsoft metadata-reader check of the same generated PE confirmed the expected reordered ownership. This is a synthetic fixture accepted by an independent reader, not a claim about arbitrary compiler-produced binaries. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity and source provenance remain preserved. - [x] Partial and uncertain outcomes remain visible. ## Validation performed On Windows x64 with the pinned Node 24.18.0 / npm 11.16.0: - `npm run test:local -- <all 10 src/dotnet/*.test.ts files>` — **158 passed**, no failures or skips. - `npm run test:focused -- tests/acceptance/managedMemberPointers.test.ts` — **1 passed**, compiled CLI/stdio MCP parity. - `npm run verify:managed` — passed, **11 portable conformance scenarios**. Optional operator-local manifest and ILSpy oracle were not configured and remain explicitly unsupported in the report. - `npm run check` — passed; existing and reported lint warnings remain, no errors. - `npm run docs:check` — passed. - `npm run verify:test-discovery` — all **845 files** discovered exactly once. - `git diff --check` — passed. - Local `codex review --base origin/main` with invocation-only compatibility overrides for two unsupported feature-table settings — completed with no actionable regressions reported. Its supplementary tests used the host-default Node 24.9.0; the authoritative test/gate results listed above were separately run on pinned Node 24.18.0. ## Compatibility, safety, and release - Breaking changes/migration: none; callers receive corrected ownership for affected metadata. - Scope limits: this change does not add ParamPtr/EventPtr/PropertyPtr interpretation, fix the separate transient cursor-accounting issue, or introduce synchronous decoder cancellation. - Native/OS coverage: no Hopper/Ghidra/Android claims. Local public-boundary verification ran on Windows; no Linux/macOS run is claimed. - No binary fixture, local evidence log, credential or generated artifact is included. ## Review checklist - [x] One concrete defect, with the existing report and independent-fix scope linked. - [x] Minimal owning-layer changes and Conventional Commit title. - [x] Executed red/green, malformed-input, provenance and allocation-boundary checks. - [x] Owning documentation and public CLI/MCP evidence included. - [x] Final diff checked for unrelated changes and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:56:18 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:14:34 +00:00

[PR #4877] [CLOSED] Fix #4856: Prevent call stack overflow on large TOML files #6452

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#4877

No description provided.

Rows
Columns