Skip to content

feat(ghidra): disable named auto-analyzers via REA_GHIDRA_DISABLED_ANALYZERS - #1800

Open
akushonkamen wants to merge 2 commits into
morluto:mainfrom
akushonkamen:fix/1783-ghidra-analyzer-overrides
Open

akushonkamen wants to merge 2 commits into
morluto:mainfrom
akushonkamen:fix/1783-ghidra-analyzer-overrides

Conversation

@akushonkamen

Copy link
Copy Markdown
Contributor

Summary

Fixes #1783. Ghidra's default auto-analysis can stall indefinitely in a single analyzer (ObjcMessageAnalyzer on a 1.85 MB Swift/ObjC arm64 Mach-O), and REA gave the caller no way to skip it and no record of what ran: the analysis profile always said analyzer_preset: "ghidra-default". This adds REA_GHIDRA_DISABLED_ANALYZERS, a comma-separated list of exact analyzer names that are disabled via a pre-analysis script before default auto-analysis, recorded in the analysis profile so snapshots and Evidence stay bound to the analysis that actually ran.

Problem and expected behavior

src/ghidra/GhidraLauncher.ts's ghidraHeadlessArguments only injected -preScript for the DOS COM loader branch and seed files — there was no channel for analyzer options (verified on main @ ce845ba4: interface 371–386, argv builder 388–468; the DOS COM -preScript sits at what is now 446–449 after unrelated main-side churn, content unchanged), and src/ghidra/GhidraAnalysisProfile.ts:152 hardcodes analyzer_preset: "ghidra-default" with no configurable analyzer field. The issue's out-of-REA controls show the same binary analyzing to completion in 76 s with one analyzer turned off, versus a 300 s analysis timeout with defaults — so a supported skip is the fix. Expected: a caller can name analyzers to disable for a session, and the profile records the override.

