Skip to content

test(targets): cover bounded executable header readers - #2227

Closed
Aurantium-47 wants to merge 3 commits into
morluto:mainfrom
Aurantium-47:fix/short-dos-stub-pe
Closed

Aurantium-47 wants to merge 3 commits into
morluto:mainfrom
Aurantium-47:fix/short-dos-stub-pe

Conversation

@Aurantium-47

@Aurantium-47 Aurantium-47 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

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 c9fed50. 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.

Confirm candidate pointers from short DOS load modules against PE signatures, including headers beyond the metadata probe. Preserve DOS-only fallback and unknown bounded inventory observations.

Assisted-by: OpenAI GPT-6 <noreply@openai.com>
@morluto

morluto commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Brilliant! Thank you for this Aurantium

1 similar comment
@morluto

morluto commented Oct 12, 2026

Copy link
Copy Markdown
Owner

Brilliant! Thank you for this Aurantium

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d33c9704ae

ℹ️ 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".

Comment thread src/domain/binaryTarget.ts Resolved
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T15:44:55.340218Z fa8e3b6 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Keep unobserved in-file PE candidates unknown in bounded executable parsing. Preserve the filesystem resolver's separately observed negative signature when validating DOS metadata and share candidate bounds with inventory.

Add a Ghidra installation admission regression for a short-stub PE header beyond its 4096-byte probe.

Assisted-by: OpenAI GPT-6 <noreply@openai.com>
@CassieFox23

Copy link
Copy Markdown
Contributor

TL;DR: Head fa8e3b6 reads a valid PE with a short DOS stub as PE. DOS-only files with stray bytes at 0x3c are never read as PE. On the two paths that read only a prefix, a far pointer now gives "unknown".

Results

I tested head fa8e3b6 against base 7711b00 (#2161) with 51 files. One set is the exact #2172 DLL, rebuilt with clang and lld-link. Each cell reads "base, then head". The table covers 48 files. The other 3 are in the fold below.

Group (files) inspect inspect-artifact inspect-go-binary Ghidra check
PE, short stub, header before byte 4,088 (9) DOS, then PE DOS, then PE rejected, then read (1 file) refused, then passes
PE, short stub, header at byte 4,088 to 8,188 (6) DOS, then PE DOS, then PE n/a refused, then unknown
PE, short stub, header at byte 8,192 or later (4) DOS, then PE DOS, then unknown n/a refused, then unknown
Go PE, short stub (2) DOS, then PE DOS, then PE rejected, then read refused as DOS, then judged as PE
PE, normal stub (6) PE PE (1 unknown) read no change
DOS-only, stray bytes at 0x3c (15) DOS DOS rejected refused as DOS
DOS-only, stray pointer past the probe (3) DOS DOS, then unknown (2 files) n/a refused, then unknown
PE, short stub, cut after the signature (2) DOS, then error DOS, then exit 1 n/a refused, then unknown
PE, normal stub, cut after the signature (1) error exit 1 n/a unknown
  • inspect is the target resolver: rea inspect <file> --provider hopper with no launcher. The DOS refusal from fix(targets): linker-generated PE DLL with a short DOS header is classified as DOS #2172 is gone for every short-stub PE.
  • The Ghidra check reads a native decompiler file in a Ghidra install. The table shows the Windows rules, tried on Linux.
  • None of the 18 DOS-only files was read as PE on any path.
  • The far-pointer cases are a limit of the prefix. The inventory reads 8,192 bytes. The Ghidra check reads 4,096. The resolver reads 4 bytes at the pointer, so it stays right.
  • A normal-stub PE with its header at 8,192 is also "unknown" in the inventory, on base too.
  • On my files the first head d33c970 gave the same answers, except in the Ghidra check. It refused 11 files that fa8e3b6 now passes (8 PE, 3 DOS-only).
  • Tests: the 7 files in the PR body pass on Node 22.23.1, 24.18.0 and 26.11.1. That is 236 passed and 1 skipped on each. The 6 new tests fail on base, as the PR says.

Not run

  • Running any fixture, the Windows loader, or a real Hopper or Ghidra analysis.
  • rea doctor. I called the built Ghidra install check with a throwaway folder instead. The Windows rules used a stand-in host with the same 4,096-byte probe.
  • MSVC link.exe. The PR's MSVC repro is not verified by me.
  • The full npm run check, lint, and a merge with the newest main.

Thanks, Aurantium-47, for the fix, the clear PR notes, and the quick follow-up on the review comment.

Also seen, not caused by this PR
  • A DOS-only file with a 64-byte header and 0x3c past the end is an error on base and head.
  • A normal-stub PE with its header past the end of the file is an error on base and head.
  • A PE whose header overlaps the DOS header (e_lfanew below 64) stays DOS on both.
Commands
# build (Node 24.18.0), once per tree: fa8e3b65b, 7711b0000, d33c9704a
npm ci && npm run build && npm run test:prepare
# fixtures: 46 from a stdlib python script, 5 from the exact #2172 recipe
python3 -I make_fixtures.py out
./make_lld_fixtures.sh lld
# CLI, each tree, each file (HOME isolated, no launcher)
rea inspect-artifact FILE --format json
rea inspect FILE --provider hopper --format json
rea inspect-go-binary FILE --format json
# tests, one Node at a time
npm run test:focused -- 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 tests/boundary/go/goBinaryMappings.test.ts

All of these exited 0. Two kinds of run exit 1 by design. One is a CLI run with no launcher or a rejected file. The other is the head tests on the base tree, with the 6 expected failures.

Test setup

🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊

Merge upstream main, preserve its header interpreter and consumers, and retain
short-stub PE coverage for Ghidra admission, artifact inventory and Go metadata.
Correct three ELF64 fixture lengths after upstream fixed-header validation.

Related to morluto#2172.

Assisted-by: OpenAI GPT-6 <noreply@openai.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@Aurantium-47 Aurantium-47 changed the title fix(targets): recognize PE images with short DOS stubs test(targets): cover bounded executable header readers Oct 13, 2026
@morluto morluto added the area: tests Test suite and verification label Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: tests Test suite and verification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants