diff --git a/packages/adapter-utils/src/execution-target-stdin-race.test.ts b/packages/adapter-utils/src/execution-target-stdin-race.test.ts index 225adc00cc..4b5dfdc027 100644 --- a/packages/adapter-utils/src/execution-target-stdin-race.test.ts +++ b/packages/adapter-utils/src/execution-target-stdin-race.test.ts @@ -1,5 +1,6 @@ import { execFile as execFileCallback, spawn } from "node:child_process"; -import { chmod, mkdir, mkdtemp, readFile, readdir, rename, rm, writeFile } from "node:fs/promises"; +import { symlinkSync } from "node:fs"; +import { chmod, lstat, mkdir, mkdtemp, readFile, readdir, readlink, rename, rm, writeFile } from "node:fs/promises"; import net from "node:net"; import os from "node:os"; import path from "node:path"; @@ -745,6 +746,109 @@ describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () return fakeBirthtimePreloadPath; } + // A test-only preload for PAP-5355: it deterministically simulates a + // same-sandbox peer that wins the gap between the wrapper's final identity + // check and its removal call. nextProbeFileName() is deterministic (pid + + // call sequence), so this preload can compute the exact probe path the + // wrapper itself will check next. It patches fs.promises.lstat inside the + // wrapper's own process: the first time that call targets the expected + // probe path, it replaces the path with a peer-owned entry before the real + // lstat runs, so the wrapper observes the swapped entry's identity, not its + // own. This is the worst case for the wrapper (the swap always lands + // before the wrapper's very last look at the path), so a wrapper that + // still leaves the peer's entry untouched under this preload proves the + // fix for every less-adversarial timing too. It never runs unless a test + // opts in, and it never touches this test file's own process. + let probeSwapPreloadDir: string | null = null; + afterAll(async () => { + if (probeSwapPreloadDir) await rm(probeSwapPreloadDir, { recursive: true, force: true }).catch(() => undefined); + }); + let probeSwapPreloadPath: Promise | null = null; + async function getProbeSwapPreloadPath(): Promise { + if (!probeSwapPreloadPath) { + probeSwapPreloadPath = (async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-swap-preload-")); + probeSwapPreloadDir = dir; + const preloadPath = path.join(dir, "probe-swap-preload.cjs"); + await writeFile( + preloadPath, + [ + `const fs = require("fs");`, + `const path = require("path");`, + `const mode = process.env.PAPERCLIP_TEST_PROBE_SWAP_MODE;`, + `const seq = process.env.PAPERCLIP_TEST_PROBE_SWAP_SEQ;`, + `const symlinkTarget = process.env.PAPERCLIP_TEST_PROBE_SWAP_SYMLINK_TARGET;`, + `if (mode && seq) {`, + ` const expectedName = ".paperclip-birthtime-probe-" + process.pid + "-" + seq;`, + ` let swapped = false;`, + ` const originalLstat = fs.promises.lstat.bind(fs.promises);`, + ` fs.promises.lstat = async (candidatePath, opts) => {`, + ` if (!swapped && path.basename(String(candidatePath)) === expectedName) {`, + ` swapped = true;`, + ` try { fs.unlinkSync(candidatePath); } catch {}`, + ` if (mode === "file") fs.writeFileSync(candidatePath, "peer-owned-content");`, + ` else if (mode === "dir") fs.mkdirSync(candidatePath);`, + ` else if (mode === "symlink") fs.symlinkSync(symlinkTarget, candidatePath);`, + ` }`, + ` return originalLstat(candidatePath, opts);`, + ` };`, + `}`, + ].join("\n"), + "utf8", + ); + return preloadPath; + })(); + } + return probeSwapPreloadPath; + } + + // A test-only preload for PAP-5374: it simulates fstat() failing on the + // wrapper's own just-opened probe file descriptor, the one signal the real + // filesystem in this sandbox never produces on demand. nextProbeFileName() + // is deterministic (pid + call sequence), so this preload knows which + // fs.promises.open() call is the wrapper's probe write and patches only the + // FileHandle that call returns, leaving every other open() untouched. It + // never runs unless a test opts in, and it never touches this test file's + // own process. + let fstatFailurePreloadDir: string | null = null; + afterAll(async () => { + if (fstatFailurePreloadDir) await rm(fstatFailurePreloadDir, { recursive: true, force: true }).catch(() => undefined); + }); + let fstatFailurePreloadPath: Promise | null = null; + async function getFstatFailurePreloadPath(): Promise { + if (!fstatFailurePreloadPath) { + fstatFailurePreloadPath = (async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), "paperclip-fstat-failure-preload-")); + fstatFailurePreloadDir = dir; + const preloadPath = path.join(dir, "fstat-failure-preload.cjs"); + await writeFile( + preloadPath, + [ + `const fs = require("fs");`, + `const path = require("path");`, + `const seq = process.env.PAPERCLIP_TEST_FSTAT_FAILURE_SEQ;`, + `if (seq) {`, + ` const expectedName = ".paperclip-birthtime-probe-" + process.pid + "-" + seq;`, + ` const originalOpen = fs.promises.open.bind(fs.promises);`, + ` fs.promises.open = async (targetPath, flags, mode) => {`, + ` const handle = await originalOpen(targetPath, flags, mode);`, + ` if (path.basename(String(targetPath)) === expectedName) {`, + ` handle.stat = async () => {`, + ` throw new Error("EIO: simulated fstat failure for test");`, + ` };`, + ` }`, + ` return handle;`, + ` };`, + `}`, + ].join("\n"), + "utf8", + ); + return preloadPath; + })(); + } + return fstatFailurePreloadPath; + } + // Run the real emitted wrapper (either variant) as a node process, with no // sandbox and no bridge in front of it. The test owns the wrapper's node // ChildProcess handle directly, so it can observe the wrapper's own exit @@ -766,6 +870,15 @@ describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () // real "no usable creation time" filesystem is not reachable in this // sandbox, so the test simulates the exact Stats shape instead. fakeBirthtime?: { target: "sessionDir" | "stdinDir"; mode: "zero" | "followCtime" }; + // Makes the wrapper's own process observe a same-sandbox peer replacing + // its birth-time probe file, through the preload above (PAP-5355). seq 1 + // is sessionDir's probe (the first one captureSessionIdentity() runs). + probeSwap?: { seq: 1 | 2; mode: "file" | "dir" | "symlink"; symlinkTarget?: string }; + // Makes the wrapper's own process observe an fstat() failure on the open + // descriptor for its own birth-time probe file, through the preload above + // (PAP-5374). seq 1 is sessionDir's probe (the first one + // captureSessionIdentity() runs). + fstatFailure?: { seq: 1 | 2 }; }) { const sessionDir = await mkdtemp(path.join(options?.parentDir ?? os.tmpdir(), "paperclip-wrapper-lifecycle-")); cleanupDirs.push(sessionDir); @@ -794,6 +907,16 @@ describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () env.PAPERCLIP_TEST_FAKE_BIRTHTIME_MODE = options.fakeBirthtime.mode; execArgv.push("--require", await getFakeBirthtimePreloadPath()); } + if (options?.probeSwap) { + env.PAPERCLIP_TEST_PROBE_SWAP_SEQ = String(options.probeSwap.seq); + env.PAPERCLIP_TEST_PROBE_SWAP_MODE = options.probeSwap.mode; + if (options.probeSwap.symlinkTarget) env.PAPERCLIP_TEST_PROBE_SWAP_SYMLINK_TARGET = options.probeSwap.symlinkTarget; + execArgv.push("--require", await getProbeSwapPreloadPath()); + } + if (options?.fstatFailure) { + env.PAPERCLIP_TEST_FSTAT_FAILURE_SEQ = String(options.fstatFailure.seq); + execArgv.push("--require", await getFstatFailurePreloadPath()); + } const child = spawn(process.execPath, [...execArgv, wrapperPath], { cwd: sessionDir, @@ -847,6 +970,7 @@ describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () } return { + pid: child.pid, sessionDir, stdinDir, eventsDir, @@ -1814,4 +1938,199 @@ describe("deterministic remote process-session wrapper shutdown (PAP-5316)", () } expect(wrapper.stderrText()).toMatch(/Latching on a lost process session identity/); }, 15_000); + + it("T20 refuses to write through a probe path a sandbox peer pre-created as a symbolic link, and leaves that link and its target untouched", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-symlink-race-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t20-child.pid"); + const childPath = path.join(rootDir, "t20-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const sessionDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-symlink-session-")); + cleanupDirs.push(sessionDir); + const stdinDir = path.join(sessionDir, "stdin"); + await mkdir(stdinDir, { recursive: true }); + + const wrapperPath = path.join(sessionDir, "wrapper.mjs"); + await writeFile(wrapperPath, getProcessSessionRemoteSource({ outputToStdout: true }), "utf8"); + const config = { command: process.execPath, args: [childPath], cwd: sessionDir, env: {} }; + const commandPayload = Buffer.from(JSON.stringify(config), "utf8").toString("base64"); + + // A file this test owns, standing in for a file a sandbox peer already + // controls. The wrapper's probe write must never reach it. + const probeLinkTarget = path.join(rootDir, "t20-probe-target.txt"); + const knownContent = "t20-untouched-content"; + await writeFile(probeLinkTarget, knownContent, "utf8"); + + const child = spawn(process.execPath, [wrapperPath], { + cwd: sessionDir, + env: { + ...process.env, + PAPERCLIP_PROCESS_SESSION_DIR: sessionDir, + PAPERCLIP_PROCESS_SESSION_COMMAND_B64: commandPayload, + }, + stdio: ["ignore", "pipe", "pipe"], + }); + + // Wins the race to the probe path against the wrapper's own probe write. + // nextProbeFileName() is deterministic: it names + // ".paperclip-birthtime-probe--1" on the wrapper's first probe call, + // which always targets sessionDir. child.pid is available synchronously + // right after spawn() returns, well before the freshly spawned process + // has loaded Node or parsed its own script, so this synchronous + // symlinkSync call lands first. This is the same advantage a real + // sandbox peer racing to pre-create the path would have, so it gives the + // strongest proof: the real wrapper process, under the real race, must + // still refuse to follow the link. + const probePath = path.join(sessionDir, `.paperclip-birthtime-probe-${child.pid}-1`); + symlinkSync(probeLinkTarget, probePath); + + let stderrText = ""; + child.stderr.on("data", (chunk: Buffer) => { + stderrText += chunk.toString("utf8"); + }); + const exited = new Promise((resolve) => child.on("close", () => resolve())); + + await Promise.race([ + exited, + delay(8_000).then(() => { + throw new Error("The wrapper process did not exit."); + }), + ]); + + await expectNoLiveProcessByArgvSubstring(childPath); + expect(stderrText).toMatch(/could not be created exclusively/); + expect((await lstat(probePath)).isSymbolicLink()).toBe(true); + expect(await readFile(probeLinkTarget, "utf8")).toBe(knownContent); + }, 15_000); + + // ---- PAP-5355: identity-aware cleanup after a same-sandbox peer replaces + // the probe file this wrapper just created, in the gap between this + // wrapper's last identity check and its removal call. The probeSwap + // preload (see getProbeSwapPreloadPath above) simulates the worst-case + // timing for that gap deterministically, instead of racing real wall-clock + // time: it swaps the path the instant the wrapper itself looks at it for + // the last time before deciding whether to remove it. + + it("T21 still removes its own probe file normally when no peer ever replaces it", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-no-swap-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t21-child.pid"); + const childPath = path.join(rootDir, "t21-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + }); + await waitForTrackedChildPid(pidFile); + + const probePath = path.join(wrapper.sessionDir, `.paperclip-birthtime-probe-${wrapper.pid}-1`); + await waitFor(async () => !(await lstat(probePath).then(() => true).catch(() => false)), 4_000); + await expect(lstat(probePath)).rejects.toThrow(); + }, 15_000); + + it("T22 leaves a peer's replacement file untouched instead of deleting it", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-swap-file-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t22-child.pid"); + const childPath = path.join(rootDir, "t22-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + probeSwap: { seq: 1, mode: "file" }, + }); + await waitForTrackedChildPid(pidFile); + + const probePath = path.join(wrapper.sessionDir, `.paperclip-birthtime-probe-${wrapper.pid}-1`); + await waitFor(async () => (await readFile(probePath, "utf8").catch(() => null)) === "peer-owned-content", 4_000); + // The wrapper's own cleanup call already ran (the preload only swaps the + // path the moment the wrapper itself checks it). This delay proves that + // run settled and nothing removes the peer's file afterward. + await delay(200); + expect(await readFile(probePath, "utf8")).toBe("peer-owned-content"); + }, 15_000); + + it("T23 leaves a peer's replacement directory untouched instead of deleting it", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-swap-dir-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t23-child.pid"); + const childPath = path.join(rootDir, "t23-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + probeSwap: { seq: 1, mode: "dir" }, + }); + await waitForTrackedChildPid(pidFile); + + const probePath = path.join(wrapper.sessionDir, `.paperclip-birthtime-probe-${wrapper.pid}-1`); + await waitFor(async () => await lstat(probePath).then((stats) => stats.isDirectory()).catch(() => false), 4_000); + await delay(200); + expect((await lstat(probePath)).isDirectory()).toBe(true); + }, 15_000); + + it("T24 leaves a peer's replacement symbolic link and its target untouched instead of deleting or following it", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-swap-symlink-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t24-child.pid"); + const childPath = path.join(rootDir, "t24-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const linkTarget = path.join(rootDir, "t24-probe-target.txt"); + const knownContent = "t24-untouched-content"; + await writeFile(linkTarget, knownContent, "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + probeSwap: { seq: 1, mode: "symlink", symlinkTarget: linkTarget }, + }); + await waitForTrackedChildPid(pidFile); + + const probePath = path.join(wrapper.sessionDir, `.paperclip-birthtime-probe-${wrapper.pid}-1`); + await waitFor(async () => await lstat(probePath).then((stats) => stats.isSymbolicLink()).catch(() => false), 4_000); + await delay(200); + expect((await lstat(probePath)).isSymbolicLink()).toBe(true); + expect(await readlink(probePath)).toBe(linkTarget); + expect(await readFile(linkTarget, "utf8")).toBe(knownContent); + }, 15_000); + + it("T25 fails closed at capture when its own probe file's identity cannot be read, so no orphan wrapper or child ever starts polling", async () => { + const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-probe-fstat-failure-")); + cleanupDirs.push(rootDir); + const pidFile = path.join(rootDir, "t25-child.pid"); + const childPath = path.join(rootDir, "t25-child.mjs"); + await writeFile(childPath, trackedChildSource(pidFile), "utf8"); + + const wrapper = await startWrapperProcess({ + outputToStdout: false, + command: process.execPath, + args: [childPath], + fstatFailure: { seq: 1 }, + }); + + await Promise.race([ + wrapper.exited, + delay(8_000).then(() => { + throw new Error("The wrapper process did not exit."); + }), + ]); + expect(wrapper.stderrText()).toMatch(/its own probe file's identity could not be read/); + await expectNoLiveProcessByArgvSubstring(childPath); + + // With no verified identity for the probe file, the wrapper must not + // remove it by path alone: it leaves the file exactly as it created it, + // rather than risking removal of a different entry a peer may have put + // at the same path. + const probePath = path.join(wrapper.sessionDir, `.paperclip-birthtime-probe-${wrapper.pid}-1`); + expect((await lstat(probePath)).isFile()).toBe(true); + }, 15_000); }); diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 801af1ab55..2a083f8a01 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -2479,31 +2479,104 @@ function nextProbeFileName() { // change-time copy. Creating and removing a file inside a directory changes // that directory's OWN change time but never its true creation time, so a // birthtimeMs that moves across the probe is a change-time copy. Returns -// false both on a detected copy and on a probe that cannot run at all (for -// example a permission error): either way the caller must not trust the -// value. +// null when the value is proven real. Returns a stderr-ready reason string +// on any failure (a detected copy, or a probe that cannot run at all, for +// example a permission error or a pre-created probe path): either way the +// caller must not trust the value. +// +// The open uses the "wx" flag: exclusive create, fail if the path exists. +// A sandbox peer cannot pre-create the probe path as a symbolic link and +// have this call follow it, because "wx" fails closed on an existing path +// instead of following a link to it. +// +// Cleanup checks identity, not only ownership of the initial create. This +// wrapper reads the probe file's identity, (dev, ino, ctimeMs), off the open +// file descriptor itself (fstat), not off the path, so a peer that swaps the +// path in the short gap after create cannot poison the identity this +// wrapper trusts as its own. Right before removal, this wrapper reads the +// path's identity again and removes it only when that identity still +// matches. A same-sandbox peer that deletes the probe file and creates its +// own entry at the same path in between leaves a different identity behind, +// so this wrapper leaves that entry untouched instead of removing it. This +// covers a peer's replacement file, a peer's replacement directory, and a +// peer's replacement symbolic link alike, because all three change the +// identity this wrapper reads back. The identity check includes ctimeMs, +// not only (dev, ino): a filesystem can hand this call's freed inode number +// straight back out to a peer's very next create at the same path, so +// (dev, ino) alone can match a path this call no longer owns; ctimeMs resets +// on every create, so a peer's replacement carries a different one even when +// the inode number repeats. Node's filesystem API has no call that removes a +// path only when its identity still matches an earlier read as one atomic +// step, so a gap remains between this wrapper's final identity read and the +// removal call itself. A peer that wins this gap can put any entry at the +// probe path before the removal call runs. This can include a pre-existing +// file the peer renames into place, not only a file the peer creates fresh. +// The removal call then removes whatever entry sits at the probe path at +// that moment. Two bounds still hold on that removal. The path always +// stays under dirPath. If the entry is a symbolic link, the removal call +// removes the link itself instead of following it to a different target. +// A non-recursive removal call also fails if the entry is a directory. async function birthtimeSurvivesProbe(dirPath) { let before; try { before = (await fs.lstat(dirPath)).birthtimeMs; } catch { - return false; + return "its reported creation time could not be read"; } const probePath = path.posix.join(dirPath, nextProbeFileName()); + let handle; try { - await fs.writeFile(probePath, ""); + handle = await fs.open(probePath, "wx"); } catch { - return false; + return "its probe file could not be created exclusively (the path may already exist)"; + } + // fstat on the open handle names the exact inode this call just created. + // A path-based lstat here instead would be racy against a peer that swaps + // the path in the gap between the create above and the stat: fstat has no + // such gap, because a file descriptor keeps naming the inode it opened no + // matter what a later swap does to the path. + let ownedIdentity = null; + try { + const createdStats = await handle.stat(); + // ctimeMs guards against inode reuse; see the function comment above. + ownedIdentity = { dev: createdStats.dev, ino: createdStats.ino, ctimeMs: createdStats.ctimeMs }; + } catch { + ownedIdentity = null; } finally { + await handle.close().catch(() => undefined); + } + if (!ownedIdentity) { + // fstat on this call's own just-opened descriptor failed. This call then + // has no verified identity for the probe file it created, so it must not + // check or remove that file by path: a peer could already own the entry + // at that path, and a path-based removal here could delete the peer's + // entry instead of this call's own file. Fail closed right here instead + // of falling through to the birthtime comparison below, so a failed + // identity read can never let this probe report success. + return "its own probe file's identity could not be read from the open file descriptor"; + } + // The one gap the fs API cannot close: this lstat and the removal below + // are two separate calls, not one atomic "remove if identity still + // matches" step. A peer that wins this narrow gap can put any entry at + // the probe path, including a pre-existing file it renames into place, + // and the removal call below removes whatever entry is there when it + // runs. + const currentStats = await fs.lstat(probePath).catch(() => null); + const stillOwned = + currentStats !== null && + currentStats.dev === ownedIdentity.dev && + currentStats.ino === ownedIdentity.ino && + currentStats.ctimeMs === ownedIdentity.ctimeMs; + if (stillOwned) { await fs.rm(probePath, { force: true }).catch(() => undefined); } let after; try { after = (await fs.lstat(dirPath)).birthtimeMs; } catch { - return false; + return "its reported creation time could not be read"; } - return before === after; + return before === after ? null : "its reported creation time changed after a probe write"; } async function refuseUnusableCreationTime(label, dirPath, reason) { @@ -2530,12 +2603,14 @@ async function refuseUnusableCreationTime(label, dirPath, reason) { // it, run once here, before either directory's identity is captured. async function captureSessionIdentity() { try { - if (!(await birthtimeSurvivesProbe(sessionDir))) { - await refuseUnusableCreationTime("sessionDir", sessionDir, "its reported creation time changed after a probe write"); + const sessionProbeFailure = await birthtimeSurvivesProbe(sessionDir); + if (sessionProbeFailure) { + await refuseUnusableCreationTime("sessionDir", sessionDir, sessionProbeFailure); return; } - if (!(await birthtimeSurvivesProbe(stdinDir))) { - await refuseUnusableCreationTime("stdinDir", stdinDir, "its reported creation time changed after a probe write"); + const stdinProbeFailure = await birthtimeSurvivesProbe(stdinDir); + if (stdinProbeFailure) { + await refuseUnusableCreationTime("stdinDir", stdinDir, stdinProbeFailure); return; } const session = await statPathIdentity(sessionDir);