diff --git a/src/native/pwntools/PwntoolsDecoder.ts b/src/native/pwntools/PwntoolsDecoder.ts index a0b9682487..489d9fc792 100644 --- a/src/native/pwntools/PwntoolsDecoder.ts +++ b/src/native/pwntools/PwntoolsDecoder.ts @@ -1,3 +1,7 @@ +import { + AnalysisCleanupLease, + AnalysisCleanupLeaseGroup, +} from "../../artifacts/AnalysisCleanupLease.js"; import { bindArtifactFileCleanup, artifactFileCancellationCause, @@ -113,7 +117,13 @@ export class PwntoolsDecoder< #pendingCleanup: | { root: Pick | undefined; - process: ProviderProcessSupervisor | undefined; + rootLease: AnalysisCleanupLease; + process: + | { + readonly supervisor: ProviderProcessSupervisor; + readonly lease: AnalysisCleanupLease; + } + | undefined; failure: AnalysisError | undefined; diagnostics: AnalysisCapturedOutput | undefined; observation: AnalysisPartialObservation | undefined; @@ -290,6 +300,7 @@ export class PwntoolsDecoder< root = await this.createRuntime(); this.#pendingCleanup = { root, + rootLease: AnalysisCleanupLease.forCloseOwner(root, root.path), process: undefined, failure: undefined, diagnostics: undefined, @@ -487,25 +498,27 @@ export class PwntoolsDecoder< : result.error.capturedOutput; } const cleanup = await this.#completeCleanup(() => this.#retire()); - if (cleanup !== undefined) - return err( - result.ok - ? cleanup - : analysisErrorWithCleanupFailure(result.error, cleanup, OPERATION), + if (result.ok && options?.signal?.aborted) + result = err( + new AnalysisCancelledError(OPERATION, { + cause: options.signal.reason, + partialObservation: result.value, + capturedOutput: result.value.diagnostics, + }), ); - return result.ok && options?.signal?.aborted - ? err( - new AnalysisCancelledError(OPERATION, { - cause: options.signal.reason, - partialObservation: result.value, - capturedOutput: { - stdout: result.value.diagnostics.stdout, - stderr: result.value.diagnostics.stderr, - truncated: result.value.diagnostics.truncated, - }, - }), - ) - : result; + if (cleanup !== undefined) { + const failure = result.ok + ? cleanup + : analysisErrorWithCleanupFailure( + result.error, + cleanup, + OPERATION, + ).bindCleanupDependencies([result.error, cleanup]); + if (this.#pendingCleanup !== undefined) + this.#pendingCleanup.failure = failure; + return err(failure); + } + return result; } async #runCommand( @@ -537,8 +550,21 @@ export class PwntoolsDecoder< ? {} : { signal: options.signal }), ...(this.launcher === undefined ? {} : { launcher: this.launcher }), - retainCleanup: (owner) => { - pending.process = owner; + retainCleanup: (supervisor) => { + const ownership = supervisor.launch.ownership; + pending.process = { + supervisor, + lease: new AnalysisCleanupLease( + async () => { + const stopped = await supervisor.stop(); + if (stopped.status === "incomplete") + throw new Error(stopped.reason, { cause: stopped }); + }, + ownership === undefined + ? ["owned-decoder"] + : ownedCommandResources(ownership), + ), + }; }, }, ), @@ -555,7 +581,7 @@ export class PwntoolsDecoder< primary, cleanup, this.profile.operation, - ); + ).bindCleanupDependencies([primary, cleanup]); } if (!outcome.ok) throw outcome.error; return outcome.value; @@ -590,6 +616,7 @@ export class PwntoolsDecoder< { reason: observation.reason }, { operation: this.profile.operation, cause }, ); + bindArtifactFileCleanup(cause, resourceFailure); failure = failure === undefined ? resourceFailure @@ -597,7 +624,7 @@ export class PwntoolsDecoder< failure, resourceFailure, this.profile.operation, - ); + ).bindCleanupDependencies([failure, resourceFailure]); } if (failure !== undefined) throw failure; }) @@ -626,10 +653,12 @@ export class PwntoolsDecoder< async #retireProcess(): Promise { const pending = this.#pendingCleanup; if (pending?.process === undefined) return undefined; - const stopped = await pending.process.stop(); - const ownership = pending.process.launch.ownership; - if (stopped.status === "incomplete") - return new ProviderCleanupError( + const process = pending.process; + const ownership = process.supervisor.launch.ownership; + try { + await process.lease.release(); + } catch (cause: unknown) { + const failure = new ProviderCleanupError( this.identity.id, ownership === undefined ? [ @@ -638,10 +667,10 @@ export class PwntoolsDecoder< ] : ownedCommandResources(ownership, pending.root?.path), { - reason: stopped.reason, + reason: cause instanceof Error ? cause.message : String(cause), captured_output: { ...(pending.diagnostics ?? - capturedPwntoolsOutput(pending.process.snapshot())), + capturedPwntoolsOutput(process.supervisor.snapshot())), }, previous_error: pending.failure === undefined @@ -650,11 +679,18 @@ export class PwntoolsDecoder< }, { operation: this.profile.operation, + cause, ...(pending.observation === undefined ? {} : { partialObservation: pending.observation }), }, ); + new AnalysisCleanupLeaseGroup(failure, [ + process.lease, + pending.rootLease, + ]); + return failure; + } pending.process = undefined; return undefined; } @@ -681,6 +717,7 @@ export class PwntoolsDecoder< }, ); } + if (failure !== undefined) bindArtifactFileCleanup(failure.cause, failure); const processFailure = await this.#retireProcess(); if (processFailure !== undefined) { return failure === undefined @@ -689,13 +726,15 @@ export class PwntoolsDecoder< failure, processFailure, this.profile.operation, - ); + ).bindCleanupDependencies([failure, processFailure]); } + // Retained descriptors still own local evidence under this root. + if (failure !== undefined) return failure; const pending = this.#pendingCleanup; if (pending?.root !== undefined) { const root = pending.root; try { - await root.close(); + await pending.rootLease.release(); pending.root = undefined; } catch (cause: unknown) { const rootFailure = new ProviderCleanupError( @@ -719,14 +758,8 @@ export class PwntoolsDecoder< : { partialObservation: pending.observation }), }, ); - failure = - failure === undefined - ? rootFailure - : analysisErrorWithCleanupFailure( - failure, - rootFailure, - this.profile.operation, - ); + new AnalysisCleanupLeaseGroup(rootFailure, [pending.rootLease]); + failure = rootFailure; } } if (!this.#resources.hasPending && pending?.root === undefined) diff --git a/tests/process-global/pwntoolsCleanup.test.ts b/tests/process-global/pwntoolsCleanup.test.ts index f976c20832..6260e65f0d 100644 --- a/tests/process-global/pwntoolsCleanup.test.ts +++ b/tests/process-global/pwntoolsCleanup.test.ts @@ -59,6 +59,7 @@ it.runIf(supportedHost)( let pid: number | undefined; let launches = 0; let cleanupAllowed = false; + let rootRemovalAllowed = false; let releaseFixture: (() => Promise) | undefined; const provider = new PwntoolsLayoutProvider( { REA_PWNTOOLS_PYTHON: process.execPath }, @@ -85,11 +86,24 @@ it.runIf(supportedHost)( }), }; }, - () => - PrivateRuntimeRoot.create({ parent: workspace.root, prefix: "owned-" }), + async () => { + const root = await PrivateRuntimeRoot.create({ + parent: workspace.root, + prefix: "owned-", + }); + return { + path: root.path, + close: async () => { + if (!rootRemovalAllowed) + throw new Error("root removal refused after process retirement"); + await root.close(); + }, + }; + }, ); onTestFinished(async () => { cleanupAllowed = true; + rootRemovalAllowed = true; await provider.close(); await releaseFixture?.(); }); @@ -125,8 +139,16 @@ it.runIf(supportedHost)( await expect(access(ownedPath)).resolves.toBeUndefined(); cleanupAllowed = true; - await provider.close(); + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); expect(() => process.kill(workerPid, 0)).toThrow(); + expect(result.error.cleanupResources).toEqual([ownedPath]); + expect(result.error.cleanupIncomplete).toBe(true); + rootRemovalAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(refused.error.cleanupIncomplete).toBe(false); await expect(access(ownedPath)).rejects.toMatchObject({ code: "ENOENT" }); }, ); @@ -189,6 +211,7 @@ it.runIf(supportedHost)( expect(releases).toBe(2); removalAllowed = true; await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); diff --git a/tests/process-global/pwntoolsCliFinalization.test.ts b/tests/process-global/pwntoolsCliFinalization.test.ts index 8728e68bc2..97c0c09f0f 100644 --- a/tests/process-global/pwntoolsCliFinalization.test.ts +++ b/tests/process-global/pwntoolsCliFinalization.test.ts @@ -14,6 +14,7 @@ import { import { BinaryLayoutService } from "../../src/application/binaryDiagnostics/BinaryLayoutService.js"; import { createCli } from "../../src/cli.js"; import * as composition from "../../src/composition/binaryDiagnostics.js"; +import { resolvedAnalysisError } from "../../src/domain/analysisErrorBase.js"; import { projectAnalysisError } from "../../src/domain/analysisErrorProjection.js"; import { PwntoolsLayoutProvider } from "../../src/native/pwntools/PwntoolsLayoutProvider.js"; import type { PwntoolsLauncher } from "../../src/native/pwntools/PwntoolsDecoder.js"; @@ -437,3 +438,50 @@ it.runIf(supportedHost)( await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); + +it.runIf(supportedHost)( + "retains cancellation and decoded facts until failed final cleanup is retried", + async () => { + const { path, root, launch } = await fixture(); + const controller = new AbortController(); + const reason = { stage: "failed final cleanup" }; + let removalAllowed = false; + const provider = new PwntoolsLayoutProvider( + { REA_PWNTOOLS_PYTHON: process.execPath }, + launch, + async () => ({ + path: root.path, + close: async () => { + if (!removalAllowed) { + controller.abort(reason); + throw new Error("root removal refused"); + } + await root.close(); + }, + }), + ); + onTestFinished(async () => { + removalAllowed = true; + await provider.close(); + }); + const result = await provider.inspectAndClose( + { path }, + { signal: controller.signal }, + ); + if (result.ok) + throw new Error("Expected cancelled finalization with retained cleanup"); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { diagnostics: { primary_error: { code: "cancelled" } } }, + }); + expect(result.error.cleanupPrimary?.cause).toBe(reason); + expect(result.error.partialObservation).toBeDefined(); + expect(result.error.capturedOutput?.stdout).toBe("decoder output"); + expect(result.error.cleanupResources).toEqual([root.path]); + removalAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(resolvedAnalysisError(result.error).cause).toBe(reason); + await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); + }, +); diff --git a/tests/process-global/pwntoolsStageRetirement.test.ts b/tests/process-global/pwntoolsStageRetirement.test.ts index 7cdf0b48aa..3e24b47cd1 100644 --- a/tests/process-global/pwntoolsStageRetirement.test.ts +++ b/tests/process-global/pwntoolsStageRetirement.test.ts @@ -1,4 +1,4 @@ -import { access, mkdir, readFile, writeFile } from "node:fs/promises"; +import { access, mkdir, open, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { @@ -25,6 +25,10 @@ import { import { prepareProcessOwnershipInspection } from "../../src/process/ProcessOwnershipObservation.js"; import { spawnOwnedProviderProcess } from "../../src/process/ProviderProcess.js"; import { PrivateRuntimeRoot } from "../../src/process/PrivateRuntimeRoot.js"; +import { + withArtifactFileHandle, + ArtifactFileCleanupFailure, +} from "../../src/artifacts/ArtifactFileOperation.js"; import * as artifacts from "../../src/artifacts/readStableArtifact.js"; import { recordedCrashFixture, @@ -269,12 +273,15 @@ it.runIf(supportedHost).each([ expect(projected.code).toBe("cleanup_incomplete"); if (retainedOwnership === undefined) throw new Error("Expected retained process ownership"); - expect(projected.details?.resources).toEqual([ - retainedOwnership.runId, - root.path, - `pid:${retainedOwnership.leaderPid}`, - `process-group:${retainedOwnership.processGroupId}`, - ]); + expect(projected.details?.resources).toEqual( + expect.arrayContaining([ + retainedOwnership.runId, + root.path, + `pid:${retainedOwnership.leaderPid}`, + `process-group:${retainedOwnership.processGroupId}`, + ]), + ); + expect(projected.details?.resources).toHaveLength(4); if (stage === "debugger") expect(projected).toMatchObject({ details: { @@ -368,3 +375,104 @@ it.runIf(supportedHost)( await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); }, ); + +it.runIf(supportedHost).each(["decoder", "debugger"])( + "does not reinterpret a completed %s read when retention cancels after failed close", + async (stage) => { + const { path, root, environment, basic } = await fixture(); + const controller = new AbortController(); + const reason = { stage: "retention callback after owner completion" }; + const reply = join( + root.path, + ...(stage === "debugger" ? ["debugger"] : []), + "reply.json", + ); + const read = artifacts.readStableArtifact; + let injected = false; + let releaseAllowed = false; + const emergencyCloses: (() => Promise)[] = []; + vi.spyOn(artifacts, "readStableArtifact").mockImplementation( + async (...args) => { + if (args[0] !== reply || injected) return read(...args); + injected = true; + const resources = args[4]; + if (resources === undefined) + throw new Error("Reply must have an artifact resource owner"); + const file = await open(reply, "r"); + const close = file.close.bind(file); + emergencyCloses.push(close); + file.close = async () => { + if (!releaseAllowed) throw new Error("reply close refused"); + await close(); + }; + const retain = resources.retain.bind(resources); + const spy = vi + .spyOn(resources, "retain") + .mockImplementation((owner) => { + retain(owner); + controller.abort(reason); + }); + try { + return await withArtifactFileHandle( + reply, + file, + resources, + controller.signal, + () => read(...args), + ); + } finally { + spy.mockRestore(); + } + }, + ); + let launches = 0; + const provider = new PwntoolsRecordedCrashProvider( + environment, + async (spawn) => { + if (++launches === 1) return basic(spawn); + await writeFile( + reply, + JSON.stringify({ ok: true, value: recordedCrashDebuggerFixture() }), + ); + return spawnOwnedProviderProcess({ + ...spawn, + arguments: ["-e", "process.stdout.write('debugger diagnostic')"], + }); + }, + async () => root, + ); + onTestFinished(async () => { + releaseAllowed = true; + try { + await provider.close(); + } finally { + await Promise.all(emergencyCloses.map((close) => close())); + } + }); + const result = await provider.inspectAndClose( + { path, include_debugger_context: true }, + { signal: controller.signal }, + ); + if (result.ok) throw new Error("Expected the physical reply-close failure"); + expect(result.error._tag).toBe("AnalysisOutputError"); + expect(result.error.cause).toBeInstanceOf(ArtifactFileCleanupFailure); + if (!(result.error.cause instanceof ArtifactFileCleanupFailure)) + throw new Error("Expected exact file owner"); + expect(result.error.cause.fileCleanupFailure.outcome.kind).toBe( + "completed", + ); + expect( + result.error.cause.fileCleanupFailure.completionCancellation, + ).toBeUndefined(); + expect(result.error.cleanupIncomplete).toBe(true); + await expect(provider.close()).rejects.toMatchObject({ + cleanupIncomplete: true, + }); + await expect(access(root.path)).resolves.toBeUndefined(); + releaseAllowed = true; + await provider.close(); + expect(result.error.cleanupIncomplete).toBe(false); + expect(result.error.capturedOutput?.stdout).toContain("basic diagnostic"); + await expect(access(root.path)).rejects.toMatchObject({ code: "ENOENT" }); + }, +);