From b5ae3b8038ba99f7cdfac0af89b28d9a5f54a344 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 18 Aug 2026 04:24:31 -0400 Subject: [PATCH] fix(ssh): close the last three ways a silent tool passed for absence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three reproduced before fixing. An lsof that is present but cannot inspect anything reports nothing, and the release command read that silence as "no owner". It now proves lsof works by asking it about this shell's own process first — the relay runs as the same user, so an lsof that can see our files can see its socket. The /proc/net/unix match compared a line suffix, which a live socket whose own path ends with a space plus ours would satisfy, and reset interpolated the path into a grep pattern, which a path holding regex metacharacters breaks. Both now compare the pathname column exactly. Reset's inventory also separates a tool that failed from one that ran and found nothing. That inventory first used a {n} interval expression, which mawk — Debian's default awk — does not support, so the strip silently matched nothing and every Linux host answered "no owner" for a live socket. Verified in a Debian container across the whole matrix, including a PATH with no lsof, pgrep, grep or awk. Test and harness fixes for the same class of problem: the reset sandbox stripped PATH so aggressively that it removed awk and tr, sending the script down a branch no real host takes (and, on Linux, past the assertion); the harness now falls back to /proc for pid discovery and the descendant walk, since a slim image ships no procps and an empty ps listing would leave a daemon and its PTY behind. --- src/main/ssh/ssh-relay-live-daemon-harness.ts | 65 ++++++++++++++++++- src/main/ssh/ssh-relay-reset.test.ts | 20 +++++- src/main/ssh/ssh-relay-reset.ts | 15 ++++- src/main/ssh/ssh-relay-socket-owner.test.ts | 14 ++++ src/main/ssh/ssh-relay-socket-owner.ts | 13 +++- 5 files changed, 118 insertions(+), 9 deletions(-) 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.