From da79630c2fc881ed3ecb75016aa93f514ea11e55 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 03:42:28 -0700 Subject: [PATCH] fix(pty): retire the node-pty master fd instead of probing around it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-pty hands the master to libuv, which closes it on EIO/EOF, but `_close()` never invalidated `_fd` and neither `resize()` nor the `process` getter consulted anything. A late ioctl therefore addressed whatever the kernel had since reissued that number to. On Linux the very next pty gets it: resizing an exited handle silently resized an unrelated live pane, with no error for #17832's probe-retry-classify to observe. Retire `_fd` in the same block that gives up the master and guard both fd-addressed syscalls on it, so a stale handle is unreachable by construction. The relay's resize keeps the isProcessAlive-proven retirement — still the only evidence of `exited` — and drops the re-probe-and-classify arm the ioctl no longer needs. Windows is unaffected: WindowsTerminal.resize goes through the conpty agent and reads no fd. --- config/patches/node-pty@1.1.0.patch | 42 +++++- pnpm-lock.yaml | 6 +- .../pty/node-pty-master-fd-retirement.test.ts | 120 ++++++++++++++++++ .../pty-handler-resize-stale-pty.test.ts | 16 ++- src/relay/pty-handler.ts | 29 ++--- 5 files changed, 189 insertions(+), 24 deletions(-) create mode 100644 src/main/pty/node-pty-master-fd-retirement.test.ts diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 348ce6ef7ce..f008b7c2adf 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -138,8 +138,24 @@ index 8c4fca9022a6d6f015bca87f61625cde2278f428..0a01730616488119aa21ef441cf3c441 process.exit(0); //# sourceMappingURL=conpty_console_list_agent.js.map \ No newline at end of file +diff --git a/lib/terminal.js b/lib/terminal.js +index e2f9bc9131077b53ebc32d207207ad82804ff185..a6af9300900eafa1c695c4207ba550f64937e542 100644 +--- a/lib/terminal.js ++++ b/lib/terminal.js +@@ -172,6 +172,11 @@ var Terminal = /** @class */ (function () { + this.end = function () { }; + this._writable = false; + this._readable = false; ++ // Orca: libuv closes the master fd on EIO/EOF, and the kernel may hand ++ // that number straight to the next open(2). Retire it in the same block ++ // that gives up the handle so no later ioctl can address a reused fd. ++ // Inert on Windows, where `_fd` is written once and never read back. ++ this._fd = -1; + }; + Terminal.prototype._parseEnv = function (env) { + var keys = Object.keys(env || {}); diff --git a/lib/unixTerminal.js b/lib/unixTerminal.js -index 1ec12f796a822c78fba9ad7f6448c3987e325c23..cec8b67aef02f8199e5606a0d257088bf1865877 100644 +index 1ec12f796a822c78fba9ad7f6448c3987e325c23..09176dcccaa3a040acfbb8ee9534f020f034befd 100644 --- a/lib/unixTerminal.js +++ b/lib/unixTerminal.js @@ -28,8 +28,12 @@ var native = utils_1.loadNativeModule('pty'); @@ -157,6 +173,30 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..cec8b67aef02f8199e5606a0d257088b var DEFAULT_FILE = 'sh'; var DEFAULT_NAME = 'xterm'; var DESTROY_SOCKET_TIMEOUT_MS = 200; +@@ -234,6 +238,11 @@ var UnixTerminal = /** @class */ (function (_super) { + * Gets the name of the process. + */ + get: function () { ++ // Orca: tcgetpgrp on a retired fd would name whatever process now ++ // owns that descriptor, so a closed master reports the spawn file. ++ if (this._fd < 0) { ++ return this._file; ++ } + if (process.platform === 'darwin') { + var title = pty.process(this._fd); + return (title !== 'kernel_task') ? title : this._file; +@@ -250,6 +259,11 @@ var UnixTerminal = /** @class */ (function (_super) { + if (cols <= 0 || rows <= 0 || isNaN(cols) || isNaN(rows) || cols === Infinity || rows === Infinity) { + throw new Error('resizing must be done using positive cols and rows'); + } ++ // Orca: a retired master is unreachable rather than EBADF-or-worse; cols ++ // and rows stay at the last size actually applied instead of a claim. ++ if (this._fd < 0) { ++ return; ++ } + pty.resize(this._fd, cols, rows); + this._cols = cols; + this._rows = rows; diff --git a/src/conpty_console_list_agent.ts b/src/conpty_console_list_agent.ts index 181ccabbbe9c4948a9725fb1db907a68e9de01fc..67f31facf85562b67adbfbd04ce28ddd8eeb4a79 100644 --- a/src/conpty_console_list_agent.ts diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1abec6c0590..33cd3c2c1ba 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -115,7 +115,7 @@ patchedDependencies: '@xterm/addon-webgl@0.20.0-beta.299': 94687e89a0115e6e6aa102837f986debdc029c091527ee5eb4a4e17ceaf9473e '@xterm/xterm@6.1.0-beta.303': 98756bcedc402bcdb7c6ab7b015d2e59cd18e97b03a2c06a27e95bb3ba429d9d lint-staged@16.4.0: 7333b3837f80a7fbd045964db6d76ba4fc118e49134bdbabb00585b6b7b60673 - node-pty@1.1.0: 40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e + node-pty@1.1.0: 929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd importers: @@ -156,7 +156,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e) + version: 1.1.0(patch_hash=929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12194,7 +12194,7 @@ snapshots: node-int64@0.4.0: {} - node-pty@1.1.0(patch_hash=40b6b6b814c89a8a29c995495ea86cff2cf6766f42124b5702aedf4ce0565c0e): + node-pty@1.1.0(patch_hash=929c703f3743efdd25929a6f7fdca716e3b54fb4a931bc32e1481633ea17defd): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/pty/node-pty-master-fd-retirement.test.ts b/src/main/pty/node-pty-master-fd-retirement.test.ts new file mode 100644 index 00000000000..66571c78a99 --- /dev/null +++ b/src/main/pty/node-pty-master-fd-retirement.test.ts @@ -0,0 +1,120 @@ +import * as pty from 'node-pty' +import { describe, expect, it } from 'vitest' + +/** + * node-pty hands the master fd to libuv, which closes it on EIO/EOF, but upstream + * never invalidated `_fd`, and neither `resize()` nor the `process` getter consulted + * anything. Orca's patch retires `_fd` in the same block that gives up the handle + * (config/patches/node-pty@1.1.0.patch), which is what makes a stale handle + * unreachable instead of merely unlucky. + */ + +const POSIX_SHELL = '/bin/sh' + +function spawnPty(command: string, cols = 80, rows = 24): pty.IPty { + return pty.spawn(POSIX_SHELL, ['-c', command], { + name: 'xterm-256color', + cols, + rows, + cwd: process.cwd(), + env: { ...process.env } + }) +} + +function masterFd(term: pty.IPty): number { + return (term as unknown as { fd: number }).fd +} + +/** Run `command` to completion and let node-pty finish giving up the master. */ +async function retiredPty(command = 'exit 0'): Promise<{ term: pty.IPty; spawnFd: number }> { + const term = spawnPty(command) + const spawnFd = masterFd(term) + await new Promise((resolve) => { + term.onExit(() => resolve()) + }) + ;(term as unknown as { destroy?: () => void }).destroy?.() + await new Promise((resolve) => setTimeout(resolve, 400)) + return { term, spawnFd } +} + +// Windows never reaches this code: WindowsTerminal.resize goes through the conpty +// agent and reads no fd, so the sentinel is written and never consulted there. +const describeOnPosix = process.platform === 'win32' ? describe.skip : describe + +describeOnPosix('node-pty master fd retirement', () => { + it('invalidates the descriptor once it gives up the handle', async () => { + const { term, spawnFd } = await retiredPty() + + expect(spawnFd).toBeGreaterThanOrEqual(0) + expect(masterFd(term)).toBe(-1) + }, 15000) + + it('answers a resize past retirement without issuing the ioctl', async () => { + const { term } = await retiredPty() + + // Pre-patch this threw `ioctl(2) failed, EBADF` out of whatever called it. + expect(() => term.resize(200, 50)).not.toThrow() + // Geometry stays at the last size actually applied rather than claiming one + // that no descriptor ever received. + expect([term.cols, term.rows]).toEqual([80, 24]) + }, 15000) + + it('names the spawn file rather than tcgetpgrp on a retired descriptor', async () => { + const { term } = await retiredPty() + + expect(term.process).toBe(POSIX_SHELL) + }, 15000) +}) + +// Linux frees the master synchronously enough that the very next pty is handed the +// same descriptor number every time, which makes the reuse hazard directly +// observable rather than a race to reproduce. +const describeOnLinux = process.platform === 'linux' ? describe : describe.skip + +describeOnLinux('node-pty master fd reuse', () => { + it('cannot resize a live pty handed the retired descriptor number', async () => { + const { term: retired, spawnFd } = await retiredPty() + + const live = spawnPty('sleep 1; stty size') + let output = '' + live.onData((data) => { + output += data + }) + try { + // The premise of this test: the kernel really did reissue the number. If it + // stops holding, the assertion below would pass for the wrong reason. + expect(masterFd(live)).toBe(spawnFd) + + // Pre-patch this reached TIOCSWINSZ on `live`'s master and silently resized + // a terminal it has no relationship to — no error, nothing for a liveness + // probe of the retired pid to observe. + retired.resize(200, 50) + + await new Promise((resolve) => { + live.onExit(() => resolve()) + }) + expect(output.trim()).toBe('24 80') + } finally { + live.kill() + } + }, 15000) + + it('does not name a live pty foreground process off the retired descriptor', async () => { + const { term: retired, spawnFd } = await retiredPty() + + // `exec` replaces the shell, so the foreground pgrp's cmdline is distinct + // from the file this pty was spawned with. + const live = spawnPty('exec sleep 5') + try { + expect(masterFd(live)).toBe(spawnFd) + // Let the shell finish exec'ing, or its own cmdline is still the fallback. + await new Promise((resolve) => setTimeout(resolve, 300)) + + // Pre-patch this read tcgetpgrp off `live`'s master and reported `sleep`, + // attributing an unrelated pane's process to a pty that had already exited. + expect(retired.process).toBe(POSIX_SHELL) + } finally { + live.kill() + } + }, 15000) +}) diff --git a/src/relay/pty-handler-resize-stale-pty.test.ts b/src/relay/pty-handler-resize-stale-pty.test.ts index 6bdc12b95a3..c93e2e3c16b 100644 --- a/src/relay/pty-handler-resize-stale-pty.test.ts +++ b/src/relay/pty-handler-resize-stale-pty.test.ts @@ -36,7 +36,14 @@ import type { MockDispatcher } from './pty-handler-test-harness' const PTY_1 = testPtyId(1) const STALE_PID = 424_242 -/** node-pty's native `pty.resize` error when the master fd is already closed. */ +/** + * node-pty's native `pty.resize` error when the ioctl reaches a closed master. + * + * Orca's node-pty patch retires `_fd` when it gives up the master, so a real + * handle answers a late resize with a no-op instead. What is left for this + * handler to contain is the tick between libuv closing the fd on a read error + * and node-pty's own handler observing it. + */ function ebadfResize(): never { throw new Error('ioctl(2) failed, EBADF') } @@ -55,7 +62,7 @@ describe('PtyHandler.resize against a stale PTY handle', () => { })) resize = vi.fn() // A shell that exited without node-pty producing `onExit`: the record is - // still in the pool and undisposed, but the master fd behind it is closed. + // still in the pool and undisposed, but the master behind it is gone. mockPtySpawn.mockReturnValue({ ...mockPtyInstance, pid: STALE_PID, resize }) await dispatcher.callRequest('pty.spawn', {}) expect(handler.activePtyCount).toBe(1) @@ -80,7 +87,7 @@ describe('PtyHandler.resize against a stale PTY handle', () => { expect(handler.activePtyCount).toBe(0) }) - it('contains an ioctl failure whose process is still live, and keeps the record', () => { + it('contains an ioctl failure without re-classifying liveness, and keeps the record', () => { vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValue(true) resize.mockImplementation(ebadfResize) const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true) @@ -94,7 +101,8 @@ describe('PtyHandler.resize against a stale PTY handle', () => { dispatcher.callNotification('pty.resize', { id: PTY_1, cols: 90, rows: 30 }) ).not.toThrow() - // Loss of an fd is not evidence the shell exited, so the claim is retained. + // Loss of an fd observed the handle, not the host that owns the pid, so it is + // `unverifiable` and the claim is retained. Only the probe above retires. expect(handler.activePtyCount).toBe(1) expect(stderr.mock.calls.map(([line]) => String(line)).join('')).toContain( 'ioctl(2) failed, EBADF' diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 348245d3c88..fa908ae350b 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -2060,30 +2060,27 @@ export class PtyHandler { if (!managed || managed.disposed) { return } - // Why probe first (same probe attach() and listProcesses() run): a shell - // that exited without node-pty's `onExit` leaves this entry holding a closed - // master fd, and `UnixTerminal.resize` has no fd guard — the ioctl throws - // `ioctl(2) failed, EBADF` straight out of this notification handler into the - // dispatcher's generic parse-error catch, where it is logged and nothing - // else. Nothing retired the entry, so it kept being advertised as live and - // kept holding `activePtyCount` above zero, which is what stops a relay with + // Why probe (same probe attach() and listProcesses() run): a shell that + // exited without node-pty's `onExit` leaves an undisposed entry behind, and + // while it stays the relay keeps advertising a dead shell and keeps holding + // `activePtyCount` above zero, which is what stops a relay with // `relayGracePeriodSeconds: 0` from ever reaching its idle-no-ptys exit - // (#12423). + // (#12423). This is retirement, not ioctl safety: only ESRCH from the host + // that owns the pid is evidence of `exited`. if (this.reapPtyProvenExited(managed)) { return } + // Safety is the handle's own, not this probe's: node-pty retires `_fd` in + // the same block that gives up the master (config/patches/node-pty@1.1.0.patch), + // so a resize past that point is a no-op instead of a TIOCSWINSZ aimed at a + // closed — or kernel-reused — descriptor. What survives is the tick between + // libuv closing the fd on a read error and node-pty's handler seeing it, and + // an EBADF there is `unverifiable`: it observed the handle, not the host. try { managed.pty.resize(cols, rows) } catch (err) { - // A failed ioctl observed the handle, not the host's process table, so on - // its own it is `unverifiable`. Re-probe: a now-absent pid retires the - // entry, anything else keeps it and is contained here rather than - // escaping as a parse error on every later resize. - if (this.reapPtyProvenExited(managed)) { - return - } process.stderr.write( - `[pty-handler] resize failed for PTY ${id} whose process is still live or unverifiable: ${err instanceof Error ? err.message : String(err)}\n` + `[pty-handler] resize failed for PTY ${id}: ${err instanceof Error ? err.message : String(err)}\n` ) } }