From 10bbcc2cd4c781bbc4bff995b9ae230a43e5d372 Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 14 Sep 2026 00:17:08 -0700 Subject: [PATCH] fix(ssh): corroborate an empty lsof answer before calling an endpoint free #18304 decided enumerability after `lsof` runs, keying on stderr, non-numeric output, and an abnormal exit. One failure carries none of those signals: probing as a uid that does not own the socket's holder, `lsof -t -a -U ` exits 1 with no stdout and no stderr. Measured on Debian 12 against #18304's own probe, a live relay owned by root probed as `nobody`: probe uid path marker pids merged nobody held.sock lsof [] <- live relay holds it merged nobody stale.sock lsof [] <- genuinely nobody merged root held.sock lsof [10] merged root stale.sock lsof [] The first two rows are byte-identical, so nothing about lsof's answer can separate them. The first reaches `verdict: exited / evidence: no-holder`, which `classifySupersededRelay` maps to `stale-endpoint-removed` and `rm -f` on an inode a live relay is still holding. `hidepid=2` produces the same shape. A positive control does not solve this. Controlling on something the probe itself holds passes precisely when we are blind: as `nobody`, `lsof -t -p $$` returns a pid while the socket query returns nothing. Blindness is to *other* uids, and another uid's process is not ours to manufacture. /proc/net/unix is. It is world-readable and lists every bound unix socket regardless of owner, so an entry for the path alongside no reported pid proves lsof was blind rather than that the path is free. Only an otherwise-clean empty answer is corroborated; a reported pid still stands on its own, and the check is skipped when the answer was already unavailable. Same run, with this change: fixed nobody held.sock unavailable [] <- no longer reapable fixed nobody stale.sock lsof [] <- still reapable fixed root held.sock lsof [10] <- unchanged fixed root stale.sock lsof [] <- unchanged The marker can only ever move from `lsof` toward `unavailable`, so this never authorises an unlink that #18304 refuses. Off Linux there is no /proc/net/unix, the check returns false, and behaviour is exactly as before -- deliberately, because defaulting to `unavailable` there would stop every macOS host from reaping a stale endpoint and trade a rare destructive bug for a universal accumulation one. The tests are Linux-gated for the same reason, with an assertion that the evidence they depend on is actually present so the block cannot pass vacuously. --- .../child-process/posix-lsof-probe.test.ts | 98 ++++++++++++++++++- src/shared/child-process/posix-lsof-probe.ts | 22 +++++ 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/src/shared/child-process/posix-lsof-probe.test.ts b/src/shared/child-process/posix-lsof-probe.test.ts index 70c038b12fa..baf94be94b8 100644 --- a/src/shared/child-process/posix-lsof-probe.test.ts +++ b/src/shared/child-process/posix-lsof-probe.test.ts @@ -1,4 +1,5 @@ -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { createServer } from 'node:net' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, expect, it } from 'vitest' @@ -44,7 +45,14 @@ process.kill = function(pid, signal) { }; ` -async function runProbe(options: { signal?: string; census?: string; censusKillDenied?: boolean }) { +async function runProbe(options: { + signal?: string + census?: string + censusKillDenied?: boolean + /** Replaces the whole stub `lsof` body, for cases about what lsof answered. */ + lsof?: string + sockPath?: string +}) { const dir = mkdtempSync(join(tmpdir(), 'orca-lsof-lifecycle-')) const pidFile = join(dir, 'lsof.pid') const psPidFile = join(dir, 'ps.pid') @@ -52,7 +60,7 @@ async function runProbe(options: { signal?: string; census?: string; censusKillD writeFileSync(join(dir, 'preload.cjs'), PRELOAD) writeFileSync( join(dir, 'lsof'), - `#!/bin/sh\necho 123\n${options.signal ? 'exec sleep 60\n' : 'exit 2\n'}`, + options.lsof ?? `#!/bin/sh\necho 123\n${options.signal ? 'exec sleep 60\n' : 'exit 2\n'}`, { mode: 0o755 } ) if (options.census !== undefined) { @@ -61,7 +69,13 @@ async function runProbe(options: { signal?: string; census?: string; censusKillD const started = performance.now() const result = await runProcess({ program: process.execPath, - args: ['--require', join(dir, 'preload.cjs'), '-e', RELAY_LSOF_PROBE_JS, '/unused.sock'], + args: [ + '--require', + join(dir, 'preload.cjs'), + '-e', + RELAY_LSOF_PROBE_JS, + options.sockPath ?? '/unused.sock' + ], env: { ...process.env, ORCA_BACKGROUND_LAUNCH: '1', @@ -151,3 +165,79 @@ describe.skipIf(process.platform === 'win32')('lsof supervisor lifecycle', () => expect(result.elapsedMs).toBeLessThan(4000) }) }) + +/** + * When the probe runs as a uid that does not own the socket's holder, `lsof -t -a -U ` + * exits 1 with no stdout and no stderr. Measured on Debian 12: a live relay owned by root, + * probed as `nobody`, produced `exit=1 stdout=[] stderr=[]` — byte-identical to a genuinely + * stale socket, so no amount of inspecting lsof's answer can separate the two. That answer + * reaches `verdict: exited / evidence: no-holder`, which `classifySupersededRelay` turns into + * `rm -f` on an inode a live relay still holds. + * + * The stub reproduces that exact shape rather than switching uid, which needs root and two + * accounts. What separates the cases is /proc/net/unix, which is world-readable. + */ +const linuxOnly = describe.skipIf(process.platform !== 'linux') + +linuxOnly('lsof blindness to another uid', () => { + const BLIND_LSOF = '#!/bin/sh\nexit 1\n' + + it('has the evidence these assertions depend on', () => { + // Why asserted rather than assumed: without /proc/net/unix every case below passes + // vacuously, and a silently degraded suite would stop covering the only thing that + // separates "lsof could not see" from "nothing holds it". + expect(existsSync('/proc/net/unix')).toBe(true) + }) + + async function withBoundSocket(run: (sockPath: string) => Promise): Promise { + const dir = mkdtempSync(join(tmpdir(), 'orca-lsof-bound-')) + const sockPath = join(dir, 'held.sock') + const server = createServer(() => {}) + await new Promise((resolve) => { + server.listen(sockPath, () => resolve()) + }) + try { + return await run(sockPath) + } finally { + server.close() + rmSync(dir, { recursive: true, force: true }) + } + } + + it('reports unavailable when nothing was reported for a socket that is still bound', async () => { + const result = await withBoundSocket((sockPath) => runProbe({ lsof: BLIND_LSOF, sockPath })) + expect(result.stdout).toBe('unavailable\n\n') + }) + + it('still reports lsof for an empty answer about a path nothing has bound', async () => { + // The control that keeps this from becoming a universal accumulation bug: on a healthy + // host a genuinely stale socket has no /proc/net/unix entry and must stay reapable. + const dir = mkdtempSync(join(tmpdir(), 'orca-lsof-stale-')) + try { + const result = await runProbe({ lsof: BLIND_LSOF, sockPath: join(dir, 'never-bound.sock') }) + expect(result.stdout).toBe('lsof\n\n') + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + + it('keeps a reported pid even while the path is still bound', async () => { + const result = await withBoundSocket((sockPath) => + runProbe({ lsof: '#!/bin/sh\necho 123\nexit 0\n', sockPath }) + ) + expect(result.stdout).toBe('lsof\n123\n') + }) + + // Both neighbours of the bound path, because a substring test would pass one and fail the + // other: `` is a prefix of the bound path, and the bound path is a prefix of + // `.other`. Only exact equality answers correctly for both. + it.each([ + ['a prefix of the bound path', (sockPath: string) => sockPath.slice(0, -1)], + ['a path the bound one prefixes', (sockPath: string) => `${sockPath}.other`] + ])('does not treat %s as bound', async (_name, derive) => { + const result = await withBoundSocket((sockPath) => + runProbe({ lsof: BLIND_LSOF, sockPath: derive(sockPath) }) + ) + expect(result.stdout).toBe('lsof\n\n') + }) +}) diff --git a/src/shared/child-process/posix-lsof-probe.ts b/src/shared/child-process/posix-lsof-probe.ts index 858758b7979..664ed492456 100644 --- a/src/shared/child-process/posix-lsof-probe.ts +++ b/src/shared/child-process/posix-lsof-probe.ts @@ -20,6 +20,24 @@ function groupExists(pid) { try { process.kill(-pid, 0); return true; } catch (error) { return error.code !== 'ESRCH'; } } +// An empty lsof answer means "nobody holds it" only if lsof could see. Running as a uid that +// does not own the holder, lsof exits 1 with no stdout and no stderr -- byte-identical to a +// genuinely stale socket. /proc/net/unix is world-readable and lists every bound unix socket +// regardless of owner, so an entry for this path alongside no pid proves lsof was blind. +// Absent off Linux, where this returns false and the marker stays whatever it already was. +function pathStillBound(target) { + var text; + try { text = require('fs').readFileSync('/proc/net/unix', 'utf8'); } + catch (error) { return false; } + var lines = text.split('\n'); + for (var i = 0; i < lines.length; i++) { + // Num: RefCount Protocol Flags Type St Inode Path -- the path is the line remainder, so + // one containing spaces survives intact. + var match = lines[i].match(/^\S+:(?:\s+\S+){5}\s+\d+ (.*)$/); + if (match && match[1] === target) return true; + } + return false; +} function finish(unconfirmed) { if (finished) return; finished = true; @@ -36,6 +54,10 @@ function finish(unconfirmed) { unavailable = true; return false; }); + // Only an otherwise-clean empty answer needs corroborating; a reported pid stands on its own. + if (!unconfirmed && !unavailable && !stderrSeen && !pids.length && pathStillBound(process.argv[1])) { + unavailable = true; + } var marker = unconfirmed ? 'cleanup-unconfirmed' : (unavailable || stderrSeen ? 'unavailable' : 'lsof'); process.stdout.write(marker + '\n' + pids.join('\n') + '\n', function() { process.exit(0); }); }