From 013a4e02ff808edd91fac2ecf03747d4b829e508 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Fri, 16 Jan 2026 09:55:29 -0800 Subject: [PATCH] fix(core): fix PTY descriptor shell leak (#16773) --- .../services/shellExecutionService.test.ts | 41 +++++++++- .../src/services/shellExecutionService.ts | 26 ++++++- packages/core/src/utils/shell-utils.test.ts | 78 +++++++++++++++++++ packages/core/src/utils/shell-utils.ts | 31 ++++++++ 4 files changed, 171 insertions(+), 5 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 14ffafa3c0..e5c977f103 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -30,11 +30,20 @@ const mockIsBinary = vi.hoisted(() => vi.fn()); const mockPlatform = vi.hoisted(() => vi.fn()); const mockGetPty = vi.hoisted(() => vi.fn()); const mockSerializeTerminalToObject = vi.hoisted(() => vi.fn()); +const mockResolveExecutable = vi.hoisted(() => vi.fn()); // Top-level Mocks vi.mock('@lydell/node-pty', () => ({ spawn: mockPtySpawn, })); +vi.mock('../utils/shell-utils.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + resolveExecutable: mockResolveExecutable, + }; +}); vi.mock('node:child_process', async (importOriginal) => { const actual = await importOriginal(); return { @@ -45,7 +54,7 @@ vi.mock('node:child_process', async (importOriginal) => { vi.mock('../utils/textUtils.js', () => ({ isBinary: mockIsBinary, })); -vi.mock('os', () => ({ +vi.mock('node:os', () => ({ default: { platform: mockPlatform, constants: { @@ -136,6 +145,7 @@ describe('ShellExecutionService', () => { onExit: Mock; write: Mock; resize: Mock; + destroy: Mock; }; let mockHeadlessTerminal: { resize: Mock; @@ -153,6 +163,8 @@ describe('ShellExecutionService', () => { mockSerializeTerminalToObject.mockReturnValue([]); mockIsBinary.mockReturnValue(false); mockPlatform.mockReturnValue('linux'); + mockResolveExecutable.mockImplementation(async (exe: string) => exe); + process.env['PATH'] = '/test/path'; mockGetPty.mockResolvedValue({ module: { spawn: mockPtySpawn }, name: 'mock-pty', @@ -167,6 +179,7 @@ describe('ShellExecutionService', () => { onExit: Mock; write: Mock; resize: Mock; + destroy: Mock; }; mockPtyProcess.pid = 12345; mockPtyProcess.kill = vi.fn(); @@ -174,6 +187,7 @@ describe('ShellExecutionService', () => { mockPtyProcess.onExit = vi.fn(); mockPtyProcess.write = vi.fn(); mockPtyProcess.resize = vi.fn(); + mockPtyProcess.destroy = vi.fn(); mockHeadlessTerminal = { resize: vi.fn(), @@ -821,6 +835,31 @@ describe('ShellExecutionService', () => { ); }); }); + + describe('Resource Management', () => { + it('should destroy the PTY process and clear activePtys on exit', async () => { + await simulateExecution('ls -l', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }); + + expect(mockPtyProcess.destroy).toHaveBeenCalled(); + expect(ShellExecutionService['activePtys'].size).toBe(0); + }); + + it('should destroy the PTY process even if destroy throws', async () => { + mockPtyProcess.destroy.mockImplementation(() => { + throw new Error('Destroy failed'); + }); + + await expect( + simulateExecution('ls -l', (pty) => { + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); + }), + ).resolves.not.toThrow(); + + expect(ShellExecutionService['activePtys'].size).toBe(0); + }); + }); }); describe('ShellExecutionService child_process fallback', () => { diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 1509072fe5..91c1df4853 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -12,7 +12,11 @@ import { TextDecoder } from 'node:util'; import os from 'node:os'; import type { IPty } from '@lydell/node-pty'; import { getCachedEncodingForBuffer } from '../utils/systemEncoding.js'; -import { getShellConfiguration, type ShellType } from '../utils/shell-utils.js'; +import { + getShellConfiguration, + resolveExecutable, + type ShellType, +} from '../utils/shell-utils.js'; import { isBinary } from '../utils/textUtils.js'; import pkg from '@xterm/headless'; import { @@ -183,7 +187,7 @@ export class ShellExecutionService { const ptyInfo = await getPty(); if (ptyInfo) { try { - return this.executeWithPty( + return await this.executeWithPty( commandToExecute, cwd, onOutputEvent, @@ -445,14 +449,14 @@ export class ShellExecutionService { } } - private static executeWithPty( + private static async executeWithPty( commandToExecute: string, cwd: string, onOutputEvent: (event: ShellOutputEvent) => void, abortSignal: AbortSignal, shellExecutionConfig: ShellExecutionConfig, ptyInfo: PtyImplementation, - ): ShellExecutionHandle { + ): Promise { if (!ptyInfo) { // This should not happen, but as a safeguard... throw new Error('PTY implementation not found'); @@ -461,6 +465,14 @@ export class ShellExecutionService { const cols = shellExecutionConfig.terminalWidth ?? 80; const rows = shellExecutionConfig.terminalHeight ?? 30; const { executable, argsPrefix, shell } = getShellConfiguration(); + + const resolvedExecutable = await resolveExecutable(executable); + if (!resolvedExecutable) { + throw new Error( + `Shell executable "${executable}" not found in PATH or at absolute location. Please ensure the shell is installed and available in your environment.`, + ); + } + const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); const args = [...argsPrefix, guardedCommand]; @@ -660,6 +672,12 @@ export class ShellExecutionService { exited = true; abortSignal.removeEventListener('abort', abortHandler); this.activePtys.delete(ptyProcess.pid); + // Attempt to destroy the PTY to ensure FD is closed + try { + (ptyProcess as IPty & { destroy?: () => void }).destroy?.(); + } catch { + // Ignore errors during cleanup + } const finalize = () => { render(true); diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 862b2014bc..745e85ca30 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -20,7 +20,9 @@ import { initializeShellParsers, stripShellWrapper, hasRedirection, + resolveExecutable, } from './shell-utils.js'; +import path from 'node:path'; const mockPlatform = vi.hoisted(() => vi.fn()); const mockHomedir = vi.hoisted(() => vi.fn()); @@ -33,6 +35,20 @@ vi.mock('os', () => ({ homedir: mockHomedir, })); +const mockAccess = vi.hoisted(() => vi.fn()); +vi.mock('node:fs', () => ({ + default: { + promises: { + access: mockAccess, + }, + constants: { X_OK: 1 }, + }, + promises: { + access: mockAccess, + }, + constants: { X_OK: 1 }, +})); + const mockSpawnSync = vi.hoisted(() => vi.fn()); vi.mock('node:child_process', () => ({ spawnSync: mockSpawnSync, @@ -463,3 +479,65 @@ describe('hasRedirection (PowerShell via mock)', () => { expect(hasRedirection('echo "-> arrow"')).toBe(true); }); }); + +describe('resolveExecutable', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { ...originalEnv }; + mockAccess.mockReset(); + }); + + afterEach(() => { + process.env = originalEnv; + }); + + it('should return the absolute path if it exists and is executable', async () => { + const absPath = path.resolve('/usr/bin/git'); + mockAccess.mockResolvedValue(undefined); // success + expect(await resolveExecutable(absPath)).toBe(absPath); + expect(mockAccess).toHaveBeenCalledWith(absPath, 1); + }); + + it('should return undefined for absolute path if it does not exist', async () => { + const absPath = path.resolve('/usr/bin/nonexistent'); + mockAccess.mockRejectedValue(new Error('ENOENT')); + expect(await resolveExecutable(absPath)).toBeUndefined(); + }); + + it('should resolve executable in PATH', async () => { + const binDir = path.resolve('/bin'); + const usrBinDir = path.resolve('/usr/bin'); + process.env['PATH'] = `${binDir}${path.delimiter}${usrBinDir}`; + mockPlatform.mockReturnValue('linux'); + + const targetPath = path.join(usrBinDir, 'ls'); + mockAccess.mockImplementation(async (p: string) => { + if (p === targetPath) return undefined; + throw new Error('ENOENT'); + }); + + expect(await resolveExecutable('ls')).toBe(targetPath); + }); + + it('should try extensions on Windows', async () => { + const sys32 = path.resolve('C:\\Windows\\System32'); + process.env['PATH'] = sys32; + mockPlatform.mockReturnValue('win32'); + mockAccess.mockImplementation(async (p: string) => { + // Use includes because on Windows path separators might differ + if (p.includes('cmd.exe')) return undefined; + throw new Error('ENOENT'); + }); + + expect(await resolveExecutable('cmd')).toContain('cmd.exe'); + }); + + it('should return undefined if not found in PATH', async () => { + process.env['PATH'] = path.resolve('/bin'); + mockPlatform.mockReturnValue('linux'); + mockAccess.mockRejectedValue(new Error('ENOENT')); + + expect(await resolveExecutable('unknown')).toBeUndefined(); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index c5eb6e8cf9..230870415c 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -5,6 +5,8 @@ */ import os from 'node:os'; +import fs from 'node:fs'; +import path from 'node:path'; import { quote } from 'shell-quote'; import { spawn, @@ -37,6 +39,35 @@ export interface ShellConfiguration { shell: ShellType; } +export async function resolveExecutable( + exe: string, +): Promise { + if (path.isAbsolute(exe)) { + try { + await fs.promises.access(exe, fs.constants.X_OK); + return exe; + } catch { + return undefined; + } + } + const paths = (process.env['PATH'] || '').split(path.delimiter); + const extensions = + os.platform() === 'win32' ? ['.exe', '.cmd', '.bat', ''] : ['']; + + for (const p of paths) { + for (const ext of extensions) { + const fullPath = path.join(p, exe + ext); + try { + await fs.promises.access(fullPath, fs.constants.X_OK); + return fullPath; + } catch { + continue; + } + } + } + return undefined; +} + let bashLanguage: Language | null = null; let treeSitterInitialization: Promise | null = null; let treeSitterInitializationError: Error | null = null;