Repository navigation
test(targets): cover bounded executable header readers - #2227
Aurantium-47 wants to merge 3 commits into
Conversation
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>
|
Brilliant! Thank you for this Aurantium |
1 similar comment
|
Brilliant! Thank you for this Aurantium |
There was a problem hiding this comment.
💡 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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>
|
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". ResultsI 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.
Not run
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
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.tsAll 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>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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
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
Known test effects:
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:
npm run checktask 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 bynode 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.tsandtests/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
Prepared and verified with OpenAI Codex assistance.