diff --git a/src/main/ssh/ssh-relay-live-daemon-harness.ts b/src/main/ssh/ssh-relay-live-daemon-harness.ts index 3a7f9bf54c1..e02c8e61b6d 100644 --- a/src/main/ssh/ssh-relay-live-daemon-harness.ts +++ b/src/main/ssh/ssh-relay-live-daemon-harness.ts @@ -8,7 +8,15 @@ import { RelayBridge } from './ssh-relay-bridge-client' import { randomBytes } from 'node:crypto' import { EventEmitter } from 'node:events' import type { Writable } from 'node:stream' -import { cpSync, existsSync, mkdirSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs' +import { + cpSync, + existsSync, + mkdirSync, + readFileSync, + readdirSync, + symlinkSync, + writeFileSync +} from 'node:fs' import { connect } from 'node:net' import { join } from 'node:path' import type { SshConnection } from './ssh-connection' @@ -39,7 +47,7 @@ export function isProcessAlive(pid: number): boolean { * leak these tests are about, reproduced on the machine running them. */ export function killProcessTree(pid: number): void { - const listing = spawnSync('ps', ['-eo', 'pid=,ppid=']).stdout?.toString() ?? '' + const listing = spawnSync('ps', ['-eo', 'pid=,ppid=']).stdout?.toString() || readProcPpids() const childrenByParent = new Map() for (const line of listing.split('\n')) { const [child, parent] = line.trim().split(/\s+/).map(Number) @@ -77,9 +85,62 @@ function pidsHoldingSockPath(sockPath: string): number[] { pids.push(pid) } } + // Why the /proc fallback: a slim Linux image ships without procps, and an empty listing + // would silently leave a daemon (and its PTY shell) running after teardown. + return pids.length > 0 ? pids : pidsFromProcCmdline(sockPath) +} + +function pidsFromProcCmdline(sockPath: string): number[] { + let entries: string[] + try { + entries = readdirSync('/proc') + } catch { + return [] + } + const pids: number[] = [] + for (const entry of entries) { + const pid = Number(entry) + if (!Number.isInteger(pid) || pid === process.pid) { + continue + } + try { + const argv = readFileSync(`/proc/${entry}/cmdline`, 'utf-8').split('\0') + if (argv.includes(sockPath) && argv.some((arg) => arg.endsWith('relay.js'))) { + pids.push(pid) + } + } catch { + /* vanished or unreadable */ + } + } return pids } +/** `pid ppid` per line from /proc, for hosts whose image ships no procps. */ +function readProcPpids(): string { + let entries: string[] + try { + entries = readdirSync('/proc') + } catch { + return '' + } + const rows: string[] = [] + for (const entry of entries) { + if (!Number.isInteger(Number(entry))) { + continue + } + try { + const status = readFileSync(`/proc/${entry}/status`, 'utf-8') + const ppid = /^PPid:\s*(\d+)/m.exec(status)?.[1] + if (ppid) { + rows.push(`${entry} ${ppid}`) + } + } catch { + /* vanished or unreadable */ + } + } + return rows.join('\n') +} + export function waitForExit(child: ChildProcess, timeoutMs = 20_000): Promise { if (child.exitCode !== null) { return Promise.resolve(child.exitCode) diff --git a/src/main/ssh/ssh-relay-reset.test.ts b/src/main/ssh/ssh-relay-reset.test.ts index 62bc07fd3b8..62a532f5ed5 100644 --- a/src/main/ssh/ssh-relay-reset.test.ts +++ b/src/main/ssh/ssh-relay-reset.test.ts @@ -31,6 +31,16 @@ fi` /** How the sandboxed host behaves while the reset script runs. */ type HostMode = 'lsof-finds-owner' | 'only-pgrep-finds-owner' | 'kill-refused' | 'no-process-tools' +function resolveTool(tool: string): string | null { + for (const dir of (process.env.PATH ?? '').split(delimiter)) { + const candidate = join(dir, tool) + if (dir && existsSync(candidate)) { + return candidate + } + } + return null +} + function writeExecutable(filePath: string, body: string): void { writeFileSync(filePath, `#!/bin/sh\n${body}\n`, { mode: 0o755 }) } @@ -124,7 +134,15 @@ printf '%s\\n' "$OWNER_PID"` } // Why a real sleep: the script's post-kill re-check is only meaningful once the owner has // actually gone, and a no-op stub made it observe a process that was still dying. - writeExecutable(join(binDir, 'sleep'), 'exec /bin/sleep "$@"') + // Why link the ordinary utilities: the no-process-tools host is one without lsof and + // pgrep, not one without a shell. Stripping PATH outright also removed awk and tr, and the + // script then took a branch no real host takes. + for (const tool of ['awk', 'sleep', 'tr']) { + const resolved = resolveTool(tool) + if (resolved) { + writeExecutable(join(binDir, tool), `exec ${resolved} "$@"`) + } + } // Why a shell function: it records what reset asked for, and in the refused mode it lets // the owner survive the signal the way a permission error would. diff --git a/src/main/ssh/ssh-relay-reset.ts b/src/main/ssh/ssh-relay-reset.ts index a0410b863eb..cfc270dda19 100644 --- a/src/main/ssh/ssh-relay-reset.ts +++ b/src/main/ssh/ssh-relay-reset.ts @@ -44,10 +44,19 @@ export async function forceStopRelayForTarget( // simply have been unable to look, and reset must not read that silence as a stop. // 0 = a process holds the socket, 1 = an inventory ran and none does, 2 = none ran. 'socket_listed() {', - ' if [ -r /proc/net/unix ]; then', - ' grep -q " $1$" /proc/net/unix && return 0 || return 1', + // Why the columns are spelled out below: mawk, Debian's default awk, does not support + // {n} interval expressions, and the strip silently matched nothing there. + // Why awk on the eighth column rather than grep on the line: the pathname may contain + // spaces and regex metacharacters, and an interpolated pattern would both over-match and + // fail on a legal path. Why the exit-code split: a missing or failing tool answers + // nothing, and reading that as "no owner" is the inference this change removes. + ' if [ -r /proc/net/unix ] && command -v awk >/dev/null 2>&1; then', + ' awk -v target="$1" \'NR>1{line=$0;' + + 'sub(/^[^ ]+ +[^ ]+ +[^ ]+ +[^ ]+ +[^ ]+ +[^ ]+ +[^ ]+ +/,"",line);' + + "if(line==target){found=1}}END{exit(found?0:1)}' /proc/net/unix", + ' case $? in 0) return 0 ;; 1) return 1 ;; *) return 2 ;; esac', ' fi', - ' if command -v lsof >/dev/null 2>&1; then', + ' if command -v lsof >/dev/null 2>&1 && [ -n "$(lsof -t -p $$ 2>/dev/null)" ]; then', ' [ -n "$(lsof -t -a -U "$1" 2>/dev/null)" ] && return 0 || return 1', ' fi', ' return 2', diff --git a/src/main/ssh/ssh-relay-socket-owner.test.ts b/src/main/ssh/ssh-relay-socket-owner.test.ts index 40bc0f43065..f8dda2ffe18 100644 --- a/src/main/ssh/ssh-relay-socket-owner.test.ts +++ b/src/main/ssh/ssh-relay-socket-owner.test.ts @@ -51,6 +51,20 @@ describe('relay socket release command', () => { expect(command).toContain('left=3') expect(command).toContain('ECONNREFUSED') }) + + it('needs an owner inventory, not just refusals, before it may unlink', () => { + // Why: refusals are an inference. /proc/net/unix, or an lsof proven able to inspect + // this user's own process, is the evidence — with neither the answer is unverifiable. + expect(command).toContain('/proc/net/unix') + expect(command).toContain('lsof -t -p $$') + expect(command).toContain('proof=none') + }) + + it('compares the pathname column exactly rather than matching a line suffix', () => { + // Why: a live socket whose own path ends with " " plus ours would match a suffix test, + // and a path holding regex metacharacters would break an interpolated pattern. + expect(command).toContain('replace(/^(?:[^ ]+ +){7}/,"")===p') + }) }) describe('relay socket release outcome', () => { diff --git a/src/main/ssh/ssh-relay-socket-owner.ts b/src/main/ssh/ssh-relay-socket-owner.ts index 473d9a2c00f..3f4994b35d4 100644 --- a/src/main/ssh/ssh-relay-socket-owner.ts +++ b/src/main/ssh/ssh-relay-socket-owner.ts @@ -31,10 +31,13 @@ const RELAY_SOCKET_RELEASE_SCRIPT = 'var net=require("net"),fs=require("fs"),p=process.argv[1],lsofProof=process.argv[2]==="lsof",' + `left=${PROBE_ATTEMPTS},mark=null,done=false,sock=null,timer=null;` + // "live" if an inventory lists the path, "absent" if one ran without it, null if none ran. + // Why the field strip rather than a suffix test: the pathname is everything after the + // seventh column, so a socket whose own path ends with " " + ours would match a suffix. 'function inventory(){var data;try{data=fs.readFileSync("/proc/net/unix","utf8")}' + 'catch(e){return lsofProof?"absent":null}' + - 'var lines=data.split(String.fromCharCode(10)),tail=" "+p;' + - 'for(var i=0;i/dev/null 2>&1; then ' + + // Why probe lsof against our own process first: an lsof that is present but cannot + // inspect anything reports nothing, and reading that silence as "no owner" is the + // inference this whole change exists to remove. The relay runs as this same user, so + // an lsof that can see our files can see its socket. + 'if command -v lsof >/dev/null 2>&1 && [ -n "$(lsof -t -p $$ 2>/dev/null)" ]; then ' + 'holders=$(lsof -t -a -U "$sock" 2>/dev/null); [ -n "$holders" ] || proof=lsof; fi; ' + 'if [ -n "$holders" ]; then echo LIVE; ' + // Why argv: passing the path and proof as arguments dodges quoting issues inside -e.