morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[GH-ISSUE #3179] [Bug] Ghidra doctor passes version validation for malformed continued properties beginning with comment markers #1117

New issue

Closed

opened 2026-10-15 18:31:13 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-15 18:31:13 +00:00

Owner

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

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Provider or capability discovery

Observed behavior

rea doctor --provider ghidra --format json silently ignores a continued property when a backslash-only physical line precedes a line starting with # or !. In the reproduced metadata-only fixtures, a malformed Unicode escape in that continued property's value left providerVersion at 12.1.4 and the installation version check at ok: true. The ordinary escaped-key control containing the same malformed escape instead cleared the version and returned version_unresolved with a malformed-Unicode diagnostic.

The overall CLI exit status was 1 for every fixture because the fixtures omit headless/native/JDK prerequisites. The incorrect result is the passing version check and retained metadata, not full provider availability.

Reproduction

Start in a clean checkout of 58300885207133422af235e0f2b10658cf958b03 using the pinned development toolchain. Prepare the runtime:

npm ci
npm run build:cached

From the checkout root, run the following probe. It creates all inputs inline, isolates each CLI invocation's HOME and XDG configuration directories, and uses an absolute GHIDRA_INSTALL_DIR. The first two cases put a malformed Unicode escape in continuation data. The other cases provide an escaped-key failure control, a genuine comment, and a corrected escape.

node --input-type=module <<'JS'
import { mkdir, writeFile } from 'node:fs/promises';
import { resolve } from 'node:path';
import { spawnSync } from 'node:child_process';
const root = resolve('repro-state');
const trialHome = resolve(root, 'home');
await mkdir(trialHome, { recursive: true });
const trialEnvironment = {
  ...process.env,
  HOME: trialHome,
  XDG_CONFIG_HOME: resolve(trialHome, 'config'),
  XDG_CACHE_HOME: resolve(trialHome, 'cache'),
  XDG_DATA_HOME: resolve(trialHome, 'data'),
};
const prefix = 'application.version=12.1.4\napplication.java.min=21\n';
const cases = {
  continued_hash: '\\\n#unused=\\uZZZZ\n',
  continued_bang: '\\\n!unused=\\uZZZZ\n',
  escaped_hash: '\\#unused=\\uZZZZ\n',
  real_comment: '#unused=\\uZZZZ\n',
  corrected: '\\\n#unused=\\u0041\n',
};
for (const [name, suffix] of Object.entries(cases)) {
  const install = resolve(root, name);
  await mkdir(resolve(install, 'Ghidra'), { recursive: true });
  await writeFile(resolve(install, 'Ghidra/application.properties'), prefix + suffix, 'latin1');
  const run = spawnSync(process.execPath, ['scripts/rea.mjs', 'doctor', '--provider', 'ghidra', '--format', 'json'], {
    encoding: 'utf8', env: { ...trialEnvironment, GHIDRA_INSTALL_DIR: install },
  });
  if (run.error) throw run.error;
  const result = JSON.parse(run.stdout);
  const ghidra = result.providerInspections.find(provider => provider.id === 'ghidra');
  const version = ghidra.checks.find(check => check.name === 'version');
  console.log(JSON.stringify({ name, exit: run.status, providerVersion: ghidra.providerVersion, version: { ok: version.ok, code: version.code, detail: version.detail } }));
}
JS

Compare each fixture's version result. continued_hash and continued_bang incorrectly pass with the version retained, while escaped_hash reports the malformed-Unicode error. Read the individual version check rather than the overall exit status, since these are intentionally incomplete installation fixtures.

The verifier executed these CLI checks against the prepared baseline runtime. Clean-checkout installation and build commands are prerequisites; the verifier did not rerun them.

Expected behavior

src/ghidra/GhidraJavaProperties.ts:3 declares Java Properties.load(InputStream) syntax. Its continuation handling at src/ghidra/GhidraJavaProperties.ts:60 identifies comments before joining continuation data. src/ghidra/GhidraInstallationPolicy.ts:54 explicitly states that a malformed Java properties escape invalidates the entire file. The existing unused-property regression at src/ghidra/GhidraInstallation.test.ts:589 also requires installation and doctor rejection for malformed escapes outside the consumed metadata keys.

Both malformed continued cases should clear providerVersion and fail the version check with version_unresolved and the malformed-Unicode diagnostic, as the escaped-key control does. A comment marker in continuation data must not discard that property. A genuine physical comment may contain uninterpreted escape text and remain ignored.

Impact and scope

Impact is P2, supported by the reproduced incorrectly passing installation-version check and retained metadata from a file that should fail whole-file validation. The trigger is unusual caller-managed properties content. No data loss, security boundary crossing, full-provider admission, or real-engine failure was demonstrated.

An available workaround is to correct the malformed escape in the caller-owned file without removing the property. Replacing it with \u0041 was exercised and preserved the passing version check. Genuine comments also remained supported in the control.

REA version

6.4.0 (main at 5830088520)

Runtime and platform

Node 26.10.0; macOS arm64. The verifier did not record the macOS release number. No Java runtime was available.

Target and artifact context

Caller-owned Ghidra/application.properties fixtures containing application.version=12.1.4 and application.java.min=21, plus the inline continuation or control input. No analysis target is needed. These fixtures contain metadata only and do not represent complete Ghidra installations.

Interface

CLI

Earliest failure stage

Setup or provider admission

Relevant output or evidence

{"name":"continued_hash","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}}
{"name":"continued_bang","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}}
{"name":"escaped_hash","exit":1,"providerVersion":null,"version":{"ok":false,"code":"version_unresolved","detail":"application.properties could not be parsed: Malformed Unicode escape in Java properties file"}}
{"name":"real_comment","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}}
{"name":"corrected","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}}

Typed outcome or recovery expected

Repeat the same fixtures. continued_hash, continued_bang, and escaped_hash should each return null providerVersion, version.ok: false, version.code: "version_unresolved", and the malformed-Unicode diagnostic. real_comment and corrected should continue passing the version check.

Also preserve a valid continued marker-bearing property's key and value when checking parser semantics. Rejecting every such continuation would not restore the syntax contract. No full-provider success is required for this metadata-only check.

Analysis or suspected cause

Source-derived analysis at the reported baseline: logicalPropertyLines in src/ghidra/GhidraJavaProperties.ts:60 recognizes physical comments only outside continuation data, and src/ghidra/GhidraJavaProperties.ts:64 joins odd-backslash continuations. A backslash-only line therefore leaves the following marker-bearing line as property data. The second comment test at src/ghidra/GhidraJavaProperties.ts:11 then skips the joined logical line before key/value escape decoding at src/ghidra/GhidraJavaProperties.ts:32.

The CLI caller path is source-derived: src/cli/setupCommands.ts:124 invokes doctor through the system host, src/doctorRuntime.ts:28 invokes Ghidra inspection, and src/ghidra/GhidraDoctor.ts:62 forwards the explicit installation directory. src/ghidra/GhidraInstallation.ts:423 selects the properties file, src/ghidra/GhidraInstallation.ts:464 reads and parses it, and src/ghidra/GhidraInstallation.ts:781 builds the caller-visible version check. The declared parse-error handling at src/ghidra/GhidraInstallationPolicy.ts:64 clears the provider version and marks Java bounds unresolved when the parser returns an error.

Additional context

P2 applies because the reproduced failure changes metadata validation for bounded inputs, and correcting the escape is an exercised workaround. Verification covered CLI metadata checks and controls. A differential comparison with Java and a real Ghidra launch were not executed.

Originally created by @coygeek on GitHub (Oct 14, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/3179 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Provider or capability discovery ### Observed behavior `rea doctor --provider ghidra --format json` silently ignores a continued property when a backslash-only physical line precedes a line starting with `#` or `!`. In the reproduced metadata-only fixtures, a malformed Unicode escape in that continued property's value left `providerVersion` at `12.1.4` and the installation version check at `ok: true`. The ordinary escaped-key control containing the same malformed escape instead cleared the version and returned `version_unresolved` with a malformed-Unicode diagnostic. The overall CLI exit status was 1 for every fixture because the fixtures omit headless/native/JDK prerequisites. The incorrect result is the passing version check and retained metadata, not full provider availability. ### Reproduction 1. Start in a clean checkout of `58300885207133422af235e0f2b10658cf958b03` using the pinned development toolchain. Prepare the runtime: ```sh npm ci npm run build:cached ``` 2. From the checkout root, run the following probe. It creates all inputs inline, isolates each CLI invocation's HOME and XDG configuration directories, and uses an absolute `GHIDRA_INSTALL_DIR`. The first two cases put a malformed Unicode escape in continuation data. The other cases provide an escaped-key failure control, a genuine comment, and a corrected escape. ```sh node --input-type=module <<'JS' import { mkdir, writeFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { spawnSync } from 'node:child_process'; const root = resolve('repro-state'); const trialHome = resolve(root, 'home'); await mkdir(trialHome, { recursive: true }); const trialEnvironment = { ...process.env, HOME: trialHome, XDG_CONFIG_HOME: resolve(trialHome, 'config'), XDG_CACHE_HOME: resolve(trialHome, 'cache'), XDG_DATA_HOME: resolve(trialHome, 'data'), }; const prefix = 'application.version=12.1.4\napplication.java.min=21\n'; const cases = { continued_hash: '\\\n#unused=\\uZZZZ\n', continued_bang: '\\\n!unused=\\uZZZZ\n', escaped_hash: '\\#unused=\\uZZZZ\n', real_comment: '#unused=\\uZZZZ\n', corrected: '\\\n#unused=\\u0041\n', }; for (const [name, suffix] of Object.entries(cases)) { const install = resolve(root, name); await mkdir(resolve(install, 'Ghidra'), { recursive: true }); await writeFile(resolve(install, 'Ghidra/application.properties'), prefix + suffix, 'latin1'); const run = spawnSync(process.execPath, ['scripts/rea.mjs', 'doctor', '--provider', 'ghidra', '--format', 'json'], { encoding: 'utf8', env: { ...trialEnvironment, GHIDRA_INSTALL_DIR: install }, }); if (run.error) throw run.error; const result = JSON.parse(run.stdout); const ghidra = result.providerInspections.find(provider => provider.id === 'ghidra'); const version = ghidra.checks.find(check => check.name === 'version'); console.log(JSON.stringify({ name, exit: run.status, providerVersion: ghidra.providerVersion, version: { ok: version.ok, code: version.code, detail: version.detail } })); } JS ``` 3. Compare each fixture's `version` result. `continued_hash` and `continued_bang` incorrectly pass with the version retained, while `escaped_hash` reports the malformed-Unicode error. Read the individual version check rather than the overall exit status, since these are intentionally incomplete installation fixtures. The verifier executed these CLI checks against the prepared baseline runtime. Clean-checkout installation and build commands are prerequisites; the verifier did not rerun them. ### Expected behavior `src/ghidra/GhidraJavaProperties.ts:3` declares Java `Properties.load(InputStream)` syntax. Its continuation handling at `src/ghidra/GhidraJavaProperties.ts:60` identifies comments before joining continuation data. `src/ghidra/GhidraInstallationPolicy.ts:54` explicitly states that a malformed Java properties escape invalidates the entire file. The existing unused-property regression at `src/ghidra/GhidraInstallation.test.ts:589` also requires installation and doctor rejection for malformed escapes outside the consumed metadata keys. Both malformed continued cases should clear `providerVersion` and fail the version check with `version_unresolved` and the malformed-Unicode diagnostic, as the escaped-key control does. A comment marker in continuation data must not discard that property. A genuine physical comment may contain uninterpreted escape text and remain ignored. ### Impact and scope Impact is P2, supported by the reproduced incorrectly passing installation-version check and retained metadata from a file that should fail whole-file validation. The trigger is unusual caller-managed properties content. No data loss, security boundary crossing, full-provider admission, or real-engine failure was demonstrated. An available workaround is to correct the malformed escape in the caller-owned file without removing the property. Replacing it with `\u0041` was exercised and preserved the passing version check. Genuine comments also remained supported in the control. ### REA version 6.4.0 (main at 58300885207133422af235e0f2b10658cf958b03) ### Runtime and platform Node 26.10.0; macOS arm64. The verifier did not record the macOS release number. No Java runtime was available. ### Target and artifact context Caller-owned `Ghidra/application.properties` fixtures containing `application.version=12.1.4` and `application.java.min=21`, plus the inline continuation or control input. No analysis target is needed. These fixtures contain metadata only and do not represent complete Ghidra installations. ### Interface CLI ### Earliest failure stage Setup or provider admission ### Relevant output or evidence ```text {"name":"continued_hash","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}} {"name":"continued_bang","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}} {"name":"escaped_hash","exit":1,"providerVersion":null,"version":{"ok":false,"code":"version_unresolved","detail":"application.properties could not be parsed: Malformed Unicode escape in Java properties file"}} {"name":"real_comment","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}} {"name":"corrected","exit":1,"providerVersion":"12.1.4","version":{"ok":true,"code":null,"detail":"12.1.4"}} ``` ### Typed outcome or recovery expected Repeat the same fixtures. `continued_hash`, `continued_bang`, and `escaped_hash` should each return null `providerVersion`, `version.ok: false`, `version.code: "version_unresolved"`, and the malformed-Unicode diagnostic. `real_comment` and `corrected` should continue passing the version check. Also preserve a valid continued marker-bearing property's key and value when checking parser semantics. Rejecting every such continuation would not restore the syntax contract. No full-provider success is required for this metadata-only check. ### Analysis or suspected cause Source-derived analysis at the reported baseline: `logicalPropertyLines` in `src/ghidra/GhidraJavaProperties.ts:60` recognizes physical comments only outside continuation data, and `src/ghidra/GhidraJavaProperties.ts:64` joins odd-backslash continuations. A backslash-only line therefore leaves the following marker-bearing line as property data. The second comment test at `src/ghidra/GhidraJavaProperties.ts:11` then skips the joined logical line before key/value escape decoding at `src/ghidra/GhidraJavaProperties.ts:32`. The CLI caller path is source-derived: `src/cli/setupCommands.ts:124` invokes doctor through the system host, `src/doctorRuntime.ts:28` invokes Ghidra inspection, and `src/ghidra/GhidraDoctor.ts:62` forwards the explicit installation directory. `src/ghidra/GhidraInstallation.ts:423` selects the properties file, `src/ghidra/GhidraInstallation.ts:464` reads and parses it, and `src/ghidra/GhidraInstallation.ts:781` builds the caller-visible version check. The declared parse-error handling at `src/ghidra/GhidraInstallationPolicy.ts:64` clears the provider version and marks Java bounds unresolved when the parser returns an error. ### Additional context P2 applies because the reproduced failure changes metadata validation for bounded inputs, and correcting the escape is an exercised workaround. Verification covered CLI metadata checks and controls. A differential comparison with Java and a real Ghidra launch were not executed.

bsakaguchi

2026-10-15 18:31:13 +00:00

closed this issue
added the
bug
label

bsakaguchi commented

2026-10-15 18:31:14 +00:00

Author

Owner

@Sriram-PR commented on GitHub (Oct 14, 2026):

I looked at this before picking it up, and I don't think it's a bug: java.util.Properties.load treats these lines the same way the parser does.

I ran the five fixtures from the report (same prefix, same bytes) through Java 25 and through parseGhidraJavaProperties on current main:

case	`Properties.load` (Java 25)	`parseGhidraJavaProperties`
`continued_hash`	`{application.java.min=21, application.version=12.1.4}`	same two keys, no error
`continued_bang`	same	same
`escaped_hash`	`IllegalArgumentException: Malformed \uxxxx encoding.`	`Malformed Unicode escape` error
`real_comment`	`{application.java.min=21, application.version=12.1.4}`	same
`corrected`	`{application.java.min=21, application.version=12.1.4}` (no `#unused` key)	same

In Properties.LineReader.readLine, the trailing backslash is dropped at end of line (len -= 1, Properties.java#L597), and the comment test only asks whether anything is buffered yet (if (len == 0) { // Still on a new logical line then #/!, #L530). A line holding only \ leaves nothing buffered, so the next line's # or ! still starts a comment. That's why continued_hash/continued_bang produce no #unused key and no escape error in Java either. Ghidra's ApplicationProperties (it extends Properties) reads application.properties with load(InputStream), so the current doctor result (version kept, check passing) matches what Ghidra itself would see.

Java check, if anyone wants to rerun it (files written with the report's prefix + cases[name]):

import java.io.*; import java.nio.file.*; import java.util.*;
public class P { public static void main(String[] a) throws Exception {
  for (String k : new String[]{"continued_hash","continued_bang","escaped_hash","real_comment","corrected"}) {
    Properties p = new Properties(); String r;
    try (InputStream in = Files.newInputStream(Path.of(k + ".properties"))) { p.load(in); r = new TreeMap<>(p).toString(); }
    catch (Exception e) { r = "ERROR " + e.getClass().getSimpleName() + ": " + e.getMessage(); }
    System.out.println(k + " => " + r); } } }

So I'd suggest closing this as not a bug rather than changing the parser, since the change would make it disagree with Java.

@Sriram-PR commented on GitHub (Oct 14, 2026): I looked at this before picking it up, and I don't think it's a bug: `java.util.Properties.load` treats these lines the same way the parser does. I ran the five fixtures from the report (same prefix, same bytes) through Java 25 and through `parseGhidraJavaProperties` on current main: | case | `Properties.load` (Java 25) | `parseGhidraJavaProperties` | |---|---|---| | `continued_hash` | `{application.java.min=21, application.version=12.1.4}` | same two keys, no error | | `continued_bang` | same | same | | `escaped_hash` | `IllegalArgumentException: Malformed \uxxxx encoding.` | `Malformed Unicode escape` error | | `real_comment` | `{application.java.min=21, application.version=12.1.4}` | same | | `corrected` | `{application.java.min=21, application.version=12.1.4}` (no `#unused` key) | same | In `Properties.LineReader.readLine`, the trailing backslash is dropped at end of line (`len -= 1`, [Properties.java#L597](https://github.com/openjdk/jdk/blob/f8c6117ce526bf2275c1cdbd528f1d1277efa397/src/java.base/share/classes/java/util/Properties.java#L597)), and the comment test only asks whether anything is buffered yet (`if (len == 0) { // Still on a new logical line` then `#`/`!`, [#L530](https://github.com/openjdk/jdk/blob/f8c6117ce526bf2275c1cdbd528f1d1277efa397/src/java.base/share/classes/java/util/Properties.java#L530)). A line holding only `\` leaves nothing buffered, so the next line's `#` or `!` still starts a comment. That's why `continued_hash`/`continued_bang` produce no `#unused` key and no escape error in Java either. Ghidra's `ApplicationProperties` (it extends `Properties`) reads `application.properties` with `load(InputStream)`, so the current doctor result (version kept, check passing) matches what Ghidra itself would see. Java check, if anyone wants to rerun it (files written with the report's `prefix + cases[name]`): ```java import java.io.*; import java.nio.file.*; import java.util.*; public class P { public static void main(String[] a) throws Exception { for (String k : new String[]{"continued_hash","continued_bang","escaped_hash","real_comment","corrected"}) { Properties p = new Properties(); String r; try (InputStream in = Files.newInputStream(Path.of(k + ".properties"))) { p.load(in); r = new TreeMap<>(p).toString(); } catch (Exception e) { r = "ERROR " + e.getClass().getSimpleName() + ": " + e.getMessage(); } System.out.println(k + " => " + r); } } } ``` So I'd suggest closing this as not a bug rather than changing the parser, since the change would make it disagree with Java.

bsakaguchi commented

2026-10-15 18:31:14 +00:00

Author

Owner

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

Closing as expected Java Properties behavior rather than a REA validation defect. The exact five-fixture Java 25 comparison in https://github.com/morluto/rea/issues/3179#issuecomment-6169112199 matches the current parser, including the malformed Unicode rejection control. A line containing only a continuation backslash leaves no buffered content, so the following comment marker still starts a comment. Changing this would diverge from the producer semantics. Current parser/source context was checked; a fresh Java runtime comparison was not executed here.

@morluto commented on GitHub (Oct 14, 2026): Closing as expected Java Properties behavior rather than a REA validation defect. The exact five-fixture Java 25 comparison in https://github.com/morluto/rea/issues/3179#issuecomment-6169112199 matches the current parser, including the malformed Unicode rejection control. A line containing only a continuation backslash leaves no buffered content, so the following comment marker still starts a comment. Changing this would diverge from the producer semantics. Current parser/source context was checked; a fresh Java runtime comparison was not executed here.

bsakaguchi referenced this issue

2026-10-17 21:08:46 +00:00

[PR #1117] [MERGED] fix(setup): treat empty client config files as new documents #2662

bsakaguchi referenced this issue

2026-10-31 00:57:16 +00:00

[PR #1983] [MERGED] fix(setup): configure an empty Hermes config.yaml #4947