From 31449715edbb47303aba9d3288ef3fb38d1f7fe4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:34:13 -0700 Subject: [PATCH] fix(pty): invalidate the descriptor when node-pty gives up the handle (#17930) Carried forward from PR #17930, which merged into this branch. Rebased onto current main; main's newer node-pty-fd-leak test is kept as-is. --- config/patches/node-pty@1.1.0.patch | 78 ++++++- .../pty/node-pty-master-fd-retirement.test.ts | 194 ++++++++++++++++++ .../pty-handler-resize-stale-pty.test.ts | 37 +++- src/relay/pty-handler.ts | 21 +- 4 files changed, 317 insertions(+), 13 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..ff474f7d95e 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -138,8 +138,34 @@ 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..6c63bfaaf75128d88f9a2efece13476348780cfd 100644 +--- a/lib/terminal.js ++++ b/lib/terminal.js +@@ -172,6 +172,21 @@ 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. ++ // Upstream named this mechanism in microsoft/node-pty#220 ("fd number got ++ // reattached to something else"), closed 2025-12-19 as completed after ++ // only improving the error message; #827 is still open. Windows guards in ++ // windowsPtyAgent.ts, Unix does not. Orca tracking: #18109. ++ this._fd = -1; ++ // Orca: the write stream holds its own copy of that number, so retiring ++ // `_fd` alone leaves the queued and in-flight writes addressing it. ++ // Undefined on Windows and on `UnixTerminal.open()` handles. ++ if (this._writeStream) { ++ this._writeStream.dispose(); ++ } + }; + 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..d838d795ecb9ea72e3bcc31113344947c006af7e 100644 --- a/lib/unixTerminal.js +++ b/lib/unixTerminal.js @@ -28,8 +28,12 @@ var native = utils_1.loadNativeModule('pty'); @@ -157,6 +183,56 @@ 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; +@@ -287,8 +301,15 @@ var CustomWriteStream = /** @class */ (function () { + CustomWriteStream.prototype.dispose = function () { + clearImmediate(this._writeImmediate); + this._writeImmediate = undefined; ++ // Orca: retire this stream's own copy of the master fd and drop what has ++ // not shipped, so nothing queued here reaches a reused descriptor. ++ this._fd = -1; ++ this._writeQueue.length = 0; + }; + CustomWriteStream.prototype.write = function (data) { ++ if (this._fd < 0) { ++ return; ++ } + // Writes are put in a queue and processed asynchronously in order to handle + // backpressure from the kernel buffer. + var buffer = typeof data === 'string' +@@ -304,7 +325,8 @@ var CustomWriteStream = /** @class */ (function () { + CustomWriteStream.prototype._processWriteQueue = function () { + var _this = this; + this._writeImmediate = undefined; +- if (this._writeQueue.length === 0) { ++ // Orca: an in-flight fs.write can re-enter here after dispose(). ++ if (this._fd < 0 || this._writeQueue.length === 0) { + return; + } + var task = this._writeQueue[0]; 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/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..a0b537de1ca --- /dev/null +++ b/src/main/pty/node-pty-master-fd-retirement.test.ts @@ -0,0 +1,194 @@ +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 none of the three fd-addressed surfaces consulted + * anything: `resize()`, the `process` getter, and `CustomWriteStream`, which holds + * its own plain-number copy of the fd taken at spawn. Orca's patch retires all + * three in the same block that gives up the handle + * (config/patches/node-pty@1.1.0.patch). + * + * Scope: this narrows the window, it does not close it. libuv closes the fd + * synchronously inside `uv_close`, before the JS `'close'` that runs `_close()`, + * so callers still need their own liveness verdict for that tick — and a relay + * host installs node-pty from npm, where this patch is not applied at all. + */ + +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 +} + +type CustomWriteStream = { _fd: number; _writeQueue: unknown[]; write(data: string): void } + +/** + * `Terminal._close()` already shadows `terminal.write` with a no-op, so the stream + * itself is the surface that still reached the fd: a residual `_writeQueue` and an + * in-flight `fs.write` both re-enter it after the close. + */ +function writeStream(term: pty.IPty): CustomWriteStream { + return (term as unknown as { _writeStream: CustomWriteStream })._writeStream +} + +/** + * Run `command` to completion and let node-pty finish giving up the master. + * + * `destroy: false` exercises only the EIO/EOF read-error path, which reaches + * `_close()` without ever calling `destroy()` — the path the exit of a shell + * actually takes, and the one the write stream was previously never told about. + */ +async function retiredPty( + command = 'exit 0', + { destroy = true }: { destroy?: boolean } = {} +): Promise<{ term: pty.IPty; spawnFd: number }> { + const term = spawnPty(command) + const spawnFd = masterFd(term) + await new Promise((resolve) => { + term.onExit(() => resolve()) + }) + if (destroy) { + ;(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('retires the write stream fd on _close(), not only on destroy()', async () => { + const { term } = await retiredPty('exit 0', { destroy: false }) + + // The stream copied the fd number at spawn, so `Terminal._fd = -1` alone + // leaves it addressing a descriptor the kernel may already have reissued. + expect(writeStream(term)._fd).toBe(-1) + + writeStream(term).write('x') + expect(writeStream(term)._writeQueue).toHaveLength(0) + }, 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. +// +// Which is also the constraint on writing a case here: the kernel hands out the +// lowest free number, so a case that returns while its live pty is still open +// leaks that descriptor into the next case's premise as an off-by-one. Await the +// exit, never a fixed sleep. +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('cannot write into a live pty handed the retired descriptor number', async () => { + const { term: retired, spawnFd } = await retiredPty('exit 0', { destroy: false }) + + const live = spawnPty('sleep 1') + let output = '' + live.onData((data) => { + output += data + }) + try { + expect(masterFd(live)).toBe(spawnFd) + + // Pre-patch this fs.write reached `live`'s master, and the line discipline + // echoed it straight back: a retired pane's bytes landing in an unrelated + // terminal. Nothing has to read them for the leak to be observable. + writeStream(retired).write('leak\r') + + // Await the exit rather than sleeping: a fixed wait leaves this descriptor + // open into the next case, whose `expect(masterFd(live)).toBe(spawnFd)` + // premise then sees the kernel hand out the lower number this pty was still + // holding. That is what broke `node 24 1/8` on Linux, where alone among the + // platforms these cases actually run. + await new Promise((resolve) => { + live.onExit(() => resolve()) + }) + expect(output).not.toContain('leak') + } 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..02039600bb0 100644 --- a/src/relay/pty-handler-resize-stale-pty.test.ts +++ b/src/relay/pty-handler-resize-stale-pty.test.ts @@ -36,7 +36,15 @@ 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 patched + * handle answers a late resize with a no-op. That leaves this handler two cases + * it still has to contain: the tick between libuv closing the fd and node-pty's + * own handler observing it, and a relay host, which installs node-pty from npm + * and has no such guard. + */ function ebadfResize(): never { throw new Error('ioctl(2) failed, EBADF') } @@ -55,7 +63,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 +88,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,13 +102,34 @@ 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' ) }) + it('retires the entry when the pid goes absent between the pre-probe and the ioctl', () => { + // The race the catch-block re-probe exists for, and the one a constant + // liveness mock cannot express: alive when the pre-probe asks, gone by the + // time the ioctl fails. libuv closes the master synchronously inside + // `uv_close`, so this window opens before any JS guard can be set — and on + // a relay host, where node-pty comes from npm, there is no JS guard at all. + vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValueOnce(true).mockReturnValueOnce(false) + resize.mockImplementation(ebadfResize) + const stderr = vi.spyOn(process.stderr, 'write').mockReturnValue(true) + + expect(() => + dispatcher.callNotification('pty.resize', { id: PTY_1, cols: 120, rows: 40 }) + ).not.toThrow() + + expect(resize).toHaveBeenCalledTimes(1) + expect(handler.activePtyCount).toBe(0) + // Proven `exited` retires silently; only live-or-unverifiable is reported. + expect(stderr).not.toHaveBeenCalled() + }) + it('still resizes a live PTY, with the clamped geometry', () => { vi.spyOn(ptyShellUtils, 'isProcessAlive').mockReturnValue(true) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index f6e8e75f48d..7afe5c30604 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -2084,18 +2084,23 @@ 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 } + // The patched node-pty retires `_fd` in the same block that gives up the + // master (config/patches/node-pty@1.1.0.patch), which makes a resize past + // that point a no-op rather than a TIOCSWINSZ aimed at a reused descriptor. + // That covers only part of the window and does not cover this process at + // all: libuv closes the fd synchronously inside `uv_close`, before the JS + // `'close'` that runs `_close()`, and a relay host installs node-pty from + // npm, where the patch is not applied. So the catch below stays. try { managed.pty.resize(cols, rows) } catch (err) {