From 3ba1f6ef2201d53a3659c9667afbf837201a655d Mon Sep 17 00:00:00 2001 From: N0zoM1z0 <161784452+N0zoM1z0@users.noreply.github.com> Date: Wed, 14 Oct 2026 00:35:02 +0000 Subject: [PATCH] fix(ghidra): restore temporary verifier address aliases Own temporary address-name edits in the shared DOS/COM boundary verifier. Restore and read back the original public name even after an uncertain write; preserve both verification and restoration failures. Keep strict address overflow, collision and CLI/MCP parity assertions. Closes #2924 Co-authored-by: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> --- .../lib/real-ghidra-address-boundaries.d.mts | 8 ++ .../lib/real-ghidra-address-boundaries.mjs | 90 ++++++++---- .../ghidra/addressBoundaryRestoration.test.ts | 136 ++++++++++++++++++ 3 files changed, 205 insertions(+), 29 deletions(-) create mode 100644 scripts/lib/real-ghidra-address-boundaries.d.mts create mode 100644 tests/boundary/providers/ghidra/addressBoundaryRestoration.test.ts diff --git a/scripts/lib/real-ghidra-address-boundaries.d.mts b/scripts/lib/real-ghidra-address-boundaries.d.mts new file mode 100644 index 000000000..1376e2d14 --- /dev/null +++ b/scripts/lib/real-ghidra-address-boundaries.d.mts @@ -0,0 +1,8 @@ +import type { Client } from "@modelcontextprotocol/client"; + +/** Verify segmented aliases and restore the entry's observed public name. */ +export function verifyGhidraAddressBoundaries( + client: Pick, + entry: string, + cli: (command: string, address: string, flags: string[]) => Promise, +): Promise; diff --git a/scripts/lib/real-ghidra-address-boundaries.mjs b/scripts/lib/real-ghidra-address-boundaries.mjs index 0835ecec3..4e6deb23d 100644 --- a/scripts/lib/real-ghidra-address-boundaries.mjs +++ b/scripts/lib/real-ghidra-address-boundaries.mjs @@ -63,35 +63,67 @@ export async function verifyGhidraAddressBoundaries(client, entry, cli) { await query("read_bytes", { address: entry, length: 1 }), ); const aliasKeys = [entry, padded]; - assert.deepEqual( - await query("set_addresses_names", { - names: { - [entry]: "rea_same_address_alias", - [padded]: "rea_same_address_alias", - }, - }), - Object.fromEntries(aliasKeys.map((address) => [address, true])), - "Equivalent address spellings with one requested name share one edit", - ); - const nameBeforeAliasedBatch = await query("address_name", { - address: entry, - }); - assert.equal(nameBeforeAliasedBatch, "rea_same_address_alias"); - assert.deepEqual( - await query("set_addresses_names", { - names: { - [entry]: "rea_first_address_alias", - [padded]: "rea_second_address_alias", - }, - }), - Object.fromEntries(aliasKeys.map((address) => [address, false])), - "Conflicting spellings of one Ghidra address must be rejected together", - ); - assert.equal( - await query("address_name", { address: entry }), - nameBeforeAliasedBatch, - "A rejected alias collision must leave the preexisting target name unchanged", - ); + const originalName = await query("address_name", { address: entry }); + assert.equal(typeof originalName, "string", "Entry has no restorable name"); + const failures = []; + let restorationFailed = false; + try { + assert.deepEqual( + await query("set_addresses_names", { + names: { + [entry]: "rea_same_address_alias", + [padded]: "rea_same_address_alias", + }, + }), + Object.fromEntries(aliasKeys.map((address) => [address, true])), + "Equivalent address spellings with one requested name share one edit", + ); + assert.equal( + await query("address_name", { address: entry }), + "rea_same_address_alias", + ); + assert.deepEqual( + await query("set_addresses_names", { + names: { + [entry]: "rea_first_address_alias", + [padded]: "rea_second_address_alias", + }, + }), + Object.fromEntries(aliasKeys.map((address) => [address, false])), + "Conflicting spellings of one Ghidra address must be rejected together", + ); + assert.equal( + await query("address_name", { address: entry }), + "rea_same_address_alias", + "A rejected alias collision must leave the preexisting target name unchanged", + ); + } catch (cause) { + failures.push(cause); + } finally { + // Restore even when a failed reply leaves the write outcome uncertain. + // The public API restores the observed name, not private symbol provenance. + try { + assert.equal( + await query("set_address_name", { address: entry, name: originalName }), + true, + "Temporary address alias restoration was rejected", + ); + assert.equal( + await query("address_name", { address: entry }), + originalName, + "Temporary address alias restoration did not restore the observed name", + ); + } catch (cause) { + restorationFailed = true; + failures.push(cause); + } + } + if (restorationFailed) + throw new AggregateError( + failures, + "Ghidra address boundary verification failed to restore its temporary name", + ); + if (failures.length > 0) throw failures[0]; for (const [command, flags, operation] of [ ["read-bytes", ["--length", "1"], "read_bytes"], [ diff --git a/tests/boundary/providers/ghidra/addressBoundaryRestoration.test.ts b/tests/boundary/providers/ghidra/addressBoundaryRestoration.test.ts new file mode 100644 index 000000000..40139bceb --- /dev/null +++ b/tests/boundary/providers/ghidra/addressBoundaryRestoration.test.ts @@ -0,0 +1,136 @@ +import type { Client } from "@modelcontextprotocol/client"; +import { expect, it } from "vitest"; + +import { verifyGhidraAddressBoundaries } from "../../../../scripts/lib/real-ghidra-address-boundaries.mjs"; + +const entry = "0x10000"; +const originalName = "fixture::entry"; + +const fixture = ( + options: { + failAfterWrite?: Error; + restoration?: "reject" | "wrong-readback"; + initialName?: null; + } = {}, +) => { + let name: string | null = options.initialName === null ? null : originalName; + const writes: Record[] = []; + const success = (value: unknown) => ({ + content: [], + structuredContent: { normalized_result: value }, + }); + const error = (operation: string, address: unknown) => ({ + code: "invalid_request", + category: "invalid_input", + message: "Address cannot be represented without truncation", + retryable: false, + remediation: { action: "Select a representable address" }, + details: { + operation, + issues: [ + { address, message: "cannot be represented without truncation" }, + ], + }, + }); + const client: Pick = { + callTool: async ({ name: operation, arguments: args = {} }) => { + const address = args.address ?? args.procedure; + if (typeof address === "string" && address.includes("100010000")) + return { + isError: true, + content: [ + { + type: "text", + text: JSON.stringify({ error: error(operation, address) }), + }, + ], + }; + switch (operation) { + case "analyze_function": + return success({ procedure: { name } }); + case "read_bytes": + return success({ bytes_hex: "90" }); + case "address_name": + return success(name); + case "set_addresses_names": { + const names = args.names as Record; + writes.push(names); + const requested = Object.values(names); + const accepted = new Set(requested).size === 1; + if (accepted) { + name = requested[0]!; + if (options.failAfterWrite !== undefined) + throw options.failAfterWrite; + } + return success( + Object.fromEntries( + Object.keys(names).map((key) => [key, accepted]), + ), + ); + } + case "set_address_name": + writes.push(args); + if (options.restoration === "reject") return success(false); + name = + options.restoration === "wrong-readback" + ? "unexpected_name" + : String(args.name); + return success(true); + default: + throw new Error(`Unmodeled operation: ${operation}`); + } + }, + }; + const cli = async (command: string, address: string) => { + const operation = + command === "read-bytes" ? "read_bytes" : "annotate_native_function"; + throw Object.assign(new Error("Rejected oversized address"), { + code: 1, + stdout: JSON.stringify(error(operation, address)), + }); + }; + return { + run: () => verifyGhidraAddressBoundaries(client, entry, cli), + observedName: () => name, + writes, + }; +}; + +it("restores the exact observed name before the next verifier consumer", async () => { + const test = fixture(); + await test.run(); + expect(test.observedName()).toBe(originalName); +}); + +it("restores an uncertain write and preserves its original failure", async () => { + const failure = new Error("Reply lost after applying the alias"); + const test = fixture({ failAfterWrite: failure }); + await expect(test.run()).rejects.toBe(failure); + expect(test.observedName()).toBe(originalName); +}); + +it.each(["reject", "wrong-readback"] as const)( + "fails closed when restoration has %s", + async (restoration) => { + await expect(fixture({ restoration }).run()).rejects.toMatchObject({ + name: "AggregateError", + errors: [expect.any(Error)], + }); + }, +); + +it("retains both verification and restoration failures", async () => { + const failure = new Error("Reply lost after applying the alias"); + await expect( + fixture({ failAfterWrite: failure, restoration: "reject" }).run(), + ).rejects.toMatchObject({ + name: "AggregateError", + errors: [failure, expect.any(Error)], + }); +}); + +it("does not mutate an entry without a restorable public name", async () => { + const test = fixture({ initialName: null }); + await expect(test.run()).rejects.toThrow("Entry has no restorable name"); + expect(test.writes).toEqual([]); +});