From d85374496965a8a997e1d13e98a27ff5e5ca1077 Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Tue, 13 Oct 2026 21:28:31 +0900 Subject: [PATCH] fix: preserve process identities in pwntools cleanup errors (#2284) Co-authored-by: morluto <76467478+morluto@users.noreply.github.com> --- src/native/pwntools/PwntoolsDecoder.ts | 23 ++++++++-------- src/process/OwnedCommand.ts | 19 +++++++++----- tests/boundary/process/ownedCommand.test.ts | 8 +++--- .../pwntoolsStageRetirement.test.ts | 26 ++++++++++++------- 4 files changed, 45 insertions(+), 31 deletions(-) diff --git a/src/native/pwntools/PwntoolsDecoder.ts b/src/native/pwntools/PwntoolsDecoder.ts index 169f25601..6aa456631 100644 --- a/src/native/pwntools/PwntoolsDecoder.ts +++ b/src/native/pwntools/PwntoolsDecoder.ts @@ -32,7 +32,10 @@ import { analysisErrorWithCleanupFailure } from "../../domain/analysisErrorClean import { ProviderAdapterError } from "../../domain/providerAdapterError.js"; import { ProviderCleanupError } from "../../domain/providerCleanupError.js"; import { err, ok, type Result } from "../../domain/result.js"; -import { runOwnedCommand } from "../../process/OwnedCommand.js"; +import { + ownedCommandResources, + runOwnedCommand, +} from "../../process/OwnedCommand.js"; import type { ProviderProcessSupervisor, OwnedProviderProcessSpawnOptions, @@ -562,20 +565,16 @@ export class PwntoolsDecoder< 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( this.identity.id, - [ - pending.process.launch.ownership?.runId ?? "owned-decoder", - ...(pending.root === undefined ? [] : [pending.root.path]), - // The retained group may still be live; name it as OwnedCommand does. - ...(pending.process.launch.ownership === undefined - ? [] - : [ - `pid:${pending.process.launch.ownership.leaderPid}`, - `process-group:${pending.process.launch.ownership.processGroupId}`, - ]), - ], + ownership === undefined + ? [ + "owned-decoder", + ...(pending.root === undefined ? [] : [pending.root.path]), + ] + : ownedCommandResources(ownership, pending.root?.path), { reason: stopped.reason, captured_output: { diff --git a/src/process/OwnedCommand.ts b/src/process/OwnedCommand.ts index 02fb4defc..d375971ae 100644 --- a/src/process/OwnedCommand.ts +++ b/src/process/OwnedCommand.ts @@ -9,6 +9,7 @@ import { } from "./ProviderProcess.js"; import { cleanupOwnedProcessGroup, + type OwnedProcessGroup, type ProcessCleanupFailure, } from "./ProcessOwnership.js"; import { @@ -34,6 +35,17 @@ export class OwnedCommandFailure extends Error { } } +/** Report recorded launch identities; these labels do not authorize signaling. */ +export const ownedCommandResources = ( + ownership: OwnedProcessGroup, + directory?: string, +): readonly string[] => [ + ownership.runId, + ...(directory === undefined ? [] : [directory]), + `pid:${ownership.leaderPid}`, + `process-group:${ownership.processGroupId}`, +]; + /** Execute a single owned process with independent cleanup and bounded diagnostic retention. */ export const runOwnedCommand = async ( spawn: OwnedProviderProcessSpawnOptions, @@ -208,12 +220,7 @@ const collectOwnedCommand = async ( ? {} : { cleanupResult: stopped.cleanupResult }), }, - [ - spawn.runId, - ...(spawn.cwd === undefined ? [] : [spawn.cwd]), - `pid:${launched.ownership.leaderPid}`, - `process-group:${launched.ownership.processGroupId}`, - ], + ownedCommandResources(launched.ownership, spawn.cwd), supervisor, ); if (failure !== undefined) diff --git a/tests/boundary/process/ownedCommand.test.ts b/tests/boundary/process/ownedCommand.test.ts index 34205af3d..ca0cb2db6 100644 --- a/tests/boundary/process/ownedCommand.test.ts +++ b/tests/boundary/process/ownedCommand.test.ts @@ -257,9 +257,11 @@ it.skipIf(process.platform === "win32").each(["cancelled", "timeout"] as const)( failures: [expect.objectContaining({ reason: "run-token-mismatch" })], }); expect(failure.cleanupResult).toEqual(cleanup); - expect(failure.resources).toEqual( - expect.arrayContaining([request.runId]), - ); + expect(failure.resources).toEqual([ + request.runId, + `pid:${launched.ownership.leaderPid}`, + `process-group:${launched.ownership.processGroupId}`, + ]); // An unresolved member does not grant authority to signal the shared group. const acquiredPid = launched.ownership.leaderPid; expect(() => process.kill(acquiredPid, 0)).not.toThrow(); diff --git a/tests/process-global/pwntoolsStageRetirement.test.ts b/tests/process-global/pwntoolsStageRetirement.test.ts index 6ec062535..7cdf0b48a 100644 --- a/tests/process-global/pwntoolsStageRetirement.test.ts +++ b/tests/process-global/pwntoolsStageRetirement.test.ts @@ -18,7 +18,10 @@ import { PWNTOOLS_MEMORY_FAILURE_EXIT, PWNTOOLS_PROVIDER_IDENTITY, } from "../../src/native/pwntools/PwntoolsRelease.js"; -import { cleanupOwnedProcessGroup } from "../../src/process/ProcessOwnership.js"; +import { + cleanupOwnedProcessGroup, + type OwnedProcessGroup, +} from "../../src/process/ProcessOwnership.js"; import { prepareProcessOwnershipInspection } from "../../src/process/ProcessOwnershipObservation.js"; import { spawnOwnedProviderProcess } from "../../src/process/ProviderProcess.js"; import { PrivateRuntimeRoot } from "../../src/process/PrivateRuntimeRoot.js"; @@ -232,12 +235,13 @@ it.runIf(supportedHost).each([ const { path, root, environment, basic } = await fixture(); let launches = 0; let cleanupAllowed = false; + let retainedOwnership: OwnedProcessGroup | undefined; const read = vi.spyOn(artifacts, "readStableArtifact"); const provider = new PwntoolsRecordedCrashProvider( environment, async (spawn) => { if (stage === "debugger" && ++launches === 1) return basic(spawn); - return changingWorker( + const launched = await changingWorker( spawn, join(spawn.cwd ?? "", "resource.failure"), { @@ -247,6 +251,8 @@ it.runIf(supportedHost).each([ }, () => cleanupAllowed, ); + retainedOwnership = launched.ownership; + return launched; }, async () => root, ); @@ -261,14 +267,14 @@ it.runIf(supportedHost).each([ if (result.ok) throw new Error("Expected unresolved process ownership"); const projected = projectAnalysisError(result.error); expect(projected.code).toBe("cleanup_incomplete"); - expect(projected).toMatchObject({ - details: { - resources: expect.arrayContaining([ - expect.stringMatching(/^pid:\d+$/u), - expect.stringMatching(/^process-group:\d+$/u), - ]), - }, - }); + 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}`, + ]); if (stage === "debugger") expect(projected).toMatchObject({ details: {