morluto/rea

Fork 0

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

[PR #2867] [CLOSED] fix(ghidra): bound language definition reads before allocation #5451

New issue

Closed

opened 2026-10-31 01:06:45 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:06:45 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2867
Author: @yasshmittal
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/ghidra-ldefs-size-before-read

📝 Commits (7)

cb15708 fix(ghidra): bound language definition reads before allocation
27cf91f fix(ghidra): retain language file cleanup ownership
c06c69f fix(ghidra): keep catalog cleanup retryable and visible
208a1b2 fix(ghidra): report each catalog close failure and synchronize its test
b76ce56 fix(ghidra): retry retained catalog cleanup at catalog entry
ac832ff fix(ghidra): retain unconfirmed language file owners
8dab92c test(registry): keep unconfirmed-cleanup selection in its own suite

📊 Changes

6 files changed (+448 additions, -12 deletions)

View changed files

📝 src/application/binary/AnalysisProviderRegistry.test.ts (+33 -0)
📝 src/application/binary/AnalysisProviderRegistry.ts (+4 -0)
📝 src/ghidra/GhidraLanguageCatalog.ts (+92 -3)
📝 src/ghidra/GhidraProvider.ts (+30 -9)
➕ tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts (+86 -0)
➕ tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts (+203 -0)

📄 Description

Summary

Fixes #2531.

The Ghidra language catalog read each .ldefs file completely with readFile and only then compared its length with the 4 MiB limit. An oversized file was therefore buffered in full before being skipped, so the limit did not bound the allocation it exists to bound.

Problem and expected behavior

readGhidraLanguageCatalog (src/ghidra/GhidraLanguageCatalog.ts) runs when a configured REA_GHIDRA_LANGUAGE_ID is admitted. An installation or extension file larger than MAX_LDEFS_BYTES should be skipped without allocating more than the limit; a small valid sibling should still load.

Change and scope

Each definition is read through the shared readBoundedFileBytes helper (src/process/BoundedFileBytes.ts, already used by GhidraClient). It reads in bounded chunks, stops after the limit plus one byte, and reports overflow; the catalog keeps its existing rule of skipping oversized files. Because the bound is applied while reading rather than from a prior stat, a file that grows during the read is bounded too. The handle is always closed.

Not included: reporting skipped oversized files as a catalog limitation, which would be a contract change beyond this fix.

This builds on the merged #2772: the bounded bytes are still decoded with TextDecoder, so UTF-8 signed definitions keep loading.

Contribution scope: an ordinary fix within existing boundaries; no maintainer scope decision applies.

Contract and boundary impact

Semantic owner and earliest changed stage: .ldefs file read in src/ghidra/GhidraLanguageCatalog.ts.
Contribution scope fit and linked maintainer decision: not applicable (bug fix).
CLI and MCP/tool-catalog contract: unchanged.
Provider, bridge, target-format, or platform compatibility: unchanged results; peak memory for oversized definitions is now bounded.
Evidence, artifact, provenance, or reconstruction contract: unchanged.
Target selection, authority, upstream behavior, and cleanup details: read-only; no Ghidra launch.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added or updated: tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts.
- An 8 MiB definition beside a small sibling: the sibling loads, the oversized file is skipped, and the bytes read from it are at most 4 MiB + 1.
- A definition of exactly 4 MiB is still accepted.
Base reproduction or other evidence: on the base revision the first test fails with expected 8388688 to be less than or equal to 4194305, so the whole 8 MiB file was buffered.
User-visible CLI/MCP output: none.
Remaining proof gaps: no real Ghidra installation run (not required for this read path).
Performance (review request): Windows 11 x64, Node 24.21.0, three fresh processes per revision reading a 256 MiB oversized definition beside a small sibling.

Revision Peak RSS (MiB) Elapsed (ms) CPU (ms)

main (readFile) 309–343 195–273 172–328

this PR 57–89 9–32 0–31

Both revisions return the same catalog. About 55 MiB of the PR figure is the process and loader baseline. Agent-token cost is unchanged.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Revision	Peak RSS (MiB)	Elapsed (ms)	CPU (ms)
`main` (`readFile`)	309–343	195–273	172–328
this PR	57–89	9–32	0–31

Validation performed

Host: Windows 11, Node.js 24.21.0.

npx vitest run --project boundary tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts: 2 passed (fails on the base revision as above).
npm run test:local -- src/ghidra/GhidraLanguageOverride.test.ts: all catalog tests pass. Four profile tests in this file fail identically on the unmodified base revision on Windows and are unrelated.
npm run check, npm run verify:test-discovery, npm run verify:module-boundaries: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: Windows only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: tightens a resource bound; no new I/O beyond the existing file read.

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.

🤖 Generated with Claude Code

_{🔄 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/2867 **Author:** [@yasshmittal](https://github.com/yasshmittal) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/ghidra-ldefs-size-before-read` --- ### 📝 Commits (7) - [`cb15708`](https://github.com/morluto/rea/commit/cb157084aab65c66192991416de732bd411d7af9) fix(ghidra): bound language definition reads before allocation - [`27cf91f`](https://github.com/morluto/rea/commit/27cf91f9e2d719fe53aff236814606d77902d4ac) fix(ghidra): retain language file cleanup ownership - [`c06c69f`](https://github.com/morluto/rea/commit/c06c69fe0a4eaf0a53930b887ef5d935308208f8) fix(ghidra): keep catalog cleanup retryable and visible - [`208a1b2`](https://github.com/morluto/rea/commit/208a1b2a68ef84c056397192816ead4eae3965d0) fix(ghidra): report each catalog close failure and synchronize its test - [`b76ce56`](https://github.com/morluto/rea/commit/b76ce56e91db623e6f47048ef12ab4e5a574e48f) fix(ghidra): retry retained catalog cleanup at catalog entry - [`ac832ff`](https://github.com/morluto/rea/commit/ac832ff277692ae6c8db0a701aa7459f63c93a7b) fix(ghidra): retain unconfirmed language file owners - [`8dab92c`](https://github.com/morluto/rea/commit/8dab92caa6a576e121251726477794b2c4124803) test(registry): keep unconfirmed-cleanup selection in its own suite ### 📊 Changes **6 files changed** (+448 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `src/application/binary/AnalysisProviderRegistry.test.ts` (+33 -0) 📝 `src/application/binary/AnalysisProviderRegistry.ts` (+4 -0) 📝 `src/ghidra/GhidraLanguageCatalog.ts` (+92 -3) 📝 `src/ghidra/GhidraProvider.ts` (+30 -9) ➕ `tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts` (+86 -0) ➕ `tests/boundary/filesystem/ghidraLanguageCatalogCleanup.test.ts` (+203 -0) </details> ### 📄 Description ## Summary Fixes #2531. The Ghidra language catalog read each `.ldefs` file completely with `readFile` and only then compared its length with the 4 MiB limit. An oversized file was therefore buffered in full before being skipped, so the limit did not bound the allocation it exists to bound. ## Problem and expected behavior `readGhidraLanguageCatalog` (`src/ghidra/GhidraLanguageCatalog.ts`) runs when a configured `REA_GHIDRA_LANGUAGE_ID` is admitted. An installation or extension file larger than `MAX_LDEFS_BYTES` should be skipped without allocating more than the limit; a small valid sibling should still load. ## Change and scope Each definition is read through the shared `readBoundedFileBytes` helper (`src/process/BoundedFileBytes.ts`, already used by `GhidraClient`). It reads in bounded chunks, stops after the limit plus one byte, and reports overflow; the catalog keeps its existing rule of skipping oversized files. Because the bound is applied while reading rather than from a prior `stat`, a file that grows during the read is bounded too. The handle is always closed. Not included: reporting skipped oversized files as a catalog limitation, which would be a contract change beyond this fix. This builds on the merged #2772: the bounded bytes are still decoded with `TextDecoder`, so UTF-8 signed definitions keep loading. Contribution scope: an ordinary fix within existing boundaries; no maintainer scope decision applies. ## Contract and boundary impact - Semantic owner and earliest changed stage: `.ldefs` file read in `src/ghidra/GhidraLanguageCatalog.ts`. - Contribution scope fit and linked maintainer decision: not applicable (bug fix). - CLI and MCP/tool-catalog contract: unchanged. - Provider, bridge, target-format, or platform compatibility: unchanged results; peak memory for oversized definitions is now bounded. - Evidence, artifact, provenance, or reconstruction contract: unchanged. - Target selection, authority, upstream behavior, and cleanup details: read-only; no Ghidra launch. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts`. - An 8 MiB definition beside a small sibling: the sibling loads, the oversized file is skipped, and the bytes read from it are at most `4 MiB + 1`. - A definition of exactly 4 MiB is still accepted. - Base reproduction or other evidence: on the base revision the first test fails with `expected 8388688 to be less than or equal to 4194305`, so the whole 8 MiB file was buffered. - User-visible CLI/MCP output: none. - Remaining proof gaps: no real Ghidra installation run (not required for this read path). - Performance (review request): Windows 11 x64, Node 24.21.0, three fresh processes per revision reading a 256 MiB oversized definition beside a small sibling. | Revision | Peak RSS (MiB) | Elapsed (ms) | CPU (ms) | | --- | --- | --- | --- | | `main` (`readFile`) | 309–343 | 195–273 | 172–328 | | this PR | 57–89 | 9–32 | 0–31 | Both revisions return the same catalog. About 55 MiB of the PR figure is the process and loader baseline. Agent-token cost is unchanged. - [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 Host: Windows 11, Node.js 24.21.0. - `npx vitest run --project boundary tests/boundary/filesystem/ghidraLanguageCatalogBound.test.ts`: 2 passed (fails on the base revision as above). - `npm run test:local -- src/ghidra/GhidraLanguageOverride.test.ts`: all catalog tests pass. Four profile tests in this file fail identically on the unmodified base revision on Windows and are unrelated. - `npm run check`, `npm run verify:test-discovery`, `npm run verify:module-boundaries`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Windows only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: tightens a resource bound; no new I/O beyond the existing file read. ## 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:06:45 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns