[PR #2227] [CLOSED] test(targets): cover bounded executable header readers #5096

Closed
opened 2026-10-31 01:03:53 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2227
Author: @Aurantium-47
Created: 10/12/2026
Status: ❌ Closed

Base: main ← Head: fix/short-dos-stub-pe


📝 Commits (3)

  • d33c970 fix(targets): recognize PE images with short DOS stubs
  • fa8e3b6 fix(targets): preserve uncertain bounded MZ observations
  • 3ca08b8 test(targets): synchronize executable header regression coverage

📊 Changes

3 files changed (+88 additions, -4 deletions)

View changed files

📝 src/ghidra/GhidraInstallation.test.ts (+42 -4)
📝 tests/boundary/filesystem/artifactMzInventory.test.ts (+32 -0)
📝 tests/boundary/go/goBinaryImage.test.ts (+14 -0)

📄 Description

Withdrawn as superseded. The original issue #2172 is resolved on main. The later ELF fixture-size correction is also covered by #2324 through headerProbe(bytes). I am withdrawing the remaining test-only follow-up now that those primary purposes have been addressed upstream.


Summary

Related to #2172 and the upstream fix c9fed506. Synchronize this PR with main and retain executable-header regressions not covered by that fix. Production code now uses main's implementation throughout.

Problem and expected behavior

Short DOS stubs must not hide valid PE identity. A bounded reader that cannot observe the PE signature must preserve uncertainty instead of rejecting the file as DOS. This behavior also needs coverage at Ghidra installation admission, artifact inventory and Go build-metadata readers, beyond the domain and target-resolver tests already on main.

The current main revision c462a4e also has three failing Ghidra admission tests: their ELF64 fixtures contain 64 bytes but report a file size of 52. The stricter upstream ELF reader correctly treats those observations as incomplete, preventing the tests from exercising their intended architecture checks. Correct the fixture sizes to 64 without changing production admission rules.

Change and scope

  • Merge main and resolve the seven original conflicts using the current upstream header interpreter and its consumers. Retain upstream PE/NE/LE/LX handling and subsequent format-validation fixes.
  • Add a Ghidra admission regression with a PE header at offset 8192 and only a 4096-byte prefix available.
  • Cover observed PE, unavailable prefix evidence and a complete DOS-only control in artifact classification, plus a real filesystem inventory scan of a short-stub DLL.
  • Cover Go build metadata in both 32-bit and 64-bit PE images with short DOS stubs.
  • Correct the three ELF64 fixture lengths in the same Ghidra test suite.

The final diff against main changes only three test files. It adds no replacement parser, product capability or public contract. Separate maintainer scope approval is not applicable to this regression coverage within existing static-format inspection.

Review thread discussion_r4249654069 is addressed by retaining upstream's shared header-observation implementation: an unobserved in-file candidate no longer falls through to DOS classification. The added Ghidra and inventory cases assert that uncertainty at the consuming boundaries. The superseded PR parser has no remaining net production diff.

Contract and boundary impact

  • Semantic owner: existing MZ/Windows header interpretation in the domain; unchanged production code.
  • CLI/MCP, provider, format-support and package contracts: unchanged.
  • Evidence contract: tests preserve PE identity and explicit unknowns under bounded observations.
  • Target authority and cleanup: tests generate benign local fixtures and use the existing test-scoped temporary-directory helper. No fixture execution or real provider startup.
  • Generated metadata and installation impact: none.

Known test effects:

  • Read local fixture files or supplied header bytes.
  • Write test-owned temporary fixture files, followed by the existing awaited cleanup.

Evidence and regression coverage

  • Executed upstream baseline: the unmodified main Ghidra test file fails the three ELF64 cases described above (42 pass, 3 fail).

  • Executed regression simulation: restoring the original short-header-as-DOS guard makes all five added short-stub test cases fail. The production source was restored byte-for-byte afterwards. This is a regression simulation, not a claim that current main fails those five cases.

  • The Ghidra case uses an injected inspection host and establishes bounded header-admission behavior, not real decompilation.

  • Observed checks and simulated regressions are distinguished.

  • Fixture identity and bounded observations remain explicit.

  • Unavailable real-provider coverage remains stated.

Validation performed

Windows x64, pinned Node.js 24.18.0 / npm 11.16.0, locked dependencies:

  • Seven relevant test files, one Vitest worker: 248 passed, 8 platform-conditioned skips.
  • Five added cases with the original short-header guard restored: five expected failures; original production source restored afterwards.
  • The full npm run check task set, run serially: typecheck, compile, lint/module boundaries, formatting, unused-code checks and metadata validation all passed; architecture guards passed across 1258 source files. Command: node scripts/run-exclusive.mjs artifacts node_modules/turbo/bin/turbo run typecheck lint:built format:check knip metadata:check --concurrency=1 --output-logs=errors-only, followed by node scripts/verify/architecture-guards.mjs.
  • node scripts/verify-test-discovery.mjs: all 1002 test files discovered exactly once.
  • git diff --check: passed.

The selected files are src/ghidra/GhidraInstallation.test.ts, src/domain/binaryTarget.test.ts, src/domain/dosMz.test.ts, tests/boundary/filesystem/binaryTarget.test.ts, tests/boundary/filesystem/artifactMzInventory.test.ts, tests/boundary/go/goBinaryImage.test.ts and tests/boundary/go/goBinaryMappings.test.ts.

Compatibility, safety, and release

No production behavior, public schema, dependency, packaging or release-metadata changes. No native Windows loader execution or real Hopper/Ghidra session was run. CI owns the full suite and coverage.

Before merge

  • Concrete regressions and fixture failures are described.
  • Scope remains existing static-format inspection; no new target authority.
  • Final diff has one focused executable-header testing outcome.
  • Related issue and upstream fix are linked.
  • Tests cover observable identities, uncertainty and reader boundaries.
  • Documentation and generated metadata need no updates.
  • No user-visible CLI/MCP contract changes require new output examples.
  • Final diff checked for unrelated edits and unsupported claims.

Prepared and verified with OpenAI Codex assistance.


🔄 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/2227 **Author:** [@Aurantium-47](https://github.com/Aurantium-47) **Created:** 10/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/short-dos-stub-pe` --- ### 📝 Commits (3) - [`d33c970`](https://github.com/morluto/rea/commit/d33c9704ae986190e3532f1d17028aabe4b500be) fix(targets): recognize PE images with short DOS stubs - [`fa8e3b6`](https://github.com/morluto/rea/commit/fa8e3b65b196eedd88f899d282a2c7c65e35b537) fix(targets): preserve uncertain bounded MZ observations - [`3ca08b8`](https://github.com/morluto/rea/commit/3ca08b832b878ff9e740825b9244b4eec965ad08) test(targets): synchronize executable header regression coverage ### 📊 Changes **3 files changed** (+88 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/ghidra/GhidraInstallation.test.ts` (+42 -4) 📝 `tests/boundary/filesystem/artifactMzInventory.test.ts` (+32 -0) 📝 `tests/boundary/go/goBinaryImage.test.ts` (+14 -0) </details> ### 📄 Description **Withdrawn as superseded.** The original issue #2172 is resolved on main. The later ELF fixture-size correction is also covered by #2324 through `headerProbe(bytes)`. I am withdrawing the remaining test-only follow-up now that those primary purposes have been addressed upstream. --- ## Summary Related to #2172 and the upstream fix c9fed506. Synchronize this PR with main and retain executable-header regressions not covered by that fix. Production code now uses main's implementation throughout. ## Problem and expected behavior Short DOS stubs must not hide valid PE identity. A bounded reader that cannot observe the PE signature must preserve uncertainty instead of rejecting the file as DOS. This behavior also needs coverage at Ghidra installation admission, artifact inventory and Go build-metadata readers, beyond the domain and target-resolver tests already on main. The current main revision c462a4e also has three failing Ghidra admission tests: their ELF64 fixtures contain 64 bytes but report a file size of 52. The stricter upstream ELF reader correctly treats those observations as incomplete, preventing the tests from exercising their intended architecture checks. Correct the fixture sizes to 64 without changing production admission rules. ## Change and scope - Merge main and resolve the seven original conflicts using the current upstream header interpreter and its consumers. Retain upstream PE/NE/LE/LX handling and subsequent format-validation fixes. - Add a Ghidra admission regression with a PE header at offset 8192 and only a 4096-byte prefix available. - Cover observed PE, unavailable prefix evidence and a complete DOS-only control in artifact classification, plus a real filesystem inventory scan of a short-stub DLL. - Cover Go build metadata in both 32-bit and 64-bit PE images with short DOS stubs. - Correct the three ELF64 fixture lengths in the same Ghidra test suite. The final diff against main changes only three test files. It adds no replacement parser, product capability or public contract. Separate maintainer scope approval is not applicable to this regression coverage within existing static-format inspection. Review thread discussion_r4249654069 is addressed by retaining upstream's shared header-observation implementation: an unobserved in-file candidate no longer falls through to DOS classification. The added Ghidra and inventory cases assert that uncertainty at the consuming boundaries. The superseded PR parser has no remaining net production diff. ## Contract and boundary impact - Semantic owner: existing MZ/Windows header interpretation in the domain; unchanged production code. - CLI/MCP, provider, format-support and package contracts: unchanged. - Evidence contract: tests preserve PE identity and explicit unknowns under bounded observations. - Target authority and cleanup: tests generate benign local fixtures and use the existing test-scoped temporary-directory helper. No fixture execution or real provider startup. - Generated metadata and installation impact: none. Known test effects: - [x] Read local fixture files or supplied header bytes. - [x] Write test-owned temporary fixture files, followed by the existing awaited cleanup. ## Evidence and regression coverage - Executed upstream baseline: the unmodified main Ghidra test file fails the three ELF64 cases described above (42 pass, 3 fail). - Executed regression simulation: restoring the original short-header-as-DOS guard makes all five added short-stub test cases fail. The production source was restored byte-for-byte afterwards. This is a regression simulation, not a claim that current main fails those five cases. - The Ghidra case uses an injected inspection host and establishes bounded header-admission behavior, not real decompilation. - [x] Observed checks and simulated regressions are distinguished. - [x] Fixture identity and bounded observations remain explicit. - [x] Unavailable real-provider coverage remains stated. ## Validation performed Windows x64, pinned Node.js 24.18.0 / npm 11.16.0, locked dependencies: - Seven relevant test files, one Vitest worker: 248 passed, 8 platform-conditioned skips. - Five added cases with the original short-header guard restored: five expected failures; original production source restored afterwards. - The full `npm run check` task set, run serially: typecheck, compile, lint/module boundaries, formatting, unused-code checks and metadata validation all passed; architecture guards passed across 1258 source files. Command: `node scripts/run-exclusive.mjs artifacts node_modules/turbo/bin/turbo run typecheck lint:built format:check knip metadata:check --concurrency=1 --output-logs=errors-only`, followed by `node scripts/verify/architecture-guards.mjs`. - `node scripts/verify-test-discovery.mjs`: all 1002 test files discovered exactly once. - `git diff --check`: passed. The selected files are `src/ghidra/GhidraInstallation.test.ts`, `src/domain/binaryTarget.test.ts`, `src/domain/dosMz.test.ts`, `tests/boundary/filesystem/binaryTarget.test.ts`, `tests/boundary/filesystem/artifactMzInventory.test.ts`, `tests/boundary/go/goBinaryImage.test.ts` and `tests/boundary/go/goBinaryMappings.test.ts`. ## Compatibility, safety, and release No production behavior, public schema, dependency, packaging or release-metadata changes. No native Windows loader execution or real Hopper/Ghidra session was run. CI owns the full suite and coverage. ## Before merge - [x] Concrete regressions and fixture failures are described. - [x] Scope remains existing static-format inspection; no new target authority. - [x] Final diff has one focused executable-header testing outcome. - [x] Related issue and upstream fix are linked. - [x] Tests cover observable identities, uncertainty and reader boundaries. - [x] Documentation and generated metadata need no updates. - [x] No user-visible CLI/MCP contract changes require new output examples. - [x] Final diff checked for unrelated edits and unsupported claims. Prepared and verified with OpenAI Codex assistance. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:03:53 +00:00
Sign in to join this conversation.
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#5096
No description provided.