fix(ssh): close the last three ways a silent tool passed for absence

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.
This commit is contained in:
Jinwoo-H
2026-08-18 04:24:31 -04:00
parent e891b2f505
commit b5ae3b8038
5 changed files with 118 additions and 9 deletions
+63 -2
View File
@@ -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<number, number[]>()
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<number | null> {
if (child.exitCode !== null) {
return Promise.resolve(child.exitCode)
+19 -1
View File
@@ -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.
+12 -3
View File
@@ -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',
@@ -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', () => {
+10 -3
View File
@@ -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<lines.length;i++){if(lines[i].slice(-tail.length)===tail){return "live"}}' +
'var lines=data.split(String.fromCharCode(10));' +
'for(var i=1;i<lines.length;i++){' +
'if(lines[i].replace(/^(?:[^ ]+ +){7}/,"")===p){return "live"}}' +
'return "absent"}' +
// Why ctime too: inode numbers are recycled, so dev+ino alone can match a socket that
// was unlinked and recreated inside the probe window. Same identity as relay.ts.
@@ -64,7 +67,11 @@ export function relaySocketReleaseCommand(nodePath: string, sockPath: string): s
// Why -a: lsof ORs selectors by default, which would match every Unix-socket holder (#8762).
return (
`sock=${shellEscape(sockPath)}; holders=; proof=none; ` +
'if command -v lsof >/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.