morluto/rea

Fork 0

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

[GH-ISSUE #4676] [Bug] Ghidra executable header inspection loses failed-close ownership #2435

New issue

Closed

opened 2026-10-17 21:04:37 +00:00 by bsakaguchi · 6 comments

bsakaguchi commented

2026-10-17 21:04:37 +00:00

Owner

Originally created by @morluto on GitHub (Oct 17, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/4676

Area

Provider or capability discovery

Observed behavior

Source-only finding at 5112fe07ae44cc99cb47641d50279efb5595ea71: the system Ghidra executable-header reader stores a numeric descriptor in a local variable and closes it in finally. A close rejection escapes with no retained descriptor owner. Read failures are swallowed as undefined; a single native read can return a short prefix without completing the requested coverage.

Reproduction

Inspected src/ghidra/GhidraInstallation.ts, systemGhidraInstallationHost().executableHeader, and its installation/doctor consumers. NOT_ENOUGH_INFO: no native OS close fault or real Ghidra reproduction. Candidate controls open real temporary files and inject close/read faults, short reads, replacement, growth, shrink, disappearance, and cancellation during final close. These are deterministic boundary controls, not real-provider verification.

Expected behavior

Follow AGENTS.md and docs/tool-design.md ownership rules: preserve the physical failure or completed prefix, retain exact failed-close ownership, and retry that descriptor after path replacement without reopening. Complete the bounded prefix and validate its extent/identity. Keep ordinary unreadable compatibility unknown; propagate cancellation and unresolved cleanup.

Impact and scope

Source-derived discovery and doctor resource-lifecycle defect. Native frequency and real-engine impact: NOT_ENOUGH_INFO. Separate from #4584 configured seed/extension readers and #4672 application properties.

REA version

6.5.0; main 5112fe07ae44cc99cb47641d50279efb5595ea71.

Runtime and platform

Controlled validation: Node 24.18.0/npm 11.16.0, Darwin arm64. Windows/Linux native behavior unverified.

Target and artifact context

Disposable synthetic regular files and a selected symlink; no target execution, JVM, or Ghidra engine.

Interface

Installation discovery and doctor; shared provider boundary.

Earliest failure stage

Native decompiler header inspection before provider startup.

Relevant output or evidence

Published reader: finally { if (handle !== undefined) closeSync(handle); } has no transfer to FileCloseResourceOwner. Candidate regression path: tests/boundary/filesystem/ghidraExecutableHeader.test.ts; final-close controls assert retained resource, failed retry, replacement of pathname, exact-owner successful retry, and resolution of the original cleanup marker.

Typed outcome or recovery expected

Explicit unknown header facts for ordinary read failures; typed cancellation with the exact opaque cause and completed-prefix summary; incomplete cleanup gates admission and remains retryable by the exact owner.

Analysis or suspected cause

The optional compatibility probe owns a descriptor without participating in the installation resource owner. Proposed structural fix: asynchronous owned regular-file prefix reading, shared stable extent/identity validation, and one retained observation consumed by installation and doctor.

Additional context

Open/closed search for Ghidra executable header cleanup found #4584/#4672 and trackers, no focused header report. No heavy profiling or stress tests were started.

Originally created by @morluto on GitHub (Oct 17, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/4676 ### Area Provider or capability discovery ### Observed behavior Source-only finding at `5112fe07ae44cc99cb47641d50279efb5595ea71`: the system Ghidra executable-header reader stores a numeric descriptor in a local variable and closes it in `finally`. A close rejection escapes with no retained descriptor owner. Read failures are swallowed as `undefined`; a single native read can return a short prefix without completing the requested coverage. ### Reproduction Inspected `src/ghidra/GhidraInstallation.ts`, `systemGhidraInstallationHost().executableHeader`, and its installation/doctor consumers. NOT_ENOUGH_INFO: no native OS close fault or real Ghidra reproduction. Candidate controls open real temporary files and inject close/read faults, short reads, replacement, growth, shrink, disappearance, and cancellation during final close. These are deterministic boundary controls, not real-provider verification. ### Expected behavior Follow AGENTS.md and docs/tool-design.md ownership rules: preserve the physical failure or completed prefix, retain exact failed-close ownership, and retry that descriptor after path replacement without reopening. Complete the bounded prefix and validate its extent/identity. Keep ordinary unreadable compatibility unknown; propagate cancellation and unresolved cleanup. ### Impact and scope Source-derived discovery and doctor resource-lifecycle defect. Native frequency and real-engine impact: NOT_ENOUGH_INFO. Separate from #4584 configured seed/extension readers and #4672 application properties. ### REA version 6.5.0; main `5112fe07ae44cc99cb47641d50279efb5595ea71`. ### Runtime and platform Controlled validation: Node 24.18.0/npm 11.16.0, Darwin arm64. Windows/Linux native behavior unverified. ### Target and artifact context Disposable synthetic regular files and a selected symlink; no target execution, JVM, or Ghidra engine. ### Interface Installation discovery and doctor; shared provider boundary. ### Earliest failure stage Native decompiler header inspection before provider startup. ### Relevant output or evidence Published reader: `finally { if (handle !== undefined) closeSync(handle); }` has no transfer to `FileCloseResourceOwner`. Candidate regression path: `tests/boundary/filesystem/ghidraExecutableHeader.test.ts`; final-close controls assert retained resource, failed retry, replacement of pathname, exact-owner successful retry, and resolution of the original cleanup marker. ### Typed outcome or recovery expected Explicit unknown header facts for ordinary read failures; typed cancellation with the exact opaque cause and completed-prefix summary; incomplete cleanup gates admission and remains retryable by the exact owner. ### Analysis or suspected cause The optional compatibility probe owns a descriptor without participating in the installation resource owner. Proposed structural fix: asynchronous owned regular-file prefix reading, shared stable extent/identity validation, and one retained observation consumed by installation and doctor. ### Additional context Open/closed search for `Ghidra executable header cleanup` found #4584/#4672 and trackers, no focused header report. No heavy profiling or stress tests were started.

bsakaguchi added the

triage

bug

labels

2026-10-17 21:04:37 +00:00

bsakaguchi commented

2026-10-17 21:04:38 +00:00

Author

Owner

@morluto commented on GitHub (Oct 17, 2026):

Published on main in e31aa72c9 (fix(ghidra): retain executable header read and cleanup ownership).

Root cause: the optional header probe owned a local numeric descriptor outside the installation resource owner. It swallowed read failure and could lose the still-open descriptor when final close threw.

A controlled compiled baseline at 5112fe07 reproduces this: injecting a real temporary descriptor's read failure and close refusal returns only controlled final close refusal, retains zero owners, and leaves the descriptor open. The control closes the exact descriptor afterward; no Ghidra/JVM ran. This upgrades the original source-only finding with a deterministic fault control, not a native OS fault reproduction.

The fix uses the existing regular-file lease, completes at most 4096 prefix bytes through short reads, and validates descriptor/name state while retaining full extent separately. Installation consumes one observation. Ordinary failures remain explicit unknown compatibility in installation/doctor diagnostics; cancellation and incomplete cleanup propagate. Final-close controls preserve exact opaque reasons and completed prefix facts or physical primary failure; refused cleanup can fail once more, then retry the original descriptor after pathname replacement without reopening.

Validation executed: 269 focused tests across eight files (header/metadata/profile file ownership, regular-file admission, installation, doctor, provider and public CLI Java doctor), npm run check:fast, and npm run docs:check including five conformance tests. Normal commit/push hooks passed.

Completed header task, 8192-byte private fixture, clean baseline/candidate, two alternating warm filesystem repeats: elapsed 59.88/61.00ms, CPU 52.34/53.17ms, sampled process-family RSS 61.64/61.57MB, cl100k_base reply tokens 59/59. Separate first processes recorded; caches not flushed. CPU uses wait4, RSS sampling can miss transients/reparenting. Whole-agent, MCP/catalog/retries, public doctor cost, cancellation cost and real-engine performance remain unmeasured. No performance improvement claim, profiler or stress workload.

Real Ghidra/JVM and Windows/Linux native close behavior remain unverified. Broader launcher precedence #4517 is separate and remains pending.

@morluto commented on GitHub (Oct 17, 2026): Published on main in e31aa72c9 (`fix(ghidra): retain executable header read and cleanup ownership`). Root cause: the optional header probe owned a local numeric descriptor outside the installation resource owner. It swallowed read failure and could lose the still-open descriptor when final close threw. A controlled compiled baseline at 5112fe07 reproduces this: injecting a real temporary descriptor's read failure and close refusal returns only `controlled final close refusal`, retains zero owners, and leaves the descriptor open. The control closes the exact descriptor afterward; no Ghidra/JVM ran. This upgrades the original source-only finding with a deterministic fault control, not a native OS fault reproduction. The fix uses the existing regular-file lease, completes at most 4096 prefix bytes through short reads, and validates descriptor/name state while retaining full extent separately. Installation consumes one observation. Ordinary failures remain explicit unknown compatibility in installation/doctor diagnostics; cancellation and incomplete cleanup propagate. Final-close controls preserve exact opaque reasons and completed prefix facts or physical primary failure; refused cleanup can fail once more, then retry the original descriptor after pathname replacement without reopening. Validation executed: 269 focused tests across eight files (header/metadata/profile file ownership, regular-file admission, installation, doctor, provider and public CLI Java doctor), `npm run check:fast`, and `npm run docs:check` including five conformance tests. Normal commit/push hooks passed. Completed header task, 8192-byte private fixture, clean baseline/candidate, two alternating warm filesystem repeats: elapsed 59.88/61.00ms, CPU 52.34/53.17ms, sampled process-family RSS 61.64/61.57MB, cl100k_base reply tokens 59/59. Separate first processes recorded; caches not flushed. CPU uses wait4, RSS sampling can miss transients/reparenting. Whole-agent, MCP/catalog/retries, public doctor cost, cancellation cost and real-engine performance remain unmeasured. No performance improvement claim, profiler or stress workload. Real Ghidra/JVM and Windows/Linux native close behavior remain unverified. Broader launcher precedence #4517 is separate and remains pending.

bsakaguchi commented

2026-10-17 21:04:38 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 17, 2026):

Agreed on the root cause: the finally { closeSync(handle) } pattern cannot transfer a failed close to FileCloseResourceOwner, so a close rejection is dropped exactly when diagnostics need it most, and swallowing read failures as undefined makes a short prefix indistinguishable from EOF. The fix commit direction looks right (retain read + cleanup ownership, retry the descriptor after path replacement). Two suggestions worth adding: (1) when a native read returns fewer bytes than requested, surface it as an explicit incomplete-prefix diagnostic instead of undefined, so doctor output can tell truncation from empty; (2) this same finally-close-without-transfer pattern likely appears in the sibling provider readers (#4674 shows the returned-vs-thrown attribution variant) — a shared regression test that injects a close fault and asserts the descriptor owner survives would cover both.

@XinwangTao commented on GitHub (Oct 17, 2026): Agreed on the root cause: the finally { closeSync(handle) } pattern cannot transfer a failed close to FileCloseResourceOwner, so a close rejection is dropped exactly when diagnostics need it most, and swallowing read failures as undefined makes a short prefix indistinguishable from EOF. The fix commit direction looks right (retain read + cleanup ownership, retry the descriptor after path replacement). Two suggestions worth adding: (1) when a native read returns fewer bytes than requested, surface it as an explicit incomplete-prefix diagnostic instead of undefined, so doctor output can tell truncation from empty; (2) this same finally-close-without-transfer pattern likely appears in the sibling provider readers (#4674 shows the returned-vs-thrown attribution variant) — a shared regression test that injects a close fault and asserts the descriptor owner survives would cover both.

bsakaguchi commented

2026-10-17 21:04:38 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 17, 2026):

Agreed on the root cause: the finally { closeSync(handle) } pattern cannot transfer a failed close to FileCloseResourceOwner, so a close rejection is dropped exactly when diagnostics need it most, and swallowing read failures as undefined makes a short prefix indistinguishable from EOF. The fix in e31aa72 looks right (retain read + cleanup ownership, retry the descriptor after path replacement). Two suggestions worth adding: (1) when a native read returns fewer bytes than requested, surface it as an explicit incomplete-prefix diagnostic instead of undefined, so doctor output can tell truncation from empty; (2) this same finally-close-without-transfer pattern likely appears in the sibling provider readers (#4674 shows the returned-vs-thrown attribution variant) — a shared regression test that injects a close fault and asserts the descriptor owner survives would cover both.

@XinwangTao commented on GitHub (Oct 17, 2026): Agreed on the root cause: the finally { closeSync(handle) } pattern cannot transfer a failed close to FileCloseResourceOwner, so a close rejection is dropped exactly when diagnostics need it most, and swallowing read failures as undefined makes a short prefix indistinguishable from EOF. The fix in e31aa72 looks right (retain read + cleanup ownership, retry the descriptor after path replacement). Two suggestions worth adding: (1) when a native read returns fewer bytes than requested, surface it as an explicit incomplete-prefix diagnostic instead of undefined, so doctor output can tell truncation from empty; (2) this same finally-close-without-transfer pattern likely appears in the sibling provider readers (#4674 shows the returned-vs-thrown attribution variant) — a shared regression test that injects a close fault and asserts the descriptor owner survives would cover both.

bsakaguchi closed this issue

2026-10-31 00:33:52 +00:00

bsakaguchi commented

2026-10-31 00:33:54 +00:00

Author

Owner

@morluto commented on GitHub (Oct 17, 2026):

Revalidated on current main d818c3d2639d3115edb8a5269a754101aea6c3bd; the published fix e31aa72c9 is an ancestor and the original local numeric-descriptor reader is gone.

The owner now uses the shared artifact regular-file lease and stable bounded-prefix reader. Its current regressions complete the bounded prefix through short native reads, reject changed extent/identity, preserve completed-header or physical failure facts during final-close cancellation, and retry the original denied-close descriptor after pathname replacement without reopening. Ghidra installation and doctor consume the typed observation; unreadable/truncated/unrecognized header compatibility remains unknown.

Executed 186 tests across nine focused files: header ownership, skill identity/cleanup, doctor lifecycle, Ghidra installation/doctor, public Java-doctor CLI and unqualified skill-doctor CLI. No source changes were needed for this issue. This establishes the reported controlled lifecycle/short-read contract; native Windows/Linux close faults and a real Ghidra/JVM/target workflow remain unverified. Closing as fixed, rather than leaving an already-published fix in the open bug queue.

@morluto commented on GitHub (Oct 17, 2026): Revalidated on current main `d818c3d2639d3115edb8a5269a754101aea6c3bd`; the published fix `e31aa72c9` is an ancestor and the original local numeric-descriptor reader is gone. The owner now uses the shared artifact regular-file lease and stable bounded-prefix reader. Its current regressions complete the bounded prefix through short native reads, reject changed extent/identity, preserve completed-header or physical failure facts during final-close cancellation, and retry the original denied-close descriptor after pathname replacement without reopening. Ghidra installation and doctor consume the typed observation; unreadable/truncated/unrecognized header compatibility remains unknown. Executed 186 tests across nine focused files: header ownership, skill identity/cleanup, doctor lifecycle, Ghidra installation/doctor, public Java-doctor CLI and unqualified skill-doctor CLI. No source changes were needed for this issue. This establishes the reported controlled lifecycle/short-read contract; native Windows/Linux close faults and a real Ghidra/JVM/target workflow remain unverified. Closing as fixed, rather than leaving an already-published fix in the open bug queue.

bsakaguchi commented

2026-10-31 00:33:54 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 18, 2026):

Revalidation on d818c3d confirms the fix closes the loop — the shared artifact regular-file lease plus bounded-prefix reader now completes short native reads, rejects changed extent/identity, and retains cleanup ownership across final-close cancellation, which covers both suggestions from my earlier comment. Looks good to close out.

@XinwangTao commented on GitHub (Oct 18, 2026): Revalidation on d818c3d confirms the fix closes the loop — the shared artifact regular-file lease plus bounded-prefix reader now completes short native reads, rejects changed extent/identity, and retains cleanup ownership across final-close cancellation, which covers both suggestions from my earlier comment. Looks good to close out.

bsakaguchi commented

2026-10-31 00:33:55 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 18, 2026):

@XinwangTao commented on GitHub (Oct 18, 2026): Revalidation on d818c3d confirms the fix closes the loop — the shared artifact regular-file lease plus bounded-prefix reader now completes short native reads, rejects changed extent/identity, and retains cleanup ownership across final-close cancellation, which covers both suggestions from my earlier comment. Looks good to close out.

bsakaguchi referenced this issue

2026-10-31 00:40:20 +00:00

[GH-ISSUE #5654] [Tracker] Open PRs that can't merge yet: conflicts and red CI (October 2026) #3425

bsakaguchi referenced this issue

2026-10-31 01:04:58 +00:00

[PR #2435] feat(managed): bind Portable PDB sequence points to selected methods #5226