From 48f700ee35e0988ed973e5486dc9db33f2a5d944 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:37:32 +0800 Subject: [PATCH] fix!: retain Frida observations through shared finalization Reuse the shared one-shot finalizer for discovery and instrumentation, preserve finalization cancellation with completed observations, and bind Frida cleanup errors to exact owners so confirmed retry recovers the primary error. BREAKING CHANGE: instrument-with-frida JSON now returns the canonical Evidence record directly. Read normalized_result.cleanup_error instead of the earlier top-level cleanup_error wrapper field. --- docs/frida-instrumentation.md | 2 + .../frida/FridaInstrumentationManager.test.ts | 81 +++++- .../frida/FridaInstrumentationService.ts | 92 ++---- src/cli/finishAnalysisCommand.ts | 23 +- src/cli/fridaCommands.ts | 124 ++++---- src/contracts/fridaToolContracts.ts | 27 +- src/domain/analysisErrorBase.ts | 3 + src/domain/fridaDiscovery.ts | 29 ++ src/frida/FridaInstrumentationManager.ts | 271 ++++++++++++------ .../cli/finishAnalysisCommand.test.ts | 33 +++ tests/boundary/cli/fridaCli.test.ts | 47 ++- .../fridaFinalizationCli.test.ts | 113 ++++++++ 12 files changed, 563 insertions(+), 282 deletions(-) create mode 100644 src/domain/fridaDiscovery.ts create mode 100644 tests/process-global/fridaFinalizationCli.test.ts diff --git a/docs/frida-instrumentation.md b/docs/frida-instrumentation.md index 4ebb974060..d029358b07 100644 --- a/docs/frida-instrumentation.md +++ b/docs/frida-instrumentation.md @@ -6,6 +6,8 @@ For remote devices, pass a host with an optional port (`host[:port]`; bracket IP MCP exposes `list_frida_devices`, `list_frida_processes`, `start_frida_session`, `load_frida_script`, `resume_frida_session`, `unload_frida_script`, `frida_session_status`, and `close_frida_session`. A spawned process starts paused so scripts can load first. Closing unloads session scripts and detaches. If a spawn remains paused, REA resumes it only after script cleanup or session detach confirms the instrumentation is released; if both cleanup paths fail, the target stays paused and the cleanup error is retained. REA never kills the target. MCP sessions end with the server process. The one-shot CLI command `instrument-with-frida` accepts one inline script or script file, observes messages for a bounded duration, and returns Evidence with the source hash and captured messages. +`instrument-with-frida --json` returns the Evidence record directly, matching MCP. Consumers of the earlier `{evidence, cleanup_error}` CLI wrapper should read the record at the JSON root and the session cleanup observation at `normalized_result.cleanup_error`. If final provider cleanup fails or cancellation arrives during it, the typed error retains completed Evidence in `details.partial_observation`. Discovery commands retain their device or process report in the same field. + For state and capture-coverage checks, pass `{"session_id":"SESSION_ID","detail":"summary"}` to `frida_session_status`. Each call reads the observed session state and retains the complete observation. The reply is an `inspect_analysis_view` Evidence summary with the session identity, state, scripts, `messages_retained`, and `messages_truncated`; message payloads remain in its `parent_evidence_id`. The count covers retained queued messages, not every target event. Omitted `detail` or `detail=complete` preserves the existing complete reply. A summary's selected fields look like this (excerpt): diff --git a/src/application/frida/FridaInstrumentationManager.test.ts b/src/application/frida/FridaInstrumentationManager.test.ts index df76dbbf51..1630d4db34 100644 --- a/src/application/frida/FridaInstrumentationManager.test.ts +++ b/src/application/frida/FridaInstrumentationManager.test.ts @@ -19,6 +19,9 @@ import { } from "../../domain/analysisErrorCore.js"; import { FridaInstrumentationManager } from "../../frida/FridaInstrumentationManager.js"; import { fridaCaptureLimitations } from "../../frida/fridaCaptureLimits.js"; +import { AnalysisError } from "../../domain/analysisErrorBase.js"; +import { projectAnalysisError } from "../../domain/analysisErrorProjection.js"; +import { FridaInstrumentationService } from "./FridaInstrumentationService.js"; const signal = void>() => { let connected: Handler | undefined; @@ -413,7 +416,13 @@ describe("FridaInstrumentationManager", () => { expect(bindings.device.resumed).toEqual([]); expect(bindings.removedRemote).toEqual([]); - await expect(manager.closeAll()).rejects.toThrow("cleanup was incomplete"); + const closing = await manager.closeAll().catch((cause: unknown) => cause); + expect(closing).toBeInstanceOf(AnalysisError); + if (!(closing instanceof AnalysisError)) + throw new Error("Expected typed cleanup failure"); + expect(projectAnalysisError(closing)).toMatchObject({ + code: "cleanup_incomplete", + }); expect(bindings.device.resumed).toEqual([]); expect(bindings.removedRemote).toEqual([]); @@ -423,6 +432,8 @@ describe("FridaInstrumentationManager", () => { bindings.getDeviceById = async () => replacement; bindings.addRemoteDevice = async () => replacement; await Promise.all([manager.closeAll(), manager.closeAll()]); + expect(started.error.cleanupIncomplete).toBe(false); + expect(closing.cleanupIncomplete).toBe(false); expect(bindings.device.resumed).toEqual([512]); expect(replacement.resumed).toEqual([]); expect(bindings.removedRemote).toEqual(["127.0.0.1:27042"]); @@ -671,6 +682,7 @@ describe("FridaInstrumentationManager", () => { expect(bindings.device.session.scripts[0]?.unloadCalls).toBe(2); expect(bindings.device.session.scripts[0]?.isDestroyed).toBe(true); expect(manager.status(started.value.sessionId)).toBeUndefined(); + if (!loaded.ok) expect(loaded.error.cleanupIncomplete).toBe(false); }); it("closes a session when its target already destroyed the script", async () => { @@ -1585,3 +1597,70 @@ it("preserves a native rejection observed before cancellation during script clea expect(result.error.cleanupIncomplete).toBe(false); expect(manager.status(started.value.sessionId)?.scripts).toEqual([]); }); + +it("restores the original load failure only after fallback cleanup confirms its exact owners", async () => { + const bindings = new FakeDeviceManager(); + bindings.device.session.nextLoadFailure = new Error( + "primary native load failed", + ); + bindings.device.session.nextUnloadFailureCount = 3; + bindings.device.session.detachFailureCount = 2; + const manager = new FridaInstrumentationManager({ deviceManager: bindings }); + const service = new FridaInstrumentationService(manager); + const result = await service.instrument({ + mode: "attach", + pid: 512, + deviceId: "local", + source: { sourceKind: "inline", source: "send(1)" }, + durationMs: 0, + }); + if (result.ok) throw new Error("Expected native load failure"); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cleanup_incomplete", + }); + const closed = await service.closeAll().catch((cause: unknown) => cause); + expect(closed).toBeInstanceOf(AnalysisError); + expect(result.error.cleanupIncomplete).toBe(true); + await service.closeAll(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "capability_unavailable", + details: { + operation: "load_frida_script", + reason: "primary native load failed", + }, + }); +}); + +it("retries only unresolved sessions and resolves the typed close aggregate", async () => { + const bindings = new FakeDeviceManager(); + const failing = new FakeSession(); + failing.detachFailureCount = 1; + const successful = new FakeSession(); + const sessions = [failing, successful]; + const calls = [0, 0]; + for (const [index, session] of sessions.entries()) { + const detach = session.detach.bind(session); + session.detach = async () => { + calls[index] = (calls[index] ?? 0) + 1; + await detach(); + }; + } + bindings.device.attach = async () => { + const session = sessions.shift(); + if (session === undefined) throw new Error("Unexpected extra attach"); + return session; + }; + const manager = new FridaInstrumentationManager({ deviceManager: bindings }); + await manager.startSession({ mode: "attach", pid: 512 }); + await manager.startSession({ mode: "attach", pid: 513 }); + const failure = await manager.closeAll().catch((cause: unknown) => cause); + if (!(failure instanceof AnalysisError)) + throw new Error("Expected typed cleanup aggregate"); + expect(failure.cleanupIncomplete).toBe(true); + expect(failure.cleanupResources).toHaveLength(1); + expect(calls).toEqual([1, 1]); + await manager.closeAll(); + expect(calls).toEqual([2, 1]); + expect(failure.cleanupIncomplete).toBe(false); +}); diff --git a/src/application/frida/FridaInstrumentationService.ts b/src/application/frida/FridaInstrumentationService.ts index 4f21b054f2..685347bd30 100644 --- a/src/application/frida/FridaInstrumentationService.ts +++ b/src/application/frida/FridaInstrumentationService.ts @@ -1,12 +1,14 @@ import { constants } from "node:fs"; import { access, stat } from "node:fs/promises"; import { setTimeout as delay } from "node:timers/promises"; -import type { z } from "zod"; -import type { toolContract } from "../../contracts/toolContracts.js"; import { createEvidence, type Evidence } from "../../domain/evidence.js"; import { jsonObjectSchema, jsonValueSchema } from "../../domain/jsonValue.js"; -import type { AnalysisError } from "../../domain/analysisErrorBase.js"; +import { + resolvedAnalysisError, + type AnalysisError, +} from "../../domain/analysisErrorBase.js"; +import { analysisErrorWithCleanupFailure } from "../../domain/analysisErrorCleanup.js"; import { AnalysisAccessDeniedError, AnalysisCancelledError, @@ -24,6 +26,10 @@ import { import { fridaCaptureLimitations } from "../../frida/fridaCaptureLimits.js"; import { unknownFridaSession } from "./fridaInputErrors.js"; import { FridaScriptLoadCancelledError } from "./FridaInstrumentationPort.js"; +import type { + FridaDeviceList, + FridaProcessList, +} from "../../domain/fridaDiscovery.js"; import type { FridaDeviceSelector, FridaInstrumentationPort, @@ -35,12 +41,6 @@ import type { StartFridaSessionInput, } from "./FridaInstrumentationPort.js"; -type DeviceListOutput = z.output< - ReturnType>["outputSchema"] ->; -type ProcessListOutput = z.output< - ReturnType>["outputSchema"] ->; type FridaSessionCloseResult = Awaited< ReturnType >; @@ -123,7 +123,7 @@ export class FridaInstrumentationService { async listDevices( remote?: FridaRemoteConnection, - ): Promise> { + ): Promise> { const result = await this.#provider.listDevices(remote); return result.ok ? ok({ @@ -139,7 +139,7 @@ export class FridaInstrumentationService { async listProcesses( selector: FridaDeviceSelector, - ): Promise> { + ): Promise> { const result = await this.#provider.listProcesses(selector); return result.ok ? ok({ @@ -332,13 +332,13 @@ export class FridaInstrumentationService { if (!loaded.ok) { const closed = await this.#provider.closeSession(started.value.sessionId); return closed.ok - ? err(withRecoveredFridaCleanup(loaded.error)) + ? err(resolvedAnalysisError(loaded.error)) : err( - withCleanupFailure( + analysisErrorWithCleanupFailure( loaded.error, - closed.error.message, - started.value.sessionId, - ), + closed.error, + "instrument_with_frida", + ).bindCleanupDependencies([loaded.error, closed.error]), ); } if (signal?.aborted) @@ -364,11 +364,11 @@ export class FridaInstrumentationService { return closed.ok ? resumed : err( - withCleanupFailure( + analysisErrorWithCleanupFailure( resumed.error, - closed.error.message, - started.value.sessionId, - ), + closed.error, + "instrument_with_frida", + ).bindCleanupDependencies([resumed.error, closed.error]), ); } if (signal?.aborted) @@ -498,7 +498,7 @@ const cancelledFridaSession = ({ script, }: FridaSessionCancellation): AnalysisCancelledError => { const cleanupError = closed.ok ? null : closed.error.message; - return new AnalysisCancelledError(operation, { + const failure = new AnalysisCancelledError(operation, { ...(cause === undefined ? {} : { cause }), diagnostics, partialObservation: fridaEvidence( @@ -529,6 +529,7 @@ const cancelledFridaSession = ({ }, }), }); + return closed.ok ? failure : failure.bindCleanupDependencies([closed.error]); }; const fridaParameters = ( @@ -560,53 +561,6 @@ const fridaParameters = ( }), }); -const withCleanupFailure = ( - original: AnalysisError, - cleanupReason: string, - sessionId: string, -): AnalysisError => - new AnalysisCapabilityUnavailableError( - "frida", - "instrument_with_frida", - `${original.message}; cleanup failed: ${cleanupReason}`, - { - cause: original, - diagnostics: original.diagnostics, - ...(original.partialObservation === undefined - ? {} - : { partialObservation: original.partialObservation }), - cleanup: { - reason: cleanupReason, - resources: [sessionId], - }, - }, - ); - -/** Preserve the original Frida failure after its owner confirms cleanup. */ -export const withRecoveredFridaCleanup = ( - error: AnalysisError, -): AnalysisError => - error instanceof AnalysisCapabilityUnavailableError && error.cleanupIncomplete - ? new AnalysisCapabilityUnavailableError( - error.providerId, - error.operation, - error.reason, - { - cause: error.cause, - diagnostics: error.diagnostics, - ...(error.capturedOutput === undefined - ? {} - : { capturedOutput: error.capturedOutput }), - ...(error.partialObservation === undefined - ? {} - : { partialObservation: error.partialObservation }), - ...(error.userMessage === undefined - ? {} - : { userMessage: error.userMessage }), - }, - ) - : error; - const sourceOutput = (script: FridaScriptObservation) => ({ source_kind: script.sourceKind, source_path: script.sourcePath, @@ -657,4 +611,4 @@ const cancelledScriptEvidence = ( { session_id: sessionId }, error.script, ), - }); + }).bindCleanupDependencies([error]); diff --git a/src/cli/finishAnalysisCommand.ts b/src/cli/finishAnalysisCommand.ts index 2b91779de2..5a529fd0a3 100644 --- a/src/cli/finishAnalysisCommand.ts +++ b/src/cli/finishAnalysisCommand.ts @@ -23,6 +23,7 @@ export const finishAnalysisCommand = async < fallbackResource?: string, ): Promise> => { let result: Result; + let cleanup: AnalysisError | undefined; try { result = await run(); } catch (cause) { @@ -39,7 +40,6 @@ export const finishAnalysisCommand = async < // adapter cannot infer resolution of a producer's earlier cleanup failure. await service.close(); } catch (cause) { - let cleanup: AnalysisError; if (cause instanceof AnalysisError) cleanup = cause; else { const observation = @@ -59,6 +59,17 @@ export const finishAnalysisCommand = async < { cause, operation: canonicalOperation }, ); } + } + await dispatchQueuedInterrupts(); + if (result.ok && signal.aborted) + result = { + ok: false, + error: new AnalysisCancelledError(canonicalOperation, { + cause: signal.reason, + partialObservation: result.value, + }), + }; + if (cleanup !== undefined) { if (result.ok) cleanup.retainPartialObservation(result.value); result = { ok: false, @@ -68,16 +79,8 @@ export const finishAnalysisCommand = async < result.error, cleanup, canonicalOperation, - ), + ).bindCleanupDependencies([result.error, cleanup]), }; } - await dispatchQueuedInterrupts(); - if (result.ok && signal.aborted) - return projectAnalysisError( - new AnalysisCancelledError(canonicalOperation, { - cause: signal.reason, - partialObservation: result.value, - }), - ); return result.ok ? result.value : projectAnalysisError(result.error); }; diff --git a/src/cli/fridaCommands.ts b/src/cli/fridaCommands.ts index aa0157fa4d..f606891aa4 100644 --- a/src/cli/fridaCommands.ts +++ b/src/cli/fridaCommands.ts @@ -1,9 +1,6 @@ import { z } from "incur"; -import { - FridaInstrumentationService, - withRecoveredFridaCleanup, -} from "../application/frida/FridaInstrumentationService.js"; +import { FridaInstrumentationService } from "../application/frida/FridaInstrumentationService.js"; import type { FridaInstrumentationPort, FridaRemoteConnection, @@ -13,16 +10,13 @@ import { CLI_COMMANDS } from "../cliCommandNames.js"; import { logCliCommand } from "../cliLogging.js"; import { projectAnalysisError } from "../domain/analysisErrorProjection.js"; import { AnalysisInputError } from "../domain/analysisErrorCore.js"; -import type { AnalysisError } from "../domain/analysisErrorBase.js"; -import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js"; -import { ProviderCleanupError } from "../domain/providerCleanupError.js"; -import { jsonValueSchema } from "../domain/jsonValue.js"; -import { err, type Result } from "../domain/result.js"; +import { ok } from "../domain/result.js"; import { FRIDA_KEEPALIVE_INTERVAL_DESCRIPTION } from "../contracts/fridaToolContracts.js"; import { isValidFridaRemoteAddress } from "../contracts/fridaRemoteAddress.js"; import type { Logger } from "pino"; import type { CliInstance } from "./types.js"; import { withCommandCancellation } from "./commandCancellation.js"; +import { finishAnalysisCommand } from "./finishAnalysisCommand.js"; /** Register Frida discovery and one-shot instrumentation CLI operations. */ export const registerFridaCommands = ( @@ -33,33 +27,7 @@ export const registerFridaCommands = ( const service = new FridaInstrumentationService(provider); // CLI calls own a disposable provider; MCP retains its shared provider until // server shutdown. Await cleanup here, including failed startup resources. - const withProviderCleanup = async ( - operation: () => Promise>, - ): Promise> => { - const result = await operation(); - try { - await provider.closeAll(); - } catch (cause: unknown) { - const cleanup = new ProviderCleanupError( - "frida", - ["Frida sessions or connections"], - result.ok - ? { partial_observation: jsonValueSchema.parse(result.value) } - : {}, - { cause, operation: "frida_cleanup" }, - ); - return err( - result.ok - ? cleanup - : analysisErrorWithCleanupFailure( - result.error, - cleanup, - "frida_cleanup", - ), - ); - } - return result.ok ? result : err(withRecoveredFridaCleanup(result.error)); - }; + const cleanup = { close: () => provider.closeAll() }; const remoteOptions = z.object({ remoteAddress: z .string() @@ -116,12 +84,17 @@ export const registerFridaCommands = ( "Remote credentials and connection options require remoteAddress", ), run: ({ options }) => - logCliCommand(logger, CLI_COMMANDS.listFridaDevices, async () => { - const result = await withProviderCleanup(() => - service.listDevices(remoteFrom(options)), - ); - return result.ok ? result.value : projectAnalysisError(result.error); - }), + withCommandCancellation((signal) => + logCliCommand(logger, CLI_COMMANDS.listFridaDevices, () => + finishAnalysisCommand( + () => service.listDevices(remoteFrom(options)), + cleanup, + "frida", + "list_frida_devices", + signal, + ), + ), + ), }); cli.command(CLI_COMMANDS.listFridaProcesses, { description: "List processes visible through a Frida device", @@ -144,17 +117,23 @@ export const registerFridaCommands = ( "Remote credentials and connection options require remoteAddress", ), run: ({ options }) => - logCliCommand(logger, CLI_COMMANDS.listFridaProcesses, async () => { - const remote = remoteFrom(options); - const result = await withProviderCleanup(() => - service.listProcesses( - remote === undefined - ? { deviceId: options.deviceId ?? "local" } - : { remote }, - ), - ); - return result.ok ? result.value : projectAnalysisError(result.error); - }), + withCommandCancellation((signal) => + logCliCommand(logger, CLI_COMMANDS.listFridaProcesses, () => { + const remote = remoteFrom(options); + return finishAnalysisCommand( + () => + service.listProcesses( + remote === undefined + ? { deviceId: options.deviceId ?? "local" } + : { remote }, + ), + cleanup, + "frida", + "list_frida_processes", + signal, + ); + }), + ), }); cli.command(CLI_COMMANDS.instrumentWithFrida, { description: "Run one Frida script against a selected process and detach", @@ -275,25 +254,28 @@ export const registerFridaCommands = ( ], ), ); - const result = await withProviderCleanup(() => - service.instrument( - { - ...targetInput, - source: - options.scriptFile === undefined - ? { sourceKind: "inline", source: options.script ?? "" } - : { sourceKind: "file", path: options.scriptFile }, - durationMs: options.durationMs, - }, - signal, - ), + return finishAnalysisCommand( + async () => { + const result = await service.instrument( + { + ...targetInput, + source: + options.scriptFile === undefined + ? { sourceKind: "inline", source: options.script ?? "" } + : { sourceKind: "file", path: options.scriptFile }, + durationMs: options.durationMs, + }, + signal, + ); + if (!result.ok) return result; + if (result.value.cleanupError !== null) process.exitCode = 1; + return ok(result.value.evidence); + }, + cleanup, + "frida", + "instrument_with_frida", + signal, ); - if (!result.ok) return projectAnalysisError(result.error); - if (result.value.cleanupError !== null) process.exitCode = 1; - return { - evidence: result.value.evidence, - cleanup_error: result.value.cleanupError, - }; }), ), }); diff --git a/src/contracts/fridaToolContracts.ts b/src/contracts/fridaToolContracts.ts index 9a6df51e85..c1a7b0568d 100644 --- a/src/contracts/fridaToolContracts.ts +++ b/src/contracts/fridaToolContracts.ts @@ -12,6 +12,10 @@ import { fridaSessionStatusSchema, } from "../domain/fridaSessionStatus.js"; import { fridaSessionStatusSummaryViewResultSchema } from "../domain/analysisView/analysisView.js"; +import { + fridaDeviceListSchema, + fridaProcessListSchema, +} from "../domain/fridaDiscovery.js"; export const FRIDA_KEEPALIVE_INTERVAL_DESCRIPTION = "Remote keepalive interval in seconds"; @@ -69,16 +73,7 @@ export const FRIDA_TOOL_CONTRACTS = [ description: "List Frida devices currently available to this host, optionally connecting to the caller-selected remote Frida endpoint. Remote authentication options are used for this call only and are never returned or recorded.", inputSchema: z.strictObject({ remote: remoteConnection.optional() }), - outputSchema: z.strictObject({ - devices: z.array( - z.strictObject({ - device_id: z.string(), - name: z.string(), - type: z.string(), - }), - ), - cleanup_error: z.string().nullable(), - }), + outputSchema: fridaDeviceListSchema, examples: [{ title: "List Frida devices", input: {} }], }, { @@ -95,17 +90,7 @@ export const FRIDA_TOOL_CONTRACTS = [ .describe( 'Provide exactly one of device_id or remote. device_id selects a device (use "local" for this host); remote requires an address.', ), - outputSchema: z.strictObject({ - device_id: z.string(), - processes: z.array( - z.strictObject({ - pid: z.int().nonnegative(), - name: z.string(), - identifier: z.string().nullable(), - }), - ), - cleanup_error: z.string().nullable(), - }), + outputSchema: fridaProcessListSchema, examples: [ { title: "List local Frida processes", diff --git a/src/domain/analysisErrorBase.ts b/src/domain/analysisErrorBase.ts index f408f471ee..aa54c7981b 100644 --- a/src/domain/analysisErrorBase.ts +++ b/src/domain/analysisErrorBase.ts @@ -255,9 +255,12 @@ import type { FileSnapshotPartialObservation } from "./fileSnapshotObservation.j import type { WebModuleResolutionObservation } from "./webModuleTrace.js"; import type { WebNetworkCapture } from "./webNetworkCapture.js"; +import type { FridaDeviceList, FridaProcessList } from "./fridaDiscovery.js"; /** Provider-neutral evidence collected before a typed analysis failure. */ export type AnalysisPartialObservation = + | FridaDeviceList + | FridaProcessList | WebModuleResolutionObservation | WebNetworkCapture | NativeLoadImagePartialObservation diff --git a/src/domain/fridaDiscovery.ts b/src/domain/fridaDiscovery.ts new file mode 100644 index 0000000000..8d5ee1e9b4 --- /dev/null +++ b/src/domain/fridaDiscovery.ts @@ -0,0 +1,29 @@ +import { z } from "zod"; + +/** Device observations retained even if the discovery connection cannot close. */ +export const fridaDeviceListSchema = z.strictObject({ + devices: z.array( + z.strictObject({ + device_id: z.string(), + name: z.string(), + type: z.string(), + }), + ), + cleanup_error: z.string().nullable(), +}); + +/** Selected device and process observations, independent of endpoint cleanup. */ +export const fridaProcessListSchema = z.strictObject({ + device_id: z.string(), + processes: z.array( + z.strictObject({ + pid: z.int().nonnegative(), + name: z.string(), + identifier: z.string().nullable(), + }), + ), + cleanup_error: z.string().nullable(), +}); + +export type FridaDeviceList = z.infer; +export type FridaProcessList = z.infer; diff --git a/src/frida/FridaInstrumentationManager.ts b/src/frida/FridaInstrumentationManager.ts index fd0a3c641c..20a709040d 100644 --- a/src/frida/FridaInstrumentationManager.ts +++ b/src/frida/FridaInstrumentationManager.ts @@ -33,6 +33,8 @@ import { AnalysisTimeoutError, } from "../domain/analysisErrorCore.js"; import type { AnalysisError } from "../domain/analysisErrorBase.js"; +import { ProviderCleanupError } from "../domain/providerCleanupError.js"; +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; import type { JsonValue } from "../domain/jsonValue.js"; import { jsonStringByteLength } from "../domain/jsonStringByteLength.js"; import { err, ok, type Result } from "../domain/result.js"; @@ -123,6 +125,7 @@ export interface FridaInstrumentationManagerOptions { export class FridaInstrumentationManager implements FridaInstrumentationPort { readonly #sessions = new Map(); readonly #pendingStartups = new Set(); + readonly #cleanupLeases = new WeakMap(); readonly #sessionQueues = new Map>(); readonly #sessionClosures = new Map< string, @@ -130,7 +133,12 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { >(); readonly #remoteDevices = new Map< string, - { references: number; optionsDigest: string; device: Promise } + { + id: string; + references: number; + optionsDigest: string; + device: Promise; + } >(); readonly #remoteQueues = new Map>(); readonly #deviceManagerLoader: () => Promise; @@ -202,9 +210,19 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { cleanupError = sanitizeError(cleanupCause, remote); } } - return err( - providerError("list_frida_devices", cause, remote, cleanupError), + const failure = providerError( + "list_frida_devices", + cause, + remote, + cleanupError, ); + const reference = + remote === undefined + ? undefined + : this.#remoteDevices.get(remote.address); + if (reference !== undefined && failure.cleanupIncomplete) + this.#retainCleanupFailure(reference, failure); + return err(failure); } } @@ -262,14 +280,19 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { cleanupError = sanitizeError(cleanupCause, selector.remote); } } - return err( - providerError( - "list_frida_processes", - cause, - selector.remote, - cleanupError, - ), + const failure = providerError( + "list_frida_processes", + cause, + selector.remote, + cleanupError, ); + const reference = + selector.remote === undefined + ? undefined + : this.#remoteDevices.get(selector.remote.address); + if (reference !== undefined && failure.cleanupIncomplete) + this.#retainCleanupFailure(reference, failure); + return err(failure); } } @@ -345,6 +368,7 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { return ok(sessionObservation(managed)); } catch (cause: unknown) { let cleanupFailures: readonly string[] = []; + let pendingCleanup: PendingStartupCleanup | undefined; if ( manager !== undefined && device !== undefined && @@ -363,16 +387,18 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { // Keep the original provider handles until every acquired resource is // released, including failures before a session can be registered. this.#pendingStartups.add(pending); + pendingCleanup = pending; cleanupFailures = await this.#cleanupStartup(pending); } - return err( - providerError( - "start_frida_session", - cause, - input.remote, - cleanupFailures.join("; ") || undefined, - ), + const failure = providerError( + "start_frida_session", + cause, + input.remote, + cleanupFailures.join("; ") || undefined, ); + if (pendingCleanup !== undefined && failure.cleanupIncomplete) + this.#retainCleanupFailure(pendingCleanup, failure); + return err(failure); } } @@ -486,15 +512,17 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { if (entry === undefined) managed.scriptCaptures.delete(scriptId); else cleanupReason = await this.#releaseScript(managed, entry); } - if (nativeFailure !== undefined && cancellation === undefined) - return err( - providerError( - "load_frida_script", - nativeFailure.cause, - managed.remote, - cleanupReason, - ), + if (nativeFailure !== undefined && cancellation === undefined) { + const failure = providerError( + "load_frida_script", + nativeFailure.cause, + managed.remote, + cleanupReason, ); + if (entry !== undefined && failure.cleanupIncomplete) + this.#retainCleanupFailure(entry, failure); + return err(failure); + } const observation: FridaScriptObservation = { scriptId, sourceKind: source.sourceKind, @@ -504,31 +532,33 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { messages: capture.messages.toArray(), messagesTruncated: capture.truncated, }; - if (cancellation !== undefined) - return err( - new FridaScriptLoadCancelledError(observation, { - cause: cancellation.reason, - scriptHandleAcquired: entry !== undefined, - ...(nativeFailure === undefined - ? {} - : { - diagnostics: { - native_failure: sanitizeError( - nativeFailure.cause, - managed.remote, - ), - }, - }), - ...(cleanupReason === undefined - ? {} - : { - cleanup: { - reason: cleanupReason, - resources: [sessionId, scriptId], - }, - }), - }), - ); + if (cancellation !== undefined) { + const failure = new FridaScriptLoadCancelledError(observation, { + cause: cancellation.reason, + scriptHandleAcquired: entry !== undefined, + ...(nativeFailure === undefined + ? {} + : { + diagnostics: { + native_failure: sanitizeError( + nativeFailure.cause, + managed.remote, + ), + }, + }), + ...(cleanupReason === undefined + ? {} + : { + cleanup: { + reason: cleanupReason, + resources: [sessionId, scriptId], + }, + }), + }); + if (entry !== undefined && failure.cleanupIncomplete) + this.#retainCleanupFailure(entry, failure); + return err(failure); + } return ok(observation); } @@ -624,16 +654,18 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { if (entry === undefined) return err(unknownFridaScript("unload_frida_script", scriptId)); const cleanupReason = await this.#releaseScript(managed, entry); - return cleanupReason === undefined - ? ok(null) - : err( - providerError( - "unload_frida_script", - new Error(cleanupReason), - managed.remote, - cleanupReason, - ), - ); + if (cleanupReason === undefined) return ok(null); + return err( + this.#retainCleanupFailure( + entry, + providerError( + "unload_frida_script", + new Error(cleanupReason), + managed.remote, + cleanupReason, + ), + ), + ); } async #releaseScript( @@ -660,6 +692,7 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { } managed.scripts.delete(entry.id); managed.scriptCaptures.delete(entry.id); + await this.#resolveCleanupFailures(entry); return undefined; } @@ -768,19 +801,23 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { } if (failures.length > 0) return err( - new AnalysisCapabilityUnavailableError( - "frida", - "close_frida_session", - failures[0] ?? "Frida session cleanup failed", - { - cleanup: { - reason: failures.join("; "), - resources: [sessionId], + this.#retainCleanupFailure( + managed, + new AnalysisCapabilityUnavailableError( + "frida", + "close_frida_session", + failures[0] ?? "Frida session cleanup failed", + { + cleanup: { + reason: failures.join("; "), + resources: [sessionId], + }, }, - }, + ), ), ); this.#sessions.delete(sessionId); + await this.#resolveCleanupFailures(managed); return ok({ targetResumeObserved }); } @@ -795,21 +832,34 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { async #closeAll(): Promise { await Promise.allSettled(this.#operations); - const startupFailures = ( - await Promise.all( - [...this.#pendingStartups].map((pending) => - this.#cleanupStartup(pending), - ), - ) - ).flat(); + const startupFailures = await Promise.all( + [...this.#pendingStartups].map(async (pending) => { + const reasons = await this.#cleanupStartup(pending); + return reasons.length === 0 + ? [] + : [ + this.#retainCleanupFailure( + pending, + new ProviderCleanupError( + "frida", + [pending.id], + { + reason: reasons.join("; "), + }, + { operation: "frida_cleanup" }, + ), + ), + ]; + }), + ); const results = await Promise.all( [...this.#sessions.keys()].map((sessionId) => this.closeSession(sessionId), ), ); const failures = [ - ...startupFailures, - ...results.flatMap((result) => (result.ok ? [] : [result.error.message])), + ...startupFailures.flat(), + ...results.flatMap((result) => (result.ok ? [] : [result.error])), ]; const manager = this.#deviceManager; if (manager !== undefined) { @@ -817,18 +867,37 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { if (reference.references !== 0) continue; try { await this.#releaseRemote(manager, address); - } catch { + } catch (cause) { failures.push( - "Unable to close remote Frida connection during shutdown", + this.#retainCleanupFailure( + reference, + new ProviderCleanupError( + "frida", + [address], + { + reason: + "Unable to close remote Frida connection during shutdown", + }, + { cause, operation: "frida_cleanup" }, + ), + ), ); } } } if (failures.length > 0) - throw new AggregateError( - failures, - "Frida session cleanup was incomplete", - ); + throw new ProviderCleanupError( + "frida", + failures.flatMap((failure) => failure.cleanupResources), + { reason: "Frida session cleanup was incomplete" }, + { + operation: "frida_cleanup", + cause: new AggregateError( + failures, + "Frida session cleanup was incomplete", + ), + }, + ).bindCleanupDependencies(failures); } #cleanupStartup(pending: PendingStartupCleanup): Promise { @@ -857,10 +926,36 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { const remoteFailure = await this.#releaseManagedRemote(pending); if (remoteFailure !== undefined) return [remoteFailure]; this.#pendingStartups.delete(pending); + await this.#resolveCleanupFailures(pending); return []; }); } + #retainCleanupFailure( + owner: { readonly id: string }, + failure: Failure, + ): Failure { + const lease = this.#cleanupLease(owner); + failure.bindCleanupResourceOwner(lease); + lease.bindCleanupError(failure); + return failure; + } + + #cleanupLease(owner: { readonly id: string }): AnalysisCleanupLease { + let lease = this.#cleanupLeases.get(owner); + if (lease === undefined) { + // The manager confirms native release before settling this private lease; + // its callback must not re-enter the session or remote cleanup lock. + lease = new AnalysisCleanupLease(async () => {}, [owner.id]); + this.#cleanupLeases.set(owner, lease); + } + return lease; + } + + async #resolveCleanupFailures(owner: { readonly id: string }): Promise { + await this.#cleanupLease(owner).release(); + } + async #releaseManagedRemote( managed: ManagedRemoteConnection, ): Promise { @@ -1188,7 +1283,12 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { remote.address, remoteOptions(remote), ); - reference = { references: 1, optionsDigest, device }; + reference = { + id: remote.address, + references: 1, + optionsDigest, + device, + }; this.#remoteDevices.set(remote.address, reference); } try { @@ -1220,6 +1320,7 @@ export class FridaInstrumentationManager implements FridaInstrumentationPort { return; } await manager.removeRemoteDevice(address); + await this.#resolveCleanupFailures(reference); if (this.#remoteDevices.get(address) === reference) this.#remoteDevices.delete(address); }); diff --git a/tests/boundary/cli/finishAnalysisCommand.test.ts b/tests/boundary/cli/finishAnalysisCommand.test.ts index fa24544f9b..0e6c2a6aca 100644 --- a/tests/boundary/cli/finishAnalysisCommand.test.ts +++ b/tests/boundary/cli/finishAnalysisCommand.test.ts @@ -72,3 +72,36 @@ it("preserves the primary operation error when cancellation arrives during close }, }); }); + +it("retains late cancellation and completed Evidence when close also fails", async () => { + const evidence = scriptCaptureEvidenceFixture(); + const controller = new AbortController(); + const cleanup = new ProviderCleanupError( + "fixture-provider", + ["retained connection"], + { reason: "close failed" }, + ); + const result = await finishAnalysisCommand( + async () => ok(evidence), + { + close: async () => { + controller.abort({ during: "close" }); + throw cleanup; + }, + }, + "fixture-provider", + "inspect_web_page", + controller.signal, + ); + expect(result).toMatchObject({ + code: "cleanup_incomplete", + category: "execution_failure", + details: { + partial_observation: evidence, + diagnostics: { + primary_error: { code: "cancelled" }, + cleanup_error: { details: { resources: ["retained connection"] } }, + }, + }, + }); +}); diff --git a/tests/boundary/cli/fridaCli.test.ts b/tests/boundary/cli/fridaCli.test.ts index 0fd08f5c01..3907869f05 100644 --- a/tests/boundary/cli/fridaCli.test.ts +++ b/tests/boundary/cli/fridaCli.test.ts @@ -102,7 +102,7 @@ it.each([false, true])( if (cleanupFails) expect(JSON.parse(stdout)).toMatchObject({ code: "cleanup_incomplete", - details: { diagnostics: { partial_observation: expected } }, + details: { partial_observation: expected }, }); else expect( @@ -213,6 +213,9 @@ it("maps one-shot Frida CLI target, script, duration, and remote options", async ); expect(exitCode).toBe(0); const result: unknown = JSON.parse(stdout); + expect(() => + toolContract("instrument_with_frida").outputSchema.parse(result), + ).not.toThrow(); expect(JSON.stringify(result)).not.toContain("synthetic-secret-token"); expect(calls).toEqual([ { @@ -359,14 +362,12 @@ it("keeps Frida script-file identity in original bytes when UTF-8 decoding repla if (name === "ff.js" || name === "fe.js") { expect(rawDigest).not.toBe(loadedSourceDigest); expect(output).toMatchObject({ - evidence: { - normalized_result: { source_sha256: rawDigest }, - raw_result: { - source_representation: { - encoding: "utf8", - decoding: "replacement", - loaded_source_sha256: loadedSourceDigest, - }, + normalized_result: { source_sha256: rawDigest }, + raw_result: { + source_representation: { + encoding: "utf8", + decoding: "replacement", + loaded_source_sha256: loadedSourceDigest, }, }, }); @@ -375,9 +376,7 @@ it("keeps Frida script-file identity in original bytes when UTF-8 decoding repla ); } else { expect(output).toMatchObject({ - evidence: { - normalized_result: { source_sha256: rawDigest }, - }, + normalized_result: { source_sha256: rawDigest }, }); expect(outputText).not.toContain("source_representation"); } @@ -388,7 +387,7 @@ it("keeps Frida script-file identity in original bytes when UTF-8 decoding repla } }); -it("awaits failed startup cleanup and retains the primary error if cleanup still fails", async () => { +it("awaits failed startup cleanup without clearing an unbound cleanup marker", async () => { for (const cleanupFails of [false, true]) { const calls: string[] = []; const provider: FridaInstrumentationPort = @@ -435,7 +434,7 @@ it("awaits failed startup cleanup and retains the primary error if cleanup still expect(calls).toEqual(["start", "cleanup"]); const output: unknown = JSON.parse(stdout); expect(output).toMatchObject({ - code: cleanupFails ? "cleanup_incomplete" : "capability_unavailable", + code: "cleanup_incomplete", }); expect(stdout).toContain("attach failed after spawn"); if (cleanupFails) expect(stdout).toContain("primary_error"); @@ -469,17 +468,15 @@ it("preserves observed devices when CLI provider cleanup fails", async () => { expect(output).toMatchObject({ code: "cleanup_incomplete", details: { - diagnostics: { - partial_observation: { - devices: [ - { - device_id: "remote-device", - name: "selected endpoint", - type: "remote", - }, - ], - cleanup_error: null, - }, + partial_observation: { + devices: [ + { + device_id: "remote-device", + name: "selected endpoint", + type: "remote", + }, + ], + cleanup_error: null, }, }, }); diff --git a/tests/process-global/fridaFinalizationCli.test.ts b/tests/process-global/fridaFinalizationCli.test.ts new file mode 100644 index 0000000000..c20189ef4f --- /dev/null +++ b/tests/process-global/fridaFinalizationCli.test.ts @@ -0,0 +1,113 @@ +import { expect, it } from "vitest"; +import { createCli } from "../../src/cli.js"; +import { FridaInstrumentationManager } from "../../src/frida/FridaInstrumentationManager.js"; +import { ProviderCleanupError } from "../../src/domain/providerCleanupError.js"; +import { ok } from "../../src/domain/result.js"; +import { toolContract } from "../../src/contracts/toolContracts.js"; +import { jsonObjectSchema } from "../../src/domain/jsonValue.js"; + +it.each([ + ["list-frida-devices", "SIGINT", false], + ["list-frida-processes", "SIGTERM", true], + ["instrument-with-frida", "SIGINT", true], +] as const)( + "retains completed %s output when %s arrives during final close (failure=%s)", + async (command, interrupt, cleanupFails) => { + const initialListeners = new Set(process.listeners(interrupt)); + const originalExitCode = process.exitCode; + const provider = new FridaInstrumentationManager(); + provider.listDevices = async () => ok({ devices: [], cleanupError: null }); + provider.listProcesses = async () => + ok({ + deviceId: "local", + processes: [{ pid: 71, name: "observed", identifier: null }], + cleanupError: null, + }); + provider.startSession = async () => + ok({ + sessionId: "00000000-0000-4000-8000-000000000001", + deviceId: "local", + target: "observed", + pid: 71, + mode: "attach", + state: "running", + }); + provider.loadScript = async () => + ok({ + scriptId: "00000000-0000-4000-8000-000000000002", + sourceKind: "inline", + sourcePath: null, + sourceSha256: "a".repeat(64), + messages: [{ type: "send", payload: "completed" }], + messagesTruncated: false, + }); + provider.closeSession = async () => ok({ targetResumeObserved: false }); + const cleanup = new ProviderCleanupError("frida", ["retained connection"], { + reason: "final close denied", + }); + let closes = 0; + provider.closeAll = async () => { + closes += 1; + const listeners = process + .listeners(interrupt) + .filter((listener) => !initialListeners.has(listener)); + expect(listeners).toHaveLength(1); + listeners[0]?.call(process); + if (cleanupFails) throw cleanup; + }; + const args = + command === "instrument-with-frida" + ? [ + command, + "attach", + "71", + "--script", + "send('completed')", + "--duration-ms", + "0", + "--json", + ] + : [command, "--json"]; + let stdout = ""; + try { + await createCli({}, undefined, provider).serve(args, { + env: {}, + exit: () => {}, + stdout: (text) => { + stdout += text; + }, + }); + const output: unknown = JSON.parse(stdout); + expect(output).toMatchObject({ + code: cleanupFails ? "cleanup_incomplete" : "cancelled", + details: cleanupFails + ? { + resources: ["retained connection"], + diagnostics: { primary_error: { code: "cancelled" } }, + } + : {}, + }); + const observation = jsonObjectSchema.parse( + jsonObjectSchema.parse(output).details, + ).partial_observation; + const name = + command === "list-frida-devices" + ? "list_frida_devices" + : command === "list-frida-processes" + ? "list_frida_processes" + : "instrument_with_frida"; + expect(() => + toolContract(name).outputSchema.parse(observation), + ).not.toThrow(); + if (command === "instrument-with-frida") + expect(observation).toMatchObject({ + normalized_result: { messages: [{ payload: "completed" }] }, + }); + expect(closes).toBe(1); + expect(process.exitCode).toBe(interrupt === "SIGINT" ? 130 : 143); + expect(process.listeners(interrupt)).toEqual([...initialListeners]); + } finally { + process.exitCode = originalExitCode; + } + }, +);