Merge pull request #72784 from skprabhanjan/fix-72650

Fix-72650 Shell paths should be validated before launched to prevent ambiguous errors

src/vs/workbench/contrib/terminal/browser/terminalInstance.ts

@@ -25,7 +25,7 @@ import { activeContrastBorder, scrollbarSliderActiveBackground, scrollbarSliderB
 import { ICssStyleCollector, ITheme, IThemeService, registerThemingParticipant } from 'vs/platform/theme/common/themeService';
 import { PANEL_BACKGROUND } from 'vs/workbench/common/theme';
 import { TerminalWidgetManager } from 'vs/workbench/contrib/terminal/browser/terminalWidgetManager';
-import { IShellLaunchConfig, ITerminalDimensions, ITerminalInstance, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_PANEL_ID, IWindowsShellHelper } from 'vs/workbench/contrib/terminal/common/terminal';
+import { IShellLaunchConfig, ITerminalDimensions, ITerminalInstance, ITerminalProcessManager, KEYBINDING_CONTEXT_TERMINAL_TEXT_SELECTED, NEVER_MEASURE_RENDER_TIME_STORAGE_KEY, ProcessState, TERMINAL_PANEL_ID, IWindowsShellHelper, SHELL_PATH_INVALID_EXIT_CODE } from 'vs/workbench/contrib/terminal/common/terminal';
 import { ansiColorIdentifiers, TERMINAL_BACKGROUND_COLOR, TERMINAL_CURSOR_BACKGROUND_COLOR, TERMINAL_CURSOR_FOREGROUND_COLOR, TERMINAL_FOREGROUND_COLOR, TERMINAL_SELECTION_BACKGROUND_COLOR } from 'vs/workbench/contrib/terminal/common/terminalColorRegistry';
 import { TERMINAL_COMMAND_ID } from 'vs/workbench/contrib/terminal/common/terminalCommands';
 import { TerminalConfigHelper } from 'vs/workbench/contrib/terminal/browser/terminalConfigHelper';
@@ -970,10 +970,32 @@ export class TerminalInstance implements ITerminalInstance {
 		}
 
 		this._isExiting = true;
-		let exitCodeMessage: string;
+		let exitCodeMessage: string | undefined;
 
+		// Create exit code message
 		if (exitCode) {
-			exitCodeMessage = nls.localize('terminal.integrated.exitedWithCode', 'The terminal process terminated with exit code: {0}', exitCode);
+			if (exitCode === SHELL_PATH_INVALID_EXIT_CODE) {
+				exitCodeMessage = nls.localize('terminal.integrated.exitedWithInvalidPath', 'The terminal shell path does not exist: {0}', this._shellLaunchConfig.executable);
+			} else if (this._processManager && this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) {
+				let args = '';
+				if (typeof this._shellLaunchConfig.args === 'string') {
+					args = ` ${this._shellLaunchConfig.args}`;
+				} else if (this._shellLaunchConfig.args && this._shellLaunchConfig.args.length) {
+					args = ' ' + this._shellLaunchConfig.args.map(a => {
+						if (typeof a === 'string' && a.indexOf(' ') !== -1) {
+							return `'${a}'`;
+						}
+						return a;
+					}).join(' ');
+				}
+				if (this._shellLaunchConfig.executable) {
+					exitCodeMessage = nls.localize('terminal.integrated.launchFailed', 'The terminal process command \'{0}{1}\' failed to launch (exit code: {2})', this._shellLaunchConfig.executable, args, exitCode);
+				} else {
+					exitCodeMessage = nls.localize('terminal.integrated.launchFailedExtHost', 'The terminal process failed to launch (exit code: {0})', exitCode);
+				}
+			} else {
+				exitCodeMessage = nls.localize('terminal.integrated.exitedWithCode', 'The terminal process terminated with exit code: {0}', exitCode);
+			}
 		}
 
 		this._logService.debug(`Terminal process exit (id: ${this.id})${this._processManager ? ' state ' + this._processManager.processState : ''}`);
@@ -981,8 +1003,8 @@ export class TerminalInstance implements ITerminalInstance {
 		// Only trigger wait on exit when the exit was *not* triggered by the
 		// user (via the `workbench.action.terminal.kill` command).
 		if (this._shellLaunchConfig.waitOnExit && (!this._processManager || this._processManager.processState !== ProcessState.KILLED_BY_USER)) {
-			if (exitCode) {
-				this._xterm.writeln(exitCodeMessage!);
+			if (exitCodeMessage) {
+				this._xterm.writeln(exitCodeMessage);
 			}
 			if (typeof this._shellLaunchConfig.waitOnExit === 'string') {
 				let message = this._shellLaunchConfig.waitOnExit;
@@ -997,29 +1019,14 @@ export class TerminalInstance implements ITerminalInstance {
 			}
 		} else {
 			this.dispose();
-			if (exitCode) {
+			if (exitCodeMessage) {
 				if (this._processManager && this._processManager.processState === ProcessState.KILLED_DURING_LAUNCH) {
-					let args = '';
-					if (typeof this._shellLaunchConfig.args === 'string') {
-						args = this._shellLaunchConfig.args;
-					} else if (this._shellLaunchConfig.args && this._shellLaunchConfig.args.length) {
-						args = ' ' + this._shellLaunchConfig.args.map(a => {
-							if (typeof a === 'string' && a.indexOf(' ') !== -1) {
-								return `'${a}'`;
-							}
-							return a;
-						}).join(' ');
-					}
-					if (this._shellLaunchConfig.executable) {
-						this._notificationService.error(nls.localize('terminal.integrated.launchFailed', 'The terminal process command \'{0}{1}\' failed to launch (exit code: {2})', this._shellLaunchConfig.executable, args, exitCode));
-					} else {
-						this._notificationService.error(nls.localize('terminal.integrated.launchFailedExtHost', 'The terminal process failed to launch (exit code: {0})', exitCode));
-					}
+					this._notificationService.error(exitCodeMessage);
 				} else {
 					if (this._configHelper.config.showExitAlert) {
-						this._notificationService.error(exitCodeMessage!);
+						this._notificationService.error(exitCodeMessage);
 					} else {
-						console.warn(exitCodeMessage!);
+						console.warn(exitCodeMessage);
 					}
 				}
 			}

src/vs/workbench/contrib/terminal/common/terminal.ts

@@ -58,6 +58,7 @@ export const TERMINAL_CONFIG_SECTION = 'terminal.integrated';
 export const DEFAULT_LETTER_SPACING = 0;
 export const MINIMUM_LETTER_SPACING = -5;
 export const DEFAULT_LINE_HEIGHT = 1;
+export const SHELL_PATH_INVALID_EXIT_CODE = -1;
 
 export type FontWeight = 'normal' | 'bold' | '100' | '200' | '300' | '400' | '500' | '600' | '700' | '800' | '900';
 
@@ -695,7 +696,6 @@ export const enum ProcessState {
 	KILLED_BY_PROCESS
 }
 
-
 export interface ITerminalProcessExtHostProxy extends IDisposable {
 	readonly terminalId: number;
 

src/vs/workbench/contrib/terminal/node/terminalProcess.ts

@@ -11,13 +11,13 @@ import * as fs from 'fs';
 import { Event, Emitter } from 'vs/base/common/event';
 import { getWindowsBuildNumber } from 'vs/workbench/contrib/terminal/node/terminal';
 import { IDisposable } from 'vs/base/common/lifecycle';
-import { IShellLaunchConfig, ITerminalChildProcess } from 'vs/workbench/contrib/terminal/common/terminal';
+import { IShellLaunchConfig, ITerminalChildProcess, SHELL_PATH_INVALID_EXIT_CODE } from 'vs/workbench/contrib/terminal/common/terminal';
 import { exec } from 'child_process';
 
 export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 	private _exitCode: number;
 	private _closeTimeout: any;
-	private _ptyProcess: pty.IPty;
+	private _ptyProcess: pty.IPty | undefined;
 	private _currentTitle: string = '';
 	private _processStartupComplete: Promise<void>;
 	private _isDisposed: boolean = false;
@@ -69,37 +69,39 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 			experimentalUseConpty: useConpty
 		};
 
-		try {
-			this._ptyProcess = pty.spawn(shellLaunchConfig.executable!, shellLaunchConfig.args || [], options);
-			this._processStartupComplete = new Promise<void>(c => {
-				this.onProcessIdReady((pid) => {
-					c();
-				});
-			});
-		} catch (error) {
-			// The only time this is expected to happen is when the file specified to launch with does not exist.
-			this._exitCode = 2;
-			this._queueProcessExit();
-			this._processStartupComplete = Promise.resolve(undefined);
-			return;
-		}
-		this._ptyProcess.on('data', (data) => {
+		fs.stat(shellLaunchConfig.executable!, (err) => {
+			if (err && err.code === 'ENOENT') {
+				this._exitCode = SHELL_PATH_INVALID_EXIT_CODE;
+				this._queueProcessExit();
+				this._processStartupComplete = Promise.resolve(undefined);
+				return;
+			}
+			this.setupPtyProcess(shellLaunchConfig, options);
+		});
+	}
+
+	private setupPtyProcess(shellLaunchConfig: IShellLaunchConfig, options: pty.IPtyForkOptions): void {
+		const ptyProcess = pty.spawn(shellLaunchConfig.executable!, shellLaunchConfig.args || [], options);
+		this._ptyProcess = ptyProcess;
+		this._processStartupComplete = new Promise<void>(c => {
+			this.onProcessIdReady(() => c());
+		});
+		ptyProcess.on('data', (data) => {
 			this._onProcessData.fire(data);
 			if (this._closeTimeout) {
 				clearTimeout(this._closeTimeout);
 				this._queueProcessExit();
 			}
 		});
-		this._ptyProcess.on('exit', (code) => {
+		ptyProcess.on('exit', (code) => {
 			this._exitCode = code;
 			this._queueProcessExit();
 		});
-
+		this._setupTitlePolling(ptyProcess);
 		// TODO: We should no longer need to delay this since pty.spawn is sync
 		setTimeout(() => {
-			this._sendProcessId();
+			this._sendProcessId(ptyProcess);
 		}, 500);
-		this._setupTitlePolling();
 	}
 
 	public dispose(): void {
@@ -114,15 +116,15 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 		this._onProcessTitleChanged.dispose();
 	}
 
-	private _setupTitlePolling() {
+	private _setupTitlePolling(ptyProcess: pty.IPty) {
 		// Send initial timeout async to give event listeners a chance to init
 		setTimeout(() => {
-			this._sendProcessTitle();
+			this._sendProcessTitle(ptyProcess);
 		}, 0);
 		// Setup polling
 		this._titleInterval = setInterval(() => {
-			if (this._currentTitle !== this._ptyProcess.process) {
-				this._sendProcessTitle();
+			if (this._currentTitle !== ptyProcess.process) {
+				this._sendProcessTitle(ptyProcess);
 			}
 		}, 200);
 	}
@@ -146,7 +148,9 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 			// Attempt to kill the pty, it may have already been killed at this
 			// point but we want to make sure
 			try {
-				this._ptyProcess.kill();
+				if (this._ptyProcess) {
+					this._ptyProcess.kill();
+				}
 			} catch (ex) {
 				// Swallow, the pty has already been killed
 			}
@@ -155,15 +159,15 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 		});
 	}
 
-	private _sendProcessId() {
-		this._onProcessIdReady.fire(this._ptyProcess.pid);
+	private _sendProcessId(ptyProcess: pty.IPty) {
+		this._onProcessIdReady.fire(ptyProcess.pid);
 	}
 
-	private _sendProcessTitle(): void {
+	private _sendProcessTitle(ptyProcess: pty.IPty): void {
 		if (this._isDisposed) {
 			return;
 		}
-		this._currentTitle = this._ptyProcess.process;
+		this._currentTitle = ptyProcess.process;
 		this._onProcessTitleChanged.fire(this._currentTitle);
 	}
 
@@ -176,7 +180,7 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 	}
 
 	public input(data: string): void {
-		if (this._isDisposed) {
+		if (this._isDisposed || !this._ptyProcess) {
 			return;
 		}
 		this._ptyProcess.write(data);
@@ -188,7 +192,9 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 		}
 		// Ensure that cols and rows are always >= 1, this prevents a native
 		// exception in winpty.
-		this._ptyProcess.resize(Math.max(cols, 1), Math.max(rows, 1));
+		if (this._ptyProcess) {
+			this._ptyProcess.resize(Math.max(cols, 1), Math.max(rows, 1));
+		}
 	}
 
 	public getInitialCwd(): Promise<string> {
@@ -198,6 +204,10 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 	public getCwd(): Promise<string> {
 		if (platform.isMacintosh) {
 			return new Promise<string>(resolve => {
+				if (!this._ptyProcess) {
+					resolve(this._initialCwd);
+					return;
+				}
 				exec('lsof -p ' + this._ptyProcess.pid + ' | grep cwd', (error, stdout, stderr) => {
 					if (stdout !== '') {
 						resolve(stdout.substring(stdout.indexOf('/'), stdout.length - 1));
@@ -208,6 +218,10 @@ export class TerminalProcess implements ITerminalChildProcess, IDisposable {
 
 		if (platform.isLinux) {
 			return new Promise<string>(resolve => {
+				if (!this._ptyProcess) {
+					resolve(this._initialCwd);
+					return;
+				}
 				fs.readlink('/proc/' + this._ptyProcess.pid + '/cwd', (err, linkedstr) => {
 					if (err) {
 						resolve(this._initialCwd);