diff --git a/pkgs/hooks_runner/CHANGELOG.md b/pkgs/hooks_runner/CHANGELOG.md index 198fb8f022..3b8a8467bd 100644 --- a/pkgs/hooks_runner/CHANGELOG.md +++ b/pkgs/hooks_runner/CHANGELOG.md @@ -1,3 +1,8 @@ +## 1.6.2-wip + +- Require `dartExecutable` to be an absolute path, and on Windows to include a + file extension + ## 1.6.1 - Support versions 3.x of `package:package_config`. diff --git a/pkgs/hooks_runner/lib/src/build_runner/build_runner.dart b/pkgs/hooks_runner/lib/src/build_runner/build_runner.dart index 10eb3326d5..ddb5e4619c 100644 --- a/pkgs/hooks_runner/lib/src/build_runner/build_runner.dart +++ b/pkgs/hooks_runner/lib/src/build_runner/build_runner.dart @@ -66,6 +66,11 @@ class NativeAssetsBuildRunner { final FileSystem _fileSystemUntraced; final Logger logger; + + /// Absolute path to the Dart executable. + /// + /// On Windows, must include the file extension (for example `.exe`). + /// [Platform.resolvedExecutable] is a valid value. final Uri dartExecutable; final Duration singleHookTimeout; final Map hookEnvironment; @@ -85,6 +90,13 @@ class NativeAssetsBuildRunner { hookEnvironment = hookEnvironment ?? filteredEnvironment(includeHookEnvironmentVariable) { + if (!dartExecutable.isAbsolute) { + throw ArgumentError.value( + dartExecutable, + 'dartExecutable', + 'Must be an absolute path.', + ); + } _fileSystem = TracingFileSystem(fileSystem, _task); } diff --git a/pkgs/hooks_runner/lib/src/utils/run_process.dart b/pkgs/hooks_runner/lib/src/utils/run_process.dart index d576f86f18..c3b6ee2d7b 100644 --- a/pkgs/hooks_runner/lib/src/utils/run_process.dart +++ b/pkgs/hooks_runner/lib/src/utils/run_process.dart @@ -12,10 +12,16 @@ import 'package:logging/logging.dart'; /// Runs a [Process]. /// +/// [executable] must be an absolute path. Relative paths and `PATH` lookup are +/// not supported. On Windows, [executable] must also include a file extension +/// (for example `.exe`); `PATHEXT` lookup is not supported. +/// +/// Supports [executable] paths and [arguments] that contain spaces. Never runs +/// through a shell, so Windows `cmd.exe` quote-stripping does not apply. +/// /// If [logger] is provided, stream stdout and stderr to it. /// /// If [captureOutput], captures stdout and stderr. -// TODO(dacoharkes): Share between package:native_toolchain_c and here. Future runProcess({ required FileSystem filesystem, required Uri executable, @@ -29,6 +35,8 @@ Future runProcess({ bool throwOnUnexpectedExitCode = false, TimelineTask? task, }) async { + _validateExecutable(executable); + final printWorkingDir = workingDirectory != null && workingDirectory != filesystem.currentDirectory.uri; @@ -59,21 +67,11 @@ Future runProcess({ workingDirectory: workingDirectory?.toFilePath(), environment: environment, includeParentEnvironment: includeParentEnvironment, - // 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), + // 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. ); final stdoutSub = process.stdout.listen((List data) { @@ -128,22 +126,31 @@ Future 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`). - if (dot <= 0) return true; - final extension = fileName.substring(dot).toLowerCase(); - return extension != '.exe' && extension != '.com'; +void _validateExecutable(Uri executable) { + if (!executable.isAbsolute) { + throw ArgumentError.value( + executable, + 'executable', + 'Must be an absolute path. Relative paths and PATH lookup are not ' + 'supported.', + ); + } + // Without a shell, Windows does not apply PATHEXT, so callers must pass the + // real file name including its extension (e.g. `dart.exe`, not `dart`). + if (Platform.isWindows && !_hasFileExtension(executable.toFilePath())) { + throw ArgumentError.value( + executable, + 'executable', + 'Must include a file extension (e.g. .exe). PATHEXT lookup is not ' + 'supported.', + ); + } +} + +bool _hasFileExtension(String filePath) { + final basename = filePath.replaceAll('\\', '/').split('/').last; + final dot = basename.lastIndexOf('.'); + return dot > 0 && dot < basename.length - 1; } /// Drop in replacement of [ProcessResult]. diff --git a/pkgs/hooks_runner/pubspec.yaml b/pkgs/hooks_runner/pubspec.yaml index 137475f9ab..d228b184fb 100644 --- a/pkgs/hooks_runner/pubspec.yaml +++ b/pkgs/hooks_runner/pubspec.yaml @@ -2,7 +2,7 @@ name: hooks_runner description: >- This package is the backend that invokes build hooks. -version: 1.6.1 +version: 1.6.2-wip repository: https://github.com/dart-lang/native/tree/main/pkgs/hooks_runner diff --git a/pkgs/hooks_runner/test/build_runner/helpers.dart b/pkgs/hooks_runner/test/build_runner/helpers.dart index 85da2de58f..f1c7122c5a 100644 --- a/pkgs/hooks_runner/test/build_runner/helpers.dart +++ b/pkgs/hooks_runner/test/build_runner/helpers.dart @@ -344,11 +344,7 @@ Future expectSymbols({ }) async { if (Platform.isLinux) { final assetUri = asset.file!; - final nmResult = await runProcess( - executable: Uri(path: 'nm'), - arguments: ['-D', assetUri.toFilePath()], - logger: logger, - ); + final nmResult = await Process.run('nm', ['-D', assetUri.toFilePath()]); expect(nmResult.stdout, stringContainsInOrder(symbols)); } diff --git a/pkgs/hooks_runner/test/helpers.dart b/pkgs/hooks_runner/test/helpers.dart index 1807d81c14..f738968779 100644 --- a/pkgs/hooks_runner/test/helpers.dart +++ b/pkgs/hooks_runner/test/helpers.dart @@ -96,7 +96,8 @@ Future tempDirForTest({ /// Runs a [Process]. /// -/// If [logger] is provided, stream stdout and stderr to it. +/// See [run_process.runProcess]. [executable] must be absolute (and include a +/// file extension on Windows); relative paths and PATHEXT are not supported. /// /// If [captureOutput], captures stdout and stderr. Future runProcess({ diff --git a/pkgs/hooks_runner/test/utils/run_process_test.dart b/pkgs/hooks_runner/test/utils/run_process_test.dart index a8e3fbf5d3..1ec3a71d41 100644 --- a/pkgs/hooks_runner/test/utils/run_process_test.dart +++ b/pkgs/hooks_runner/test/utils/run_process_test.dart @@ -6,11 +6,10 @@ // 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` 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. +// `runProcess` never runs through a shell. Absolute paths (with a file +// extension on Windows) are required; relative paths and PATHEXT are not +// supported. Spaces in the executable path and arguments are handled by +// passing them as separate CreateProcess / exec arguments. import 'dart:io'; @@ -77,55 +76,6 @@ void main(List 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( @@ -139,4 +89,41 @@ void main(List args) { expect(result.stdout, contains('ARGC:2')); expect(result.stdout, contains('ARGV:fir"st arg|sec\'ond arg')); }); + + test('runProcess rejects a relative executable path', () async { + await expectLater( + runProcess( + executable: Uri(path: 'dart'), + logger: logger, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains('absolute'), + ), + ), + ); + }); + + test( + 'runProcess rejects a Windows executable without a file extension', + () async { + if (!Platform.isWindows) return; + + await expectLater( + runProcess( + executable: Uri.file(r'C:\path\to\dart'), + logger: logger, + ), + throwsA( + isA().having( + (e) => e.message, + 'message', + contains('PATHEXT'), + ), + ), + ); + }, + ); }