From e628cf35dae8497edea2d090277a945b66b89170 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Wed, 26 Aug 2026 17:51:36 -0700 Subject: [PATCH] fix(adapter-utils): harden the wrapper birth-time probe with exclusive create and identity-aware cleanup (#12248) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Adapter utilities run process sessions in local and remote environments. > - The remote process-session wrapper uses a probe file to verify directory creation time. > - A peer could pre-create the probe path or replace it before cleanup. > - This pull request uses exclusive create and file-descriptor identity checks to protect the probe. > - The benefit is safer cleanup and fail-closed behavior at the sandbox boundary. ## Linked Issues or Issue Description **What existing behavior does this improve?** The remote process-session wrapper creates and removes a birth-time probe file. The old path-based flow did not prove that the wrapper created the path or that the path still named the same file. **Current behavior** A sandbox peer can race with the probe path. The peer can pre-create a symbolic link or replace the probe before cleanup. The wrapper can then inspect or remove an object that it did not create. **Proposed behavior** The wrapper creates the probe with exclusive create. It reads `(dev, ino, ctimeMs)` from the open file descriptor. It removes the path only when a final identity read matches the created file. **Reason and benefit** This change prevents symlink-following during creation and avoids removal of a peer's replacement object. The wrapper still fails closed when it cannot prove a real creation time. **Breaking changes** None. The wrapper keeps its existing fail-closed capture behavior. ## What Changed - Create the birth-time probe with `fs.open(path, "wx")`. - Read probe identity with `fstat` from the open descriptor. - Remove the probe only after a matching final identity read. - Add focused race tests for ordinary cleanup and file, directory, and symbolic-link replacement. ## Verification - Run `pnpm --filter @paperclipai/adapter-utils exec tsc --noEmit`. - Run the focused suite `packages/adapter-utils/src/execution-target-stdin-race.test.ts`. - Confirm that the focused suite passes all 33 tests. ## Risks The change affects shared wrapper source for local and remote process sessions. An identity read or cleanup failure leaves the probe in place and stops capture. The focused tests cover the new race paths. ## Model Used OpenAI Codex, GPT-5, tool use and code execution. The model reviewed and prepared this pull request from the supplied implementation and test results. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../src/execution-target-stdin-race.test.ts | 321 +++++++++++++++++- .../adapter-utils/src/execution-target.ts | 99 +++++- 2 files changed, 407 insertions(+), 13 deletions(-) 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);