mirror of
https://github.com/morluto/rea.git
synced 2026-11-07 05:56:19 -10:00
fix(cli): report incomplete debugger cleanup as typed outcomes (#8021)
* fix(cli): report incomplete debugger cleanup as typed outcomes debug-with-gdb and debug-with-rizin hand-rolled final cleanup in try/finally blocks: a failed or interrupted final close only logged and assigned the process exit status, so callers still received complete successful Evidence with no typed error or partial observation. Both commands now route through the shared close owner used by the other one-shot CLI commands (deliverCliAnalysisEvidence / finishAnalysisCommand) via a small closeDebuggerCommandSession helper. A rejected closeAll projects as ProviderCleanupError, which the owner turns into cleanup_incomplete retaining the completed Evidence as partial observation; a signal observed after finalization projects as cancelled with the same retained observation. The per-session close failure still only logs, preserving the rule that the retained owner's closeAll decides completion. Fixes #7378. Fixes #7429. * test(cli): reject unmodeled GDB requests in cleanup regressions * test(cli): match modeled GDB requests exactly * Update tests/boundary/cli/gdbStartupCleanupCli.test.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --------- Co-authored-by: morluto <[email protected]> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
This commit is contained in:
3 files changed
+385
-118
No files matched your search
@@ -9,8 +9,9 @@ import {
|
||||
AnalysisOutputError,
|
||||
} from "../domain/analysisErrorCore.js";
|
||||
import { evidenceSchema, type Evidence } from "../domain/evidence.js";
|
||||
import type { JsonValue } from "../domain/jsonValue.js";
|
||||
import type { Result } from "../domain/result.js";
|
||||
import { err } from "../domain/result.js";
|
||||
import { err, ok } from "../domain/result.js";
|
||||
import { projectAnalysisError } from "../domain/analysisErrorProjection.js";
|
||||
import {
|
||||
OBJDUMP_ARTIFACT_OPERATIONS,
|
||||
@@ -26,11 +27,15 @@ import {
|
||||
gdbOneShotCommandMayInvalidateDetachProof,
|
||||
} from "../gdb/GdbSessionManager.js";
|
||||
import { RizinDebugSessionManager } from "../rizin/RizinDebugSessionManager.js";
|
||||
import { ProviderCleanupError } from "../domain/providerCleanupError.js";
|
||||
import {
|
||||
analysisEvidenceDeliveryOptions,
|
||||
cliEvidenceDeliveryInputError,
|
||||
deliverCliAnalysisEvidence,
|
||||
publishCliAnalysisEvidence,
|
||||
} from "./analysisEvidenceDelivery.js";
|
||||
import { finishAnalysisCommand } from "./finishAnalysisCommand.js";
|
||||
import { projectCliAnalysisResult } from "./projectCliAnalysisResult.js";
|
||||
|
||||
const executeWithCleanup = async (
|
||||
service: ReverseEngineeringService,
|
||||
@@ -117,6 +122,48 @@ const asCleanupError = (cause: unknown): AnalysisError =>
|
||||
{ cause },
|
||||
);
|
||||
|
||||
/** One-shot debugger cleanup: the retained owner's closeAll decides whether cleanup remains incomplete. */
|
||||
const closeDebuggerCommandSession = async (
|
||||
manager: {
|
||||
close(sessionId: string): Promise<Result<unknown, AnalysisError>>;
|
||||
closeAll(): Promise<void>;
|
||||
},
|
||||
sessionId: string | undefined,
|
||||
providerId: string,
|
||||
operation: string,
|
||||
log: (cleanup: JsonValue | string) => void,
|
||||
): Promise<void> => {
|
||||
if (sessionId !== undefined) {
|
||||
try {
|
||||
const closed = await manager.close(sessionId);
|
||||
if (!closed.ok) log(projectAnalysisError(closed.error));
|
||||
} catch (cause) {
|
||||
log(cause instanceof Error ? cause.message : "Unknown cleanup failure");
|
||||
}
|
||||
}
|
||||
try {
|
||||
await manager.closeAll();
|
||||
} catch (cause) {
|
||||
throw new ProviderCleanupError(
|
||||
providerId,
|
||||
sessionId === undefined ? [] : [sessionId],
|
||||
{
|
||||
reason:
|
||||
cause instanceof AggregateError
|
||||
? cause.errors
|
||||
.map((error: unknown) =>
|
||||
error instanceof Error ? error.message : String(error),
|
||||
)
|
||||
.join("; ")
|
||||
: cause instanceof Error
|
||||
? cause.message
|
||||
: String(cause),
|
||||
},
|
||||
{ cause, operation },
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
/** Add one-shot reverse-engineering commands using the shared service layer. */
|
||||
export const registerReverseEngineeringCommands = (
|
||||
cli: CliInstance,
|
||||
@@ -310,16 +357,13 @@ export const registerReverseEngineeringCommands = (
|
||||
);
|
||||
const manager = createGdbManager(environment);
|
||||
let sessionId: string | undefined;
|
||||
const completed = await (async () => {
|
||||
try {
|
||||
return deliverCliAnalysisEvidence({
|
||||
...delivery,
|
||||
run: async () => {
|
||||
const started = await manager.start(signal, {
|
||||
inferiorTty: options.inferiorTty,
|
||||
});
|
||||
if (!started.ok)
|
||||
return {
|
||||
ok: false as const,
|
||||
error: projectAnalysisError(started.error),
|
||||
};
|
||||
if (!started.ok) return err(started.error);
|
||||
sessionId = started.value.session_id;
|
||||
const executed = await manager.execute(
|
||||
sessionId,
|
||||
@@ -327,59 +371,24 @@ export const registerReverseEngineeringCommands = (
|
||||
signal,
|
||||
);
|
||||
return executed.ok
|
||||
? { ok: true as const, value: executed.value.evidence }
|
||||
: {
|
||||
ok: false as const,
|
||||
error: projectAnalysisError(executed.error),
|
||||
};
|
||||
} finally {
|
||||
let cleanupFailed = false;
|
||||
if (sessionId !== undefined) {
|
||||
try {
|
||||
const closed = await manager.close(sessionId);
|
||||
cleanupFailed = !closed.ok;
|
||||
if (!closed.ok)
|
||||
? ok(executed.value.evidence)
|
||||
: err(executed.error);
|
||||
},
|
||||
service: {
|
||||
close: () =>
|
||||
closeDebuggerCommandSession(
|
||||
manager,
|
||||
sessionId,
|
||||
"gdb",
|
||||
"gdb_console",
|
||||
(cleanup) =>
|
||||
logger.error(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithGdb,
|
||||
cleanup: projectAnalysisError(closed.error),
|
||||
},
|
||||
{ command: CLI_COMMANDS.debugWithGdb, cleanup },
|
||||
"GDB cleanup failed after collecting command output",
|
||||
);
|
||||
} catch (cause) {
|
||||
cleanupFailed = true;
|
||||
logger.error(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithGdb,
|
||||
cleanup:
|
||||
cause instanceof Error
|
||||
? cause.message
|
||||
: "Unknown cleanup failure",
|
||||
},
|
||||
"GDB cleanup failed after collecting command output",
|
||||
);
|
||||
}
|
||||
}
|
||||
try {
|
||||
await manager.closeAll();
|
||||
cleanupFailed = false;
|
||||
} catch (cause) {
|
||||
cleanupFailed = true;
|
||||
logger.error(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithGdb,
|
||||
cleanup:
|
||||
cause instanceof Error
|
||||
? cause.message
|
||||
: "Unknown cleanup failure",
|
||||
},
|
||||
"GDB fallback cleanup failed",
|
||||
);
|
||||
}
|
||||
if (cleanupFailed) process.exitCode = 1;
|
||||
}
|
||||
})();
|
||||
return publishCliAnalysisEvidence(completed, delivery);
|
||||
),
|
||||
),
|
||||
},
|
||||
});
|
||||
}),
|
||||
),
|
||||
});
|
||||
@@ -410,68 +419,47 @@ export const registerReverseEngineeringCommands = (
|
||||
logCliCommand(logger, CLI_COMMANDS.debugWithRizin, async () => {
|
||||
const manager = new RizinDebugSessionManager(environment);
|
||||
let sessionId: string | undefined;
|
||||
try {
|
||||
const started = await manager.start(
|
||||
{
|
||||
...args,
|
||||
inferior_tty: options.inferiorTty,
|
||||
...(options.backend === undefined
|
||||
? {}
|
||||
: { backend: options.backend }),
|
||||
},
|
||||
signal,
|
||||
);
|
||||
if (!started.ok) return projectAnalysisError(started.error);
|
||||
sessionId = started.value.session_id;
|
||||
const executed = await manager.execute(
|
||||
started.value.session_id,
|
||||
args.command,
|
||||
signal,
|
||||
);
|
||||
return executed.ok
|
||||
? executed.value.evidence
|
||||
: projectAnalysisError(executed.error);
|
||||
} finally {
|
||||
try {
|
||||
if (sessionId !== undefined) {
|
||||
const closed = await manager.close(sessionId);
|
||||
if (!closed.ok)
|
||||
logger.error(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithRizin,
|
||||
cleanup: projectAnalysisError(closed.error),
|
||||
},
|
||||
"Rizin cleanup failed after collecting command output",
|
||||
);
|
||||
}
|
||||
} catch (cause) {
|
||||
logger.error(
|
||||
return finishAnalysisCommand(
|
||||
async () => {
|
||||
const started = await manager.start(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithRizin,
|
||||
cleanup:
|
||||
cause instanceof Error
|
||||
? cause.message
|
||||
: "Unknown cleanup failure",
|
||||
...args,
|
||||
inferior_tty: options.inferiorTty,
|
||||
...(options.backend === undefined
|
||||
? {}
|
||||
: { backend: options.backend }),
|
||||
},
|
||||
"Rizin cleanup failed after collecting command output",
|
||||
signal,
|
||||
);
|
||||
}
|
||||
// The retained owner retries all sessions. Only its final outcome
|
||||
// establishes whether command cleanup remains incomplete.
|
||||
await manager.closeAll().catch((cause: unknown) => {
|
||||
process.exitCode = 1;
|
||||
logger.error(
|
||||
{
|
||||
command: CLI_COMMANDS.debugWithRizin,
|
||||
cleanup:
|
||||
cause instanceof Error
|
||||
? cause.message
|
||||
: "Unknown cleanup failure",
|
||||
},
|
||||
"Rizin fallback cleanup failed",
|
||||
if (!started.ok) return err(started.error);
|
||||
sessionId = started.value.session_id;
|
||||
const executed = await manager.execute(
|
||||
sessionId,
|
||||
args.command,
|
||||
signal,
|
||||
);
|
||||
});
|
||||
}
|
||||
return executed.ok
|
||||
? ok(executed.value.evidence)
|
||||
: err(executed.error);
|
||||
},
|
||||
{
|
||||
close: () =>
|
||||
closeDebuggerCommandSession(
|
||||
manager,
|
||||
sessionId,
|
||||
"rizin",
|
||||
"rizin_debug_command",
|
||||
(cleanup) =>
|
||||
logger.error(
|
||||
{ command: CLI_COMMANDS.debugWithRizin, cleanup },
|
||||
"Rizin cleanup failed after collecting command output",
|
||||
),
|
||||
),
|
||||
},
|
||||
"rizin",
|
||||
"rizin_debug_command",
|
||||
signal,
|
||||
).then(projectCliAnalysisResult);
|
||||
}),
|
||||
),
|
||||
});
|
||||
|
||||
@@ -1,3 +1,6 @@
|
||||
import { EventEmitter } from "node:events";
|
||||
import { PassThrough } from "node:stream";
|
||||
|
||||
import pino from "pino";
|
||||
import { expect, it, vi } from "vitest";
|
||||
|
||||
@@ -123,7 +126,12 @@ it("marks the CLI failed when retained GDB cleanup remains incomplete", async ()
|
||||
args: { command: "info files" },
|
||||
options: { inferiorTty: "/dev/pts/42" },
|
||||
});
|
||||
expect(result).toMatchObject({ code: "unreadable_output" });
|
||||
expect(result).toMatchObject({
|
||||
code: "cleanup_incomplete",
|
||||
details: {
|
||||
diagnostics: { primary_error: { code: "unreadable_output" } },
|
||||
},
|
||||
});
|
||||
expect(manager.closeAll).toHaveBeenCalledOnce();
|
||||
expect(process.exitCode).toBe(1);
|
||||
} finally {
|
||||
@@ -199,3 +207,135 @@ it("keeps prelaunch caller cancellation distinct from executable refusal", async
|
||||
await manager.closeAll();
|
||||
}
|
||||
});
|
||||
|
||||
class FakeGdb extends EventEmitter {
|
||||
pid = process.pid + 1;
|
||||
stdout = new PassThrough();
|
||||
stderr = new PassThrough();
|
||||
stdin = new PassThrough();
|
||||
exitCode: number | null = null;
|
||||
signalCode: NodeJS.Signals | null = null;
|
||||
kill(signal: NodeJS.Signals) {
|
||||
this.exitCode = 0;
|
||||
this.signalCode = signal;
|
||||
this.stdout.end();
|
||||
this.stderr.end();
|
||||
this.stdin.destroy();
|
||||
this.emit("exit", 0, signal);
|
||||
this.emit("close", 0, signal);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
const modeledGdbReplies = new Map([
|
||||
["-gdb-set auto-load off", "^done"],
|
||||
['-inferior-tty-set "/dev/pts/7"', "^done"],
|
||||
["-inferior-tty-show", '^done,inferior_tty_terminal="/dev/pts/7"'],
|
||||
['-interpreter-exec console "info files"', "^done"],
|
||||
["-list-thread-groups --recurse 1", "^done,groups=[]"],
|
||||
["-gdb-exit", "^exit"],
|
||||
]);
|
||||
|
||||
it.each(["incomplete", "interrupt", "control"] as const)(
|
||||
"reports final GDB cleanup %s as a typed outcome",
|
||||
async (mode) => {
|
||||
const child = new FakeGdb();
|
||||
const unmodeled: string[] = [];
|
||||
let requests = 0;
|
||||
child.stdout.write("(gdb)\n");
|
||||
child.stdin.on("data", (chunk) => {
|
||||
for (const line of String(chunk).split("\n")) {
|
||||
const match = /^(\d+)(.*)$/u.exec(line);
|
||||
if (!match) continue;
|
||||
requests++;
|
||||
const token = match[1] ?? "";
|
||||
const tail = match[2] ?? "";
|
||||
const reply = modeledGdbReplies.get(tail);
|
||||
if (reply === undefined) unmodeled.push(line);
|
||||
child.stdout.write(
|
||||
`${token}${reply ?? '^error,msg="unmodeled request"'}\n`,
|
||||
);
|
||||
}
|
||||
});
|
||||
let calls = 0;
|
||||
let recovering = false;
|
||||
let retainedId: string | undefined;
|
||||
const manager = new GdbSessionManager({
|
||||
platform: "linux",
|
||||
spawnProcess: async (options) => ({
|
||||
process: child,
|
||||
ownership: {
|
||||
runId: options.runId,
|
||||
leaderPid: child.pid,
|
||||
processGroupId: child.pid,
|
||||
expectedParentPid: process.pid,
|
||||
},
|
||||
}),
|
||||
stopProcess: async () => {
|
||||
calls++;
|
||||
if (mode === "interrupt" && calls === 1) process.emit("SIGINT");
|
||||
if (mode === "incomplete" && !recovering)
|
||||
return {
|
||||
status: "incomplete",
|
||||
reason: "synthetic stop remains pending",
|
||||
};
|
||||
child.kill("SIGTERM");
|
||||
return { status: "stopped" };
|
||||
},
|
||||
});
|
||||
const originalStart = manager.start.bind(manager);
|
||||
manager.start = async (...args) => {
|
||||
const started = await originalStart(...args);
|
||||
if (started.ok) retainedId = started.value.session_id;
|
||||
return started;
|
||||
};
|
||||
const command = registerGdbCommand(() => manager);
|
||||
const previous = process.exitCode;
|
||||
try {
|
||||
process.exitCode = 0;
|
||||
const result = await command.run({
|
||||
args: { command: "info files" },
|
||||
options: { inferiorTty: "/dev/pts/7" },
|
||||
});
|
||||
expect(requests).toBeGreaterThan(0);
|
||||
expect(unmodeled).toEqual([]);
|
||||
if (mode === "control") {
|
||||
expect(result).toMatchObject({
|
||||
operation: "gdb_console",
|
||||
parameters: { session_id: retainedId },
|
||||
normalized_result: { completion_status: "complete" },
|
||||
});
|
||||
expect(process.exitCode).toBe(0);
|
||||
} else if (mode === "interrupt") {
|
||||
expect(result).toMatchObject({
|
||||
code: "cancelled",
|
||||
details: {
|
||||
partial_observation: {
|
||||
operation: "gdb_console",
|
||||
parameters: { session_id: retainedId },
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(process.exitCode).toBe(130);
|
||||
} else {
|
||||
expect(result).toMatchObject({
|
||||
code: "cleanup_incomplete",
|
||||
details: {
|
||||
cleanup: "incomplete",
|
||||
resources: [retainedId],
|
||||
partial_observation: {
|
||||
operation: "gdb_console",
|
||||
parameters: { session_id: retainedId },
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(process.exitCode).toBe(1);
|
||||
expect(retainedId).toBeDefined();
|
||||
}
|
||||
} finally {
|
||||
recovering = true;
|
||||
await manager.closeAll();
|
||||
process.exitCode = previous;
|
||||
}
|
||||
},
|
||||
);
|
||||
@@ -17,6 +17,7 @@ import {
|
||||
} from "../../../src/domain/evidence.js";
|
||||
import { err, ok } from "../../../src/domain/result.js";
|
||||
import { RizinDebugSessionManager } from "../../../src/rizin/RizinDebugSessionManager.js";
|
||||
import { closeOwnedDebuggerSessions } from "../../../src/process/ownedDebuggerSession.js";
|
||||
import { ReverseEngineeringService as CoreReverseEngineeringService } from "../../../src/application/reverse/ReverseEngineeringService.js";
|
||||
import { createReverseEngineeringCommandPort } from "../../../src/composition/reverseEngineering.js";
|
||||
import { OwnedCommandFailure } from "../../../src/process/OwnedCommand.js";
|
||||
@@ -512,3 +513,141 @@ it.each([false, true])(
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
it.each(["incomplete", "interrupt", "control"] as const)(
|
||||
"reports final Rizin cleanup %s as a typed outcome",
|
||||
async (mode) => {
|
||||
const commands = new Map<
|
||||
string,
|
||||
{ run(input: unknown): Promise<unknown> }
|
||||
>();
|
||||
const cli = {
|
||||
command(
|
||||
name: string,
|
||||
definition: { run(input: unknown): Promise<unknown> },
|
||||
) {
|
||||
commands.set(name, definition);
|
||||
return cli;
|
||||
},
|
||||
} as unknown as CliInstance;
|
||||
const previous = process.exitCode;
|
||||
const evidence = createEvidence(
|
||||
undefined,
|
||||
{ id: "rizin.debugger", name: "Rizin debugger", version: null },
|
||||
{
|
||||
predicateType: "rea.debugger.command-observation",
|
||||
operation: "rizin_debug_command",
|
||||
parameters: {
|
||||
session_id: "fixture-session",
|
||||
command: "dr",
|
||||
inferior_tty: "/dev/pts/7",
|
||||
},
|
||||
result: { output: "registers" },
|
||||
rawResult: { output: "registers" },
|
||||
},
|
||||
);
|
||||
const sessions = new Map([["fixture-session", { id: "fixture-session" }]]);
|
||||
vi.spyOn(RizinDebugSessionManager.prototype, "start").mockResolvedValue(
|
||||
ok({ session_id: "fixture-session", backend: null }),
|
||||
);
|
||||
const execute = vi
|
||||
.spyOn(RizinDebugSessionManager.prototype, "execute")
|
||||
.mockResolvedValue(
|
||||
ok({
|
||||
evidence,
|
||||
result: {
|
||||
command: "dr",
|
||||
output: "registers",
|
||||
output_scope: "command_and_interleaved_session_output",
|
||||
output_truncated: false,
|
||||
completion_status: "complete",
|
||||
backend: null,
|
||||
},
|
||||
}),
|
||||
);
|
||||
vi.spyOn(RizinDebugSessionManager.prototype, "close").mockImplementation(
|
||||
async () => {
|
||||
if (mode === "incomplete")
|
||||
return err(
|
||||
new AnalysisOutputError(
|
||||
"close_rizin_debug_session",
|
||||
"stop unconfirmed",
|
||||
{
|
||||
cleanup: {
|
||||
reason: "stop unconfirmed",
|
||||
resources: ["fixture-session"],
|
||||
},
|
||||
},
|
||||
),
|
||||
);
|
||||
sessions.clear();
|
||||
return ok({ target_state: "unknown" });
|
||||
},
|
||||
);
|
||||
let abortedAtCleanupEnd = false;
|
||||
let signal: AbortSignal | undefined;
|
||||
const closeAll = vi
|
||||
.spyOn(RizinDebugSessionManager.prototype, "closeAll")
|
||||
.mockImplementation(async () => {
|
||||
signal = execute.mock.calls[0]?.[2];
|
||||
await new Promise<void>((resolve) =>
|
||||
setImmediate(() => {
|
||||
if (mode === "interrupt") process.emit("SIGINT");
|
||||
resolve();
|
||||
}),
|
||||
);
|
||||
abortedAtCleanupEnd = signal?.aborted === true;
|
||||
await closeOwnedDebuggerSessions(
|
||||
sessions,
|
||||
async () => ({ status: "incomplete", reason: "stop unconfirmed" }),
|
||||
"Rizin",
|
||||
);
|
||||
});
|
||||
registerReverseEngineeringCommands(cli, pino({ enabled: false }), {});
|
||||
const command = commands.get(CLI_COMMANDS.debugWithRizin);
|
||||
if (command === undefined)
|
||||
throw new Error("Rizin CLI command was not registered");
|
||||
try {
|
||||
process.exitCode = 0;
|
||||
const result = await command.run({
|
||||
args: { path: "./sample.bin", command: "dr" },
|
||||
options: { inferiorTty: "/dev/pts/7" },
|
||||
});
|
||||
expect(closeAll).toHaveBeenCalledOnce();
|
||||
if (mode === "control") {
|
||||
expect(result).toBe(evidence);
|
||||
expect(process.exitCode).toBe(0);
|
||||
expect(abortedAtCleanupEnd).toBe(false);
|
||||
} else if (mode === "interrupt") {
|
||||
expect(result).toMatchObject({
|
||||
code: "cancelled",
|
||||
details: {
|
||||
partial_observation: {
|
||||
operation: "rizin_debug_command",
|
||||
parameters: { session_id: "fixture-session" },
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(process.exitCode).toBe(130);
|
||||
expect(abortedAtCleanupEnd).toBe(true);
|
||||
} else {
|
||||
expect(result).toMatchObject({
|
||||
code: "cleanup_incomplete",
|
||||
details: {
|
||||
cleanup: "incomplete",
|
||||
resources: ["fixture-session"],
|
||||
partial_observation: {
|
||||
operation: "rizin_debug_command",
|
||||
parameters: { session_id: "fixture-session" },
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(sessions.size).toBe(1);
|
||||
expect(process.exitCode).toBe(1);
|
||||
}
|
||||
} finally {
|
||||
process.exitCode = previous;
|
||||
vi.restoreAllMocks();
|
||||
}
|
||||
},
|
||||
);
|
||||
Reference in new issue
Block a user