morluto/rea

Fork 0

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

[PR #4463] [MERGED] fix(pe): ignore zero-extent sections in resource interval ownership #6256

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4463
Author: @jayinvers
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @morluto

Base: main ← Head: fix/pe-zero-extent-section-ownership

📝 Commits (1)

e65ad51 fix(pe): ignore zero-extent sections in resource interval ownership

📊 Changes

2 files changed (+106 additions, -10 deletions)

View changed files

📝 src/native/pe/PeResourceLayout.ts (+21 -10)
📝 src/native/pe/PeResourceParser.test.ts (+85 -0)

📄 Description

Summary

Fixes #4455. Fixes #4373.

inspect-pe-resources rejected otherwise-admitted PE images whose resource
interval (or header-backed payload interval) merely contained a declared
section with VirtualSize = 0 and SizeOfRawData = 0:

Resource interval intersects overlapping virtual sections.
Ambiguous resource header RVA mapping.

Such a section owns no bytes, yet both overlap predicates treated its empty
interval as a point intersecting the requested range. Conversely (#4373), an
empty payload whose RVA below SizeOfHeaders equaled the start of a
positive-extent section silently resolved to the header file offset although
that section owns the point.

Problem and expected behavior

Smallest trigger (from #4455, reproduced on main fe2a398dd): add to the
single-resource fixture a second section with VirtualSize = 0, RVA 0x2001
(inside the resource interval), SizeOfRawData = 0; the CLI rejects the image
as invalid_request/invalid_format although the zero-extent section
contributes no bytes. Violated invariant: the interval checks must agree with
the mapper's own parsed facts — it already computes this section's extent as
zero and excludes it from point ownership.

Expected per the issues: the empty-section controls complete with unchanged
resource locations, bytes and digests; positive-extent overlap controls remain
invalid_format; an empty payload at a section start below SizeOfHeaders
returns the existing ambiguous-mapping failure instead of a silently chosen
header location; unambiguous empty header payloads stay supported.

Change and scope

Single-owner change in src/native/pe/PeResourceLayout.ts: one normalized
intersectsResource predicate reused by the header and section-branch checks.
A section without virtual extent owns no byte and never conflicts; an empty
resource owns exactly its start point (the same rule containing already
applies), so a positive-extent section starting there conflicts with
header-backed ownership. No schema, catalog, or generated-metadata change.

Contribution scope: ordinary bug fix inside the existing static PE boundary;
maintainer scope decision not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: PE resource layout RVA mapping
(src/native/pe/PeResourceLayout.ts); earliest stage is static artifact
parsing/validation.
Contribution scope fit and linked maintainer decision: ordinary fix; not
applicable.
CLI and MCP/tool-catalog contract: unchanged; only the acceptance set for
malformed-edge static inputs changes.
Provider, bridge, target-format, or platform compatibility: built-in static
PE parser only; no engine, bridge, or platform behavior involved.
Evidence, artifact, provenance, or reconstruction contract: unchanged;
locations and digests for previously accepted images are identical.
Target selection, authority, upstream behavior, and cleanup details: not
applicable (no target execution).
Generated metadata, package, or installation impact: none.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added: PE resource ownership boundaries (5 tests) in
src/native/pe/PeResourceParser.test.ts covering both #4455 empty-section
controls, both positive-extent rejection controls, the #4373 conflict, and
the retained unambiguous empty header payload.
Base reproduction (executed; source-level probe with peResourceFixture,
Node 24.18.0, Windows 11):

control	main `fe2a398dd`	this change
empty-section-in-resources	rejected `Resource interval intersects overlapping virtual sections.`	complete, identical location + sha256
empty-section-in-header	rejected `Ambiguous resource header RVA mapping.`	complete, header offset kept
positive-section-in-resources	rejected	rejected (unchanged)
positive-section-in-header	rejected	rejected (unchanged)
#4373 empty payload at section start in headers	complete with silently chosen header offset	rejected `Ambiguous resource header RVA mapping.`
empty payload in headers only	complete	complete (unchanged)

Red/green: reverting only the PeResourceLayout.ts change makes exactly the
three new behavior tests fail; with the change all 33 tests in the file pass.
Remaining proof gaps: no real-image corpus was measured (NOT_ENOUGH_INFO, as
in #4455); Windows loader acceptance and MCP-transport reproduction were not
exercised (the CLI path was not run end-to-end; validation is at the parser
boundary).

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:focused -- src/native/pe/PeResourceParser.test.ts → 33 passed
npm run test:focused -- src/application/binaryDiagnostics/PeResourcesService.test.ts → 5 passed
npm run check:fast → typecheck + lint successful
npx oxfmt --check src/native/pe/PeResourceLayout.ts src/native/pe/PeResourceParser.test.ts → clean
npm run verify:test-discovery → all 1417 test files discovered exactly once
Full CI workflow on the fork for this exact head commit → success (all jobs:
static checks, docs generation, package build, 6/6 test shards, reverse-tools
quality, website, SQLite runtime matrix, real browser/web/EVM/binary-layout
lanes, Windows capability lane, native package baselines):
https://github.com/jayinvers/rea/actions/runs/38898631227

Compatibility, safety, and release

Breaking changes or migration steps: none for previously accepted images; for
size > 0 payloads the predicate is byte-for-byte equivalent to the previous
one. Only zero-extent sections and the empty-payload-at-section-start header
case change, both previously mis-handled.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider).
Package or release metadata impact: none.
Security, privacy, process, or containment review: static parsing only; no
new capability.

Author readiness

Human owner: @jayinvers

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, 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/4463 **Author:** [@jayinvers](https://github.com/jayinvers) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/pe-zero-extent-section-ownership` --- ### 📝 Commits (1) - [`e65ad51`](https://github.com/morluto/rea/commit/e65ad516df79783b36707473beea7bb8ecf0a52c) fix(pe): ignore zero-extent sections in resource interval ownership ### 📊 Changes **2 files changed** (+106 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `src/native/pe/PeResourceLayout.ts` (+21 -10) 📝 `src/native/pe/PeResourceParser.test.ts` (+85 -0) </details> ### 📄 Description  ## Summary Fixes #4455. Fixes #4373. `inspect-pe-resources` rejected otherwise-admitted PE images whose resource interval (or header-backed payload interval) merely contained a declared section with `VirtualSize = 0` and `SizeOfRawData = 0`: - `Resource interval intersects overlapping virtual sections.` - `Ambiguous resource header RVA mapping.` Such a section owns no bytes, yet both overlap predicates treated its empty interval as a point intersecting the requested range. Conversely (#4373), an empty payload whose RVA below `SizeOfHeaders` equaled the start of a positive-extent section silently resolved to the header file offset although that section owns the point. ## Problem and expected behavior Smallest trigger (from #4455, reproduced on main `fe2a398dd`): add to the single-resource fixture a second section with `VirtualSize = 0`, RVA `0x2001` (inside the resource interval), `SizeOfRawData = 0`; the CLI rejects the image as `invalid_request`/`invalid_format` although the zero-extent section contributes no bytes. Violated invariant: the interval checks must agree with the mapper's own parsed facts — it already computes this section's extent as zero and excludes it from point ownership. Expected per the issues: the empty-section controls complete with unchanged resource locations, bytes and digests; positive-extent overlap controls remain `invalid_format`; an empty payload at a section start below `SizeOfHeaders` returns the existing ambiguous-mapping failure instead of a silently chosen header location; unambiguous empty header payloads stay supported. ## Change and scope Single-owner change in `src/native/pe/PeResourceLayout.ts`: one normalized `intersectsResource` predicate reused by the header and section-branch checks. A section without virtual extent owns no byte and never conflicts; an empty resource owns exactly its start point (the same rule `containing` already applies), so a positive-extent section starting there conflicts with header-backed ownership. No schema, catalog, or generated-metadata change. Contribution scope: ordinary bug fix inside the existing static PE boundary; maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: PE resource layout RVA mapping (`src/native/pe/PeResourceLayout.ts`); earliest stage is static artifact parsing/validation. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: unchanged; only the acceptance set for malformed-edge static inputs changes. - Provider, bridge, target-format, or platform compatibility: built-in static PE parser only; no engine, bridge, or platform behavior involved. - Evidence, artifact, provenance, or reconstruction contract: unchanged; locations and digests for previously accepted images are identical. - Target selection, authority, upstream behavior, and cleanup details: not applicable (no target execution). - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added: `PE resource ownership boundaries` (5 tests) in `src/native/pe/PeResourceParser.test.ts` covering both #4455 empty-section controls, both positive-extent rejection controls, the #4373 conflict, and the retained unambiguous empty header payload. - Base reproduction (executed; source-level probe with `peResourceFixture`, Node 24.18.0, Windows 11): | control | main `fe2a398dd` | this change | | --- | --- | --- | | empty-section-in-resources | rejected `Resource interval intersects overlapping virtual sections.` | complete, identical location + sha256 | | empty-section-in-header | rejected `Ambiguous resource header RVA mapping.` | complete, header offset kept | | positive-section-in-resources | rejected | rejected (unchanged) | | positive-section-in-header | rejected | rejected (unchanged) | | #4373 empty payload at section start in headers | complete with silently chosen header offset | rejected `Ambiguous resource header RVA mapping.` | | empty payload in headers only | complete | complete (unchanged) | - Red/green: reverting only the `PeResourceLayout.ts` change makes exactly the three new behavior tests fail; with the change all 33 tests in the file pass. - Remaining proof gaps: no real-image corpus was measured (NOT_ENOUGH_INFO, as in #4455); Windows loader acceptance and MCP-transport reproduction were not exercised (the CLI path was not run end-to-end; validation is at the parser boundary). For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- src/native/pe/PeResourceParser.test.ts` → 33 passed - `npm run test:focused -- src/application/binaryDiagnostics/PeResourcesService.test.ts` → 5 passed - `npm run check:fast` → typecheck + lint successful - `npx oxfmt --check src/native/pe/PeResourceLayout.ts src/native/pe/PeResourceParser.test.ts` → clean - `npm run verify:test-discovery` → all 1417 test files discovered exactly once - Full `CI` workflow on the fork for this exact head commit → success (all jobs: static checks, docs generation, package build, 6/6 test shards, reverse-tools quality, website, SQLite runtime matrix, real browser/web/EVM/binary-layout lanes, Windows capability lane, native package baselines): https://github.com/jayinvers/rea/actions/runs/38898631227 ## Compatibility, safety, and release - Breaking changes or migration steps: none for previously accepted images; for `size > 0` payloads the predicate is byte-for-byte equivalent to the previous one. Only zero-extent sections and the empty-payload-at-section-start header case change, both previously mis-handled. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider). - Package or release metadata impact: none. - Security, privacy, process, or containment review: static parsing only; no new capability. ## Author readiness Human owner: @jayinvers - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, 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 01:13:05 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:21 +00:00

[PR #6256] [MERGED] fix(call-path): count only expanded edges as explored #7230

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

No description provided.

Rows
Columns