Change and scope

  • New packaged pre-script bridge/ghidra/ReaGhidraAnalysisOptions.java (modeled on the existing ReaGhidraPrepareCom precedent): requires exactly one comma-separated argument and calls setAnalysisOption(currentProgram, "<name>", "false") for each name.
  • New src/config/ghidraDisabledAnalyzers.ts parses REA_GHIDRA_DISABLED_ANALYZERS: comma-separated, blank entries dropped, at most 32 names of at most 120 chars, each matching ^[A-Za-z0-9][A-Za-z0-9 ._-]*$ (a name can neither start with - nor contain a comma — Ghidra's headless parser would eat those as its own options). Anything else fails config admission.
  • Plumbing: src/config/environment.ts:43 → src/config/parseConfig.ts:91-93 → src/config/types.ts:17 → src/ghidra/GhidraProvider.ts:195 → src/ghidra/GhidraProviderClient.ts → src/ghidra/GhidraLauncher.ts:468-475, where the -preScript is injected after the seed script and before the post-analysis repair/bridge postScripts.
  • The analysis profile records the override truthfully: analyzer_preset: "ghidra-default+disabled:<names>" plus structured analysis_options.disabled_analyzers (src/ghidra/GhidraAnalysisProfile.ts). Mirroring the language-override binding, ghidraProfileDisabledAnalyzers reads the commitment back and a session fails closed (refuses to open) when its committed profile and the active setting disagree.
  • docs/installation.md documents the setting, its name rules, and that disabling an analyzer skips the facts it would have produced.
  • Not included: a general analyzer-preset selector, deadline diagnostics on timeout (the issue's second suggested remedy), and any change to default behavior — with the setting unset, argv is byte-identical to before (covered by test).

Contract and boundary impact

  • Semantic owner and earliest changed stage: Ghidra headless launcher argv construction and analysis-profile resolution (src/ghidra/GhidraLauncher.ts, src/ghidra/GhidraAnalysisProfile.ts).
  • CLI and MCP/tool-catalog contract: none; the setting is environment/config-only, no CLI flag or MCP field added.
  • Provider, bridge, target-format, or platform compatibility: new packaged bridge script bridge/ghidra/ReaGhidraAnalysisOptions.java (added to package.json files and the packaged-Windows verifier manifest); no change to existing scripts.
  • Evidence, artifact, provenance, or reconstruction contract: the analysis profile now records analyzer_preset: "ghidra-default+disabled:<names>" and analysis_options.disabled_analyzers, and session open fails closed on profile/setting mismatch, so snapshots stay bound to the analysis that ran.
  • Process execution, authorization, cleanup, or containment impact: one additional Ghidra pre-script in the same bridge directory under the existing -scriptPath; no new processes or cleanup paths.
  • Generated metadata, package, or installation impact: package.json ships the new bridge file; docs/installation.md documents the setting. No committed generated outputs change.

Evidence and regression coverage

  • Tests added or updated:
    • src/config/ghidraDisabledAnalyzers.ts covered by new src/ghidra/GhidraDisabledAnalyzers.test.ts (10 tests: parsing, limits, name rules).
    • tests/boundary/providers/ghidra/ghidraLauncher.test.ts: argv injection, ordering after seed / before -postScript, byte-identical argv when unset (3 new tests).
    • src/ghidra/GhidraProvider.behavior.test.ts: profile recording, fail-closed binding, end-to-end provider threading (3 new tests).
    • scripts/verify-packaged-ghidra-windows.mjs covers the new pre-script in the packaged Windows verifier manifest (second commit).
  • Base reproduction: after rebasing onto main @ 40d9c7b8 (7 commits advanced since the branch point ce845ba4, clean rebase), the red state was reproduced by restoring all non-test source files to origin/main while keeping the new tests, then:
    • timeout 300 npx vitest run tests/boundary/providers/ghidra/ghidraLauncher.test.ts — 3 failed | 16 passed | 1 skipped.
    • timeout 300 npx vitest run src/ghidra/GhidraDisabledAnalyzers.test.ts — Error: Cannot find module '../config/ghidraDisabledAnalyzers.js'.
    • timeout 300 npx vitest run src/ghidra/GhidraProvider.behavior.test.ts -t "disabled analyzer" — 3 failed | 40 skipped.
      All pass with the fix restored (numbers under Validation performed).
  • User-visible CLI/MCP output (if applicable): none by default; with REA_GHIDRA_DISABLED_ANALYZERS="Objective-C Message Analyzer" the session's profile carries analyzer_preset: "ghidra-default+disabled:Objective-C Message Analyzer".
  • Remaining proof gaps: real Ghidra was not exercised — this machine has no Ghidra installation (which analyzeHeadless → not found; no ~/ghidra*, /opt/ghidra*, /Applications/ghidra*; GHIDRA_INSTALL_DIR empty), so the Ghidra 12.1.4 stall reproduction and the disable-one-analyzer 76 s control come from the issue report's out-of-REA analyzeHeadless runs, as stated in the commit message.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Rebased onto origin/main @ 40d9c7b8 (branch point ce845ba4), then, on macOS arm64 / Node 24.14.0 — red state first as listed above, then with the fix restored:

  • timeout 300 npx vitest run tests/boundary/providers/ghidra/ghidraLauncher.test.ts — 19 passed | 1 skipped.
  • timeout 300 npx vitest run src/ghidra/GhidraDisabledAnalyzers.test.ts — 10 passed.
  • timeout 300 npx vitest run src/ghidra/GhidraProvider.behavior.test.ts — 43 passed (full file, incl. the 3 new tests).
  • timeout 300 npx vitest run src/ghidra/GhidraDosComLoadImage.test.ts — 13 passed.
  • timeout 300 npx vitest run src/config.test.ts src/ghidra/GhidraLanguageOverride.test.ts src/ghidra/GhidraAnalysisSeeds.test.ts src/ghidra/GhidraLauncherEnvironment.test.ts — 76 passed.
  • timeout 300 npx vitest run tests/boundary/providers/ghidra/ — 54 passed | 1 skipped.
  • timeout 300 npx vitest run tests/boundary/mcp/ghidraEvidenceMcp.test.ts — 4 passed.
  • timeout 570 npx vitest run src/ghidra/ — 17 files, 314 passed.
  • npm run check:fast — 2 successful (turbo typecheck + lint, lint includes verify:module-boundaries).
  • npm run verify:test-discovery — "Discovered all 823 test files exactly once."
  • npm run build:cached — exit 0; then node scripts/check-doc-facts.mjs — exit 0.
  • node scripts/generate-package-metadata.mjs --check, node scripts/generate-skill-metadata.mjs --check, node scripts/generate-product-catalog.mjs --check, node scripts/generate-completion-ledger.mjs --check, node scripts/generate-error-schema.mjs --check — each exit 0. (completion-ledger's first run failed on the git-ignored docs/verification/managed-conformance-manifest.json; npm run docs:generate regenerated it, after which the check passes. Working tree stayed clean.)
  • npm run verify:ghidra (real Ghidra e2e) — not run: no Ghidra installation on this machine (see Remaining proof gaps).

Compatibility, safety, and release

  • Breaking changes or migration steps: none. With the setting unset, launcher argv is byte-identical to before and the profile records "ghidra-default" as before.
  • Real Hopper/Ghidra, browser, or OS coverage: real Ghidra not available here; the report's Ghidra 12.1.4 controls cover the runtime behavior of the pre-script mechanism (setAnalysisOption in a pre-script) outside REA. Unit/boundary coverage covers everything REA adds around it.
  • Package or release metadata impact: bridge/ghidra/ReaGhidraAnalysisOptions.java added to the packaged file list and the packaged-Windows verifier manifest.
  • Security, privacy, process, or containment review: analyzer names are strictly whitelisted (length, count, character set) and injected as a single pre-script argument into the existing bridge -scriptPath; no shell interpolation, no new scripts sourced from user paths, no change to process cleanup.

Review checklist

  • This PR addresses a concrete problem or an agreed enhancement.
  • 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.

Disclosure: this contribution is prepared with AI assistance (Claude), reviewed and verified by me (same disclosure as the issue claim comment).

…ALYZERS

Default auto-analysis can stall in a single analyzer, such as
ObjcMessageAnalyzer on a Swift/Objective-C arm64 Mach-O, and a caller
had no way around it: the headless launcher passed -preScript only for
DOS COM preparation and seed files, and the analysis profile always
recorded analyzer_preset "ghidra-default" (morluto#1783).

REA_GHIDRA_DISABLED_ANALYZERS now names comma-separated Ghidra
auto-analyzers that a pre-analysis script disables before default
auto-analysis, following the same packaged pre-script precedent. The
analysis profile records the override in analyzer_preset and
analysis_options, and a session refuses to open when its committed
profile and the setting differ, so snapshots and Evidence stay bound
to the analysis that actually ran.

Validation: new tests for the setting parser, launcher argv injection
and ordering, and profile recording plus fail-closed profile binding;
existing launcher, DOS COM, seeds, language-override, and Ghidra MCP
evidence coverage rerun green (314 src/ghidra module tests, 54 ghidra
boundary tests, check:fast, verify:test-discovery, generated-file
checks). Real Ghidra was not exercised: this machine has no Ghidra
installation; the stall reproduction and the disable-one-analyzer
control come from the issue report's out-of-repo analyzeHeadless runs
on Ghidra 12.1.4.
…ndows verifier

The caller-cwd shadowing verifier enumerated exactly the bridge scripts
the launcher passes as arguments, but missed the new
ReaGhidraAnalysisOptions.java preScript, leaving the new script outside
the shadowing coverage and the caller-cwd preservation assertion.

Sorting the list is required, not cosmetic: the assertion compares
readdir(callerDirectory).sort() against it, so the pre-sort order
introduced in 162f9bf and af88184 could never match on any platform
(reproduced with a local readdir+sort probe; the lane itself requires a
Windows x64 host and was not run here). Also documents the 120-character
single-name limit next to the 32-name cap in the installation guide.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 11, 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 ⚠️ Failed 2026-10-11T11:26:37.891480Z f9cc9e6 PR opened
ℹ️ 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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Ghidra provider times out on a 1.85 MB Swift/ObjC arm64 Mach-O: auto-analysis stalls in ObjcMessageAnalyzer and cannot be skipped

1 participant