From bef234d6ce63cd296a654400443fcdbbcd44d7de Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Fri, 9 Oct 2026 02:50:33 +0800 Subject: [PATCH] fix(ghidra): preserve Unicode boundaries without breaking sessions Validate native annotation text before mutation and escape surrogate code units in Java bridge JSON replies. Reject unsupported text with field and index diagnostics while preserving supported Unicode and line endings. Probe real CLI/MCP annotation, transport, mapping, rollback and cleanup behavior; replace seven redundant unit tests with E2E and SDK boundary coverage. --- bridge/ghidra/ReaGhidraBridge.java | 29 ++++- docs/native-investigation.md | 5 +- docs/testing.md | 8 +- scripts/lib/real-ghidra-boundaries.mjs | 114 +++++++++++++++-- src/ghidra/GhidraFunctionValues.test.ts | 46 ------- src/ghidra/GhidraInventoryValues.test.ts | 116 +----------------- src/ghidra/GhidraProviderCapabilities.ts | 1 + tests/boundary/mcp/ghidraInventoryMcp.test.ts | 77 ++++++++++++ 8 files changed, 222 insertions(+), 174 deletions(-) create mode 100644 tests/boundary/mcp/ghidraInventoryMcp.test.ts diff --git a/bridge/ghidra/ReaGhidraBridge.java b/bridge/ghidra/ReaGhidraBridge.java index ef11ecec3..0fa3895a6 100644 --- a/bridge/ghidra/ReaGhidraBridge.java +++ b/bridge/ghidra/ReaGhidraBridge.java @@ -1007,6 +1007,9 @@ public final class ReaGhidraBridge extends HeadlessScript { Address entry = function.getEntryPoint(); if (function.isExternal() || !currentProgram.getMemory().contains(entry)) throw new RequestFailure("invalid_request", "Annotations require a local function entry: " + canonicalAddress(entry)); + for (String field : List.of("name", "comment", "inline_comment")) { + if (params.has(field)) validateAnnotationText(requireText(params, field), field); + } int transaction = currentProgram.startTransaction("REA function annotations"); boolean commit = false; try { @@ -1065,6 +1068,21 @@ public final class ReaGhidraBridge extends HeadlessScript { return value.getAsString(); } + private static void validateAnnotationText(String text, String field) { + for (int index = 0; index < text.length(); index++) { + char character = text.charAt(index); + if (character == 0) + throw new RequestFailure("invalid_request", "Annotation " + field + " contains NUL at UTF-16 index " + index + "; Ghidra's decompiler cannot marshal NUL text"); + if (Character.isHighSurrogate(character) && index + 1 < text.length() && + Character.isLowSurrogate(text.charAt(index + 1))) { + index++; + continue; + } + if (Character.isSurrogate(character)) + throw new RequestFailure("invalid_request", "Annotation " + field + " contains an unpaired Unicode surrogate at UTF-16 index " + index + "; Ghidra cannot preserve it as annotation text"); + } + } + private JsonObject analyzeFunction(JsonObject params) throws Exception { requireKeys(params, Set.of("procedure")); Function function = resolveProcedure(requireString(params, "procedure")); @@ -3228,7 +3246,16 @@ public final class ReaGhidraBridge extends HeadlessScript { private static void writeResponse(BufferedWriter writer, JsonObject response) throws IOException { String encoded = GSON.toJson(response); - writer.write(encoded); + // Gson leaves UTF-16 surrogates literal. Escape them in JSON so even + // diagnostics quoting ill-formed caller text remain lossless UTF-8. + int start = 0; + for (int index = 0; index < encoded.length(); index++) { + if (!Character.isSurrogate(encoded.charAt(index))) continue; + writer.write(encoded, start, index - start); + writer.write("\\u" + Integer.toHexString(encoded.charAt(index))); + start = index + 1; + } + writer.write(encoded, start, encoded.length() - start); writer.newLine(); writer.flush(); } diff --git a/docs/native-investigation.md b/docs/native-investigation.md index 533c8c787..4f61375bf 100644 --- a/docs/native-investigation.md +++ b/docs/native-investigation.md @@ -257,7 +257,10 @@ protected runtime DACLs, and handle-based path admission. See [Windows Ghidra P0 [issue #527](https://github.com/morluto/rea/issues/527). On Linux and macOS, `annotate_native_function` atomically edits a function name and entry comments in the ephemeral database, returning refreshed analysis -without changing executable bytes. Windows P0 remains read-only. Ghidra has no +without changing executable bytes. Annotation text must contain no NUL or +unpaired Unicode surrogate; a rejection identifies the field and UTF-16 index +and leaves every annotation unchanged. CRLF, supplementary Unicode characters, +and combining characters are preserved. Windows P0 remains read-only. Ghidra has no GUI authority, and REA never falls back automatically to Hopper. - `npm run verify:ghidra`: host-native debug/stripped targets, native type layout, diff --git a/docs/testing.md b/docs/testing.md index 6f3b53ff5..8221305c3 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -249,8 +249,12 @@ direct versus targetless calls, byte-read completeness, invalid input diagnostic CLI/MCP parity, atomic annotation rollback, refreshed inventories, unchanged executable bytes, and discarded edits after reopen. A deliberately long temporary path exercises private Unix socket allocation and cleanup, including cancellation -after a real headless process launches. Malformed annotation -readback remains a separate SDK/provider integration case; success from a real +after a real headless process launches. Native annotation probes reject NUL and +unpaired Unicode surrogates without partial edits or a broken bridge, preserve +supported Unicode and control text, and check lossless malformed-text diagnostics. +Memory-to-file mapping is checked against original artifact bytes. Malformed +annotation readback, memory completeness, and inventory data remain separate +SDK/provider integration cases; success from a real provider cannot establish rejection of a contradictory provider response. The Linux switch lane checks dense, sparse-with-holes, shared-body, nonzero, diff --git a/scripts/lib/real-ghidra-boundaries.mjs b/scripts/lib/real-ghidra-boundaries.mjs index 309eb1a24..eb8986c54 100644 --- a/scripts/lib/real-ghidra-boundaries.mjs +++ b/scripts/lib/real-ghidra-boundaries.mjs @@ -97,6 +97,16 @@ export async function verifyGhidraBoundaries( assert.equal(baseline.status, "decoded"); const bytes = await call("read_bytes", { address, length: baseline.length }); assert.equal(bytes.bytes_hex, baseline.bytes); + assert.equal((await call("read_bytes", { address })).requested_bytes, 256); + const mapped = await call("address_to_file_offset", { address }); + assert.equal(mapped.address, address); + assert.equal( + (await readFile(target.path)) + .subarray(mapped.file_offset, mapped.file_offset + baseline.length) + .toString("hex"), + bytes.bytes_hex, + "Memory-to-file mapping must identify the bytes in the caller's artifact", + ); const largeRead = await call("read_bytes", { address, length: Number.MAX_SAFE_INTEGER, @@ -182,6 +192,28 @@ export async function verifyGhidraBoundaries( } const strings = await call("list_strings"); assert.ok(strings.length > 0); + assert.deepEqual( + await call("list_strings", { address: strings[0].address }), + [strings[0]], + ); + const marker = strings.find( + (item) => item.value === "REA_GHIDRA_INVENTORY_ENTRY", + ); + assert.ok(marker); + assert.ok( + ( + await call("search_strings", { pattern: marker.value.toLowerCase() }) + ).some((item) => item.address === marker.address), + ); + assert.equal( + ( + await call("search_strings", { + pattern: marker.value.toLowerCase(), + case_sensitive: true, + }) + ).some((item) => item.address === marker.address), + false, + ); assert.equal( (await call("inspect_native_instruction", { address: strings[0].address })) .status, @@ -218,6 +250,26 @@ export async function verifyGhidraBoundaries( { document: "rea_missing_program" }, /rea_missing_program.*active program/u, ); + // JSON permits escaped lone surrogates. Error replies must preserve them + // without letting the Java UTF-8 writer terminate the provider session. + await invalid( + "procedure_address", + { procedure: "missing_\ud800" }, + /missing_\\ud800/u, + ); + await invalid( + "list_procedures", + { document: "missing_\udfff" }, + /missing_\\udfff/u, + ); + for (const [operation, args] of [ + ["read_bytes", { address, length: 0 }], + ["list_procedures", { limit: 100 }], + ["list_documents", { extra: true }], + ["xrefs", {}], + ["procedure_info", { procedure: address, extra: true }], + ]) + await invalid(operation, args); for (const changes of [ {}, { name: "" }, @@ -296,6 +348,46 @@ export async function verifyGhidraBoundaries( ).some((item) => item.address === address), ); + for (const field of ["name", "comment", "inline_comment"]) { + for (const text of ["bad\0text", "bad\ud800text", "bad\udffftext"]) { + await invalid( + "annotate_native_function", + { + procedure: address, + name: "MUST_ROLL_BACK", + comment: "MUST ROLL BACK", + inline_comment: "MUST ROLL BACK", + [field]: text, + }, + new RegExp( + `Annotation ${field}.*(?:NUL|unpaired Unicode surrogate).*index`, + "u", + ), + ); + assert.deepEqual( + ( + await call("annotate_native_function", { + procedure: address, + name: changes.name, + }) + ).annotations, + updated.annotations, + "Invalid native text must leave every annotation unchanged", + ); + } + } + const unicodeText = + "CRLF\r\nastral 🧪 decomposed e\u0301 control \u0001\b\v\f\u001f noncharacter \ufffe\uffff"; + const unicode = await call("annotate_native_function", { + procedure: address, + comment: unicodeText, + inline_comment: unicodeText, + }); + assert.equal(unicode.annotations.comment, unicodeText); + assert.equal(unicode.annotations.inline_comment, unicodeText); + assert.ok(unicode.dossier.comments.some((item) => item.text === unicodeText)); + await call("annotate_native_function", changes); + await invalid( "annotate_native_function", { @@ -339,15 +431,17 @@ export async function verifyGhidraBoundaries( ]); assert.deepEqual(cliUpdated.annotations, updated.annotations); assert.deepEqual(cliUpdated.effects, updated.effects); - await call("annotate_native_function", { - procedure: address, - name: "0xordinary", - }); - assert.equal( - await call("procedure_address", { procedure: "0xordinary" }), - address, - ); - assert.equal(await call("address_name", { address }), "0xordinary"); + for (const renamed of ["0xordinary", "probe::qualified", "🧪probe"]) { + await call("annotate_native_function", { + procedure: address, + name: renamed, + }); + assert.equal( + await call("procedure_address", { procedure: renamed }), + address, + ); + assert.equal(await call("address_name", { address }), renamed); + } await call("close_binary"); await assert.rejects(access(socketRoot), { code: "ENOENT" }); await assert.rejects(access(runtimeRoot), { code: "ENOENT" }); @@ -372,6 +466,8 @@ export async function verifyGhidraBoundaries( rejected_calls: rejectedCalls, cli_mcp_parity: true, mutation_rollback: true, + annotation_native_text_validation: true, + lossless_unicode_transport: true, source_immutable: true, reopen_discards_edits: true, long_tmpdir_private_socket_cleanup: true, diff --git a/src/ghidra/GhidraFunctionValues.test.ts b/src/ghidra/GhidraFunctionValues.test.ts index f50a3759a..d93f22f2b 100644 --- a/src/ghidra/GhidraFunctionValues.test.ts +++ b/src/ghidra/GhidraFunctionValues.test.ts @@ -3,7 +3,6 @@ import { describe, expect, it } from "vitest"; import type { JsonValue } from "../domain/jsonValue.js"; import { functionDossierSchema } from "../domain/hopperValues.js"; import { - parseGhidraFunctionInput, parseGhidraFunctionResult, type GhidraFunctionOperation, } from "./GhidraFunctionValues.js"; @@ -16,51 +15,6 @@ import { ghidraReferenceEdge, } from "../domain/ghidraValues.fixture.js"; -describe("Ghidra function-analysis boundary values", () => { - it("defaults inputs and rejects undeclared or implicit addresses", () => { - expect( - parseGhidraFunctionInput("procedure_info", { procedure: "main" }), - ).toEqual({ - ok: true, - value: { document: null, procedure: "main" }, - }); - expect( - parseGhidraFunctionInput("procedure_references", { procedure: "main" }), - ).toEqual({ - ok: true, - value: { - document: null, - procedure: "main", - direction: "outgoing", - }, - }); - expect( - parseGhidraFunctionInput("analyze_function", { procedure: "main" }), - ).toMatchObject({ - ok: true, - value: { procedure: "main" }, - }); - expect( - parseGhidraFunctionInput("read_function_instructions", { - procedure: "main", - }), - ).toEqual({ - ok: true, - value: { document: null, procedure: "main" }, - }); - expect(parseGhidraFunctionInput("xrefs", {})).toMatchObject({ - ok: false, - error: { _tag: "AnalysisInputError" }, - }); - expect( - parseGhidraFunctionInput("procedure_info", { - procedure: "main", - extra: true, - }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - }); -}); - describe("Ghidra function-analysis result values", () => { it("parses provider-classified function facts and reference kinds", () => { expect( diff --git a/src/ghidra/GhidraInventoryValues.test.ts b/src/ghidra/GhidraInventoryValues.test.ts index c0ebcfa37..8a882e27b 100644 --- a/src/ghidra/GhidraInventoryValues.test.ts +++ b/src/ghidra/GhidraInventoryValues.test.ts @@ -1,120 +1,6 @@ import { describe, expect, it } from "vitest"; -import { - isGhidraInventoryOperation, - parseGhidraInventoryInput, - parseGhidraInventoryResult, -} from "./GhidraInventoryValues.js"; - -describe("Ghidra inventory boundary values", () => { - it("admits explicit byte reads and source offset mapping without invented caps", () => { - expect( - parseGhidraInventoryInput("read_bytes", { address: "0x10000" }), - ).toEqual({ - ok: true, - value: { address: "0x10000", document: null, length: 256 }, - }); - expect( - parseGhidraInventoryInput("read_bytes", { - address: "0x10000", - length: 4097, - }).ok, - ).toBe(true); - expect( - parseGhidraInventoryInput("read_bytes", { address: "0x10000", length: 0 }) - .ok, - ).toBe(false); - expect( - parseGhidraInventoryInput("address_to_file_offset", { - address: "HEADER:0x1", - }).ok, - ).toBe(true); - }); - it("checks byte length and completeness before accepting memory Evidence", () => { - const result = { - address: "0x10000", - requested_bytes: 4, - returned_bytes: 2, - bytes_hex: "0410", - complete: false, - }; - expect(parseGhidraInventoryResult("read_bytes", result)).toEqual({ - ok: true, - value: result, - }); - for (const change of [ - { complete: true }, - { returned_bytes: 4 }, - { bytes_hex: "04" }, - { requested_bytes: 1 }, - ]) - expect( - parseGhidraInventoryResult("read_bytes", { ...result, ...change }).ok, - ).toBe(false); - expect( - parseGhidraInventoryResult("address_to_file_offset", { - address: "0x10000", - file_offset: -1, - }).ok, - ).toBe(false); - }); - it("accepts complete inventories and searches without page controls", () => { - expect(parseGhidraInventoryInput("list_procedures", {})).toEqual({ - ok: true, - value: { document: null }, - }); - expect( - parseGhidraInventoryInput("search_strings", { pattern: "needle" }), - ).toEqual({ - ok: true, - value: { - pattern: "needle", - mode: "literal", - case_sensitive: false, - document: null, - }, - }); - expect( - parseGhidraInventoryInput("list_procedures", { limit: 100 }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - expect( - parseGhidraInventoryInput("list_documents", { extra: true }), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisInputError" } }); - }); - - it("keeps the admitted operation set closed", () => { - expect(isGhidraInventoryOperation("list_names")).toBe(true); - expect(isGhidraInventoryOperation("goto_address")).toBe(false); - expect(isGhidraInventoryOperation("set_comment")).toBe(false); - }); - - it("returns complete typed inventory arrays", () => { - const procedures = Array.from({ length: 700 }, (_, index) => ({ - address: `0x${(0x401000 + index).toString(16)}`, - value: `sub_${index}`, - procedure: { external: false, thunk: false, thunk_target: null }, - })); - expect(parseGhidraInventoryResult("list_procedures", procedures)).toEqual({ - ok: true, - value: procedures, - }); - }); - - it("rejects malformed inventory items", () => { - expect( - parseGhidraInventoryResult("list_procedures", [ - { - address: "00401000", - value: "main", - procedure: { external: false, thunk: false, thunk_target: null }, - }, - ]), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisOutputError" } }); - expect( - parseGhidraInventoryResult("search_strings", [{ address: "0x1000" }]), - ).toMatchObject({ ok: false, error: { _tag: "AnalysisOutputError" } }); - }); -}); +import { parseGhidraInventoryResult } from "./GhidraInventoryValues.js"; // Wire-boundary fixture only; real-provider acceptance belongs to the DOS verifier. describe("containing-procedure complete body evidence", () => { diff --git a/src/ghidra/GhidraProviderCapabilities.ts b/src/ghidra/GhidraProviderCapabilities.ts index 402f6d1e8..2d3170083 100644 --- a/src/ghidra/GhidraProviderCapabilities.ts +++ b/src/ghidra/GhidraProviderCapabilities.ts @@ -46,6 +46,7 @@ export const limitationsFor = (operation: string): readonly string[] => { return [ ...common, "Names use Ghidra USER_DEFINED source. Name writes preserve the existing namespace; readback uses the fully qualified name. Regular comments map to PRE and inline comments to EOL at the exact function entry. Changes commit together after readback and refreshed analysis; failure rolls them all back.", + "Annotation text rejects NUL and unpaired Unicode surrogates before mutation, with the field and UTF-16 index in the error. Supported Unicode and line endings are preserved.", "Metadata edits invalidate immutable analysis snapshots and are discarded on close. CLI returns the updated dossier before session cleanup; this is not a saved Ghidra project.", ]; case "inspect_native_load_image": diff --git a/tests/boundary/mcp/ghidraInventoryMcp.test.ts b/tests/boundary/mcp/ghidraInventoryMcp.test.ts new file mode 100644 index 000000000..dc3d600c8 --- /dev/null +++ b/tests/boundary/mcp/ghidraInventoryMcp.test.ts @@ -0,0 +1,77 @@ +import { expect, it } from "vitest"; +import { jsonValueSchema } from "../../../src/domain/jsonValue.js"; +import { ok } from "../../../src/domain/result.js"; +import { connectGhidraMcp } from "./ghidraMcpHarness.js"; + +it("rejects contradictory provider memory and inventory data before emitting MCP Evidence", async () => { + const bytes = { + address: "0x401000", + requested_bytes: 4, + returned_bytes: 2, + bytes_hex: "0410", + complete: false, + }; + let output = jsonValueSchema.parse(bytes); + const harness = await connectGhidraMcp("ghidra-invalid-inventory", () => + Promise.resolve(ok(output)), + ); + try { + const accepted = await harness.mcp.callTool({ + name: "read_bytes", + arguments: { address: bytes.address, length: 4 }, + }); + expect(accepted.isError).not.toBe(true); + expect(accepted.structuredContent).toMatchObject({ result: bytes }); + const cases = [ + ...[ + { complete: true }, + { returned_bytes: 4 }, + { bytes_hex: "04" }, + { requested_bytes: 1 }, + ].map((change, index) => ({ + name: "read_bytes", + arguments: { + address: `0x${(0x401004 + index * 4).toString(16)}`, + length: 4, + }, + output: { ...bytes, ...change }, + })), + { + name: "address_to_file_offset", + arguments: { address: bytes.address }, + output: { address: bytes.address, file_offset: -1 }, + }, + { + name: "list_procedures", + arguments: {}, + output: [ + { + address: "00401000", + value: "main", + procedure: { external: false, thunk: false, thunk_target: null }, + }, + ], + }, + { + name: "search_strings", + arguments: { pattern: "needle" }, + output: [{ address: "0x401000" }], + }, + ]; + for (const probe of cases) { + output = jsonValueSchema.parse(probe.output); + const rejected = await harness.mcp.callTool({ + name: probe.name, + arguments: probe.arguments, + }); + expect(rejected.isError, probe.name).toBe(true); + expect(rejected.structuredContent).toMatchObject({ + error: { code: "unreadable_output" }, + }); + expect(rejected.structuredContent).not.toHaveProperty("evidence_id"); + expect(rejected.structuredContent).not.toHaveProperty("result"); + } + } finally { + await harness.close(); + } +});