Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions pkgs/hooks_runner/lib/src/utils/run_process.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@

import 'dart:async';
import 'dart:developer';
import 'dart:io' show Process, ProcessException, ProcessResult, systemEncoding;
import 'dart:io'
show Platform, Process, ProcessException, ProcessResult, systemEncoding;

import 'package:file/file.dart';
import 'package:logging/logging.dart';
Expand Down Expand Up @@ -58,11 +59,21 @@ Future<RunProcessResult> runProcess({
workingDirectory: workingDirectory?.toFilePath(),
environment: environment,
includeParentEnvironment: includeParentEnvironment,
// Never run through a shell. On Windows, running an executable through
// `cmd.exe /c` mangles the command line when more than one argument is
// quoted (cmd strips the outer quotes when the line contains more than
// two quote characters), which breaks any invocation whose executable
// and arguments contain spaces.
// On Windows, only launch through `cmd.exe` when strictly necessary.
//
// `CreateProcess` (used when `runInShell` is `false`) can directly launch
// `.exe`/`.com` executables and applies the correct command-line quoting,
// so we avoid the shell for those: running them through `cmd.exe /c`
// mangles the command line when more than one argument is quoted (cmd
// strips the outer quotes when the line contains more than two quote
// characters), which breaks any invocation whose executable and arguments
// contain spaces.
//
// However, `CreateProcess` cannot resolve a bare command name to a
// `.bat`/`.cmd` shim via `PATHEXT`, nor execute such a shim directly (for
// example the executables generated by `dart install`). Those must go
// through `cmd.exe`, so we opt into the shell for them.
runInShell: Platform.isWindows && _needsShellOnWindows(executable),
);

final stdoutSub = process.stdout.listen((List<int> data) {
Expand Down Expand Up @@ -117,6 +128,24 @@ Future<RunProcessResult> runProcess({
}
}

/// Whether [executable] must be launched through `cmd.exe` on Windows.
///
/// `CreateProcess` can only directly launch `.exe`/`.com` binaries. Anything
/// else (a `.bat`/`.cmd` shim, or a bare command name that resolves to one via
/// `PATHEXT`) has to be run through the shell.
bool _needsShellOnWindows(Uri executable) {
final path = executable.toFilePath();
final lastSeparator = path.lastIndexOf(RegExp(r'[\\/]'));
final fileName = lastSeparator == -1
? path
: path.substring(lastSeparator + 1);
final dot = fileName.lastIndexOf('.');
// No extension (e.g. a bare command name resolved via `PATHEXT`).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

riiight, pathext

if (dot <= 0) return true;
final extension = fileName.substring(dot).toLowerCase();
return extension != '.exe' && extension != '.com';
}

/// Drop in replacement of [ProcessResult].
class RunProcessResult {
final int pid;
Expand Down
60 changes: 54 additions & 6 deletions pkgs/hooks_runner/test/utils/run_process_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
// contain a space (e.g. the default pub cache under a Windows user name with
// a space, `C:\Users\First Last\AppData\Local\Pub\Cache\...`).
//
// On Windows, `runProcess` runs through `cmd.exe` (`runInShell`) whenever a
// `workingDirectory` is passed. `cmd.exe`'s `/c` quote-stripping rule mangles
// the command line as soon as more than one token needs quoting because it
// contains a space (the executable path and an argument, or two arguments),
// causing errors like
// `'C:\Program' is not recognized as an internal or external command`.
// On Windows, `runProcess` only runs through `cmd.exe` (`runInShell`) when
// strictly necessary: bare command names (resolved via `PATHEXT`) and
// `.bat`/`.cmd` shims. For `.exe`/`.com` binaries it uses `CreateProcess`
// directly so command lines with multiple quoted tokens are not mangled by
// `cmd.exe`'s `/c` quote-stripping rule.

import 'dart:io';

Expand Down Expand Up @@ -78,6 +77,55 @@ void main(List<String> args) {
expect(result.stdout, contains('ARGV:first arg|second arg'));
});

test(
'runProcess runs bare .bat shim from working directory on Windows',
() async {
if (!Platform.isWindows) return;

final binDir = await tempDirForTest();
final batUri = binDir.resolve('test shim.bat');
await File.fromUri(batUri).writeAsString(
'@echo off\r\necho SHIM_OK %*\r\n',
);

final result = await runProcess(
executable: Uri.parse('test shim'),
arguments: ['--help'],
workingDirectory: binDir,
logger: logger,
);

expect(result.exitCode, 0);
expect(result.stdout, contains('SHIM_OK'));
expect(result.stdout, contains('--help'));
},
);

test(
'runProcess runs bare .bat shim from PATH on Windows',
() async {
if (!Platform.isWindows) return;

final binDir = await tempDirForTest();
final batUri = binDir.resolve('test shim.bat');
await File.fromUri(batUri).writeAsString(
'@echo off\r\necho SHIM_OK %*\r\n',
);

final originalPath = Platform.environment['PATH'] ?? '';
final result = await runProcess(
executable: Uri.parse('test shim'),
arguments: ['--help'],
environment: {'PATH': '${binDir.toFilePath()};$originalPath'},
logger: logger,
);

expect(result.exitCode, 0);
expect(result.stdout, contains('SHIM_OK'));
expect(result.stdout, contains('--help'));
},
);

test('runProcess handles arguments containing a space and quotes', () async {
final workingDir = await tempDirForTest();
final result = await runProcess(
Expand Down
Loading