Skip to content

Commit

Permalink
child_process: fix switches for alternative shells on Windows
Browse files Browse the repository at this point in the history
On Windows, normalizeSpawnArguments set "/d /s /c" for any shells.
It cause exec and other methods are limited to cmd.exe as a shell.

Powershell and git-bash are often used instead of cmd.exe,
and they can recieve "-c" switch like unix shells.
So normalizeSpawnArguments is changed to set "/d /s /c" for cmd.exe,
and "-c" for others.

Fixes: #21905
PR-URL: #21943
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
  • Loading branch information
tkamenoko authored and joaocgreis committed Sep 5, 2018
1 parent 3209679 commit af883e1
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 11 deletions.
9 changes: 5 additions & 4 deletions doc/api/child_process.md
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ changes:
[Default Windows Shell][]. **Default:** `false` (no shell).
* `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
done on Windows. Ignored on Unix. This is set to `true` automatically
when `shell` is specified. **Default:** `false`.
when `shell` is specified and is CMD. **Default:** `false`.
* `windowsHide` {boolean} Hide the subprocess console window that would
normally be created on Windows systems. **Default:** `true`.
* Returns: {ChildProcess}
Expand Down Expand Up @@ -867,7 +867,7 @@ changes:
[Default Windows Shell][]. **Default:** `false` (no shell).
* `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
done on Windows. Ignored on Unix. This is set to `true` automatically
when `shell` is specified. **Default:** `false`.
when `shell` is specified and is CMD. **Default:** `false`.
* `windowsHide` {boolean} Hide the subprocess console window that would
normally be created on Windows systems. **Default:** `true`.
* Returns: {Object}
Expand Down Expand Up @@ -1432,8 +1432,9 @@ to `stdout` although there are only 4 characters.

## Shell Requirements

The shell should understand the `-c` switch on UNIX or `/d /s /c` on Windows.
On Windows, command line parsing should be compatible with `'cmd.exe'`.
The shell should understand the `-c` switch. If the shell is `'cmd.exe'`, it
should understand the `/d /s /c` switches and command line parsing should be
compatible.

## Default Windows Shell

Expand Down
11 changes: 8 additions & 3 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -469,14 +469,19 @@ function normalizeSpawnArguments(file, args, options) {

if (options.shell) {
const command = [file].concat(args).join(' ');

// Set the shell, switches, and commands.
if (process.platform === 'win32') {
if (typeof options.shell === 'string')
file = options.shell;
else
file = process.env.comspec || 'cmd.exe';
args = ['/d', '/s', '/c', `"${command}"`];
options.windowsVerbatimArguments = true;
// '/d /s /c' is used only for cmd.exe.
if (/^(?:.*\\)?cmd(?:\.exe)?$/i.test(file)) {
args = ['/d', '/s', '/c', `"${command}"`];
options.windowsVerbatimArguments = true;
} else {
args = ['-c', command];
}
} else {
if (typeof options.shell === 'string')
file = options.shell;
Expand Down
68 changes: 68 additions & 0 deletions test/parallel/test-child-process-exec-any-shells-windows.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const fs = require('fs');
const tmpdir = require('../common/tmpdir');

// This test is only relevant on Windows.
if (!common.isWindows)
common.skip('Windows specific test.');

// This test ensures that child_process.exec can work with any shells.

tmpdir.refresh();
const tmpPath = `${tmpdir.path}\\path with spaces`;
fs.mkdirSync(tmpPath);

const test = (shell) => {
cp.exec('echo foo bar', { shell: shell },
common.mustCall((error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes('foo') && stdout.includes('bar'));
}));
};
const testCopy = (shellName, shellPath) => {
// Copy the executable to a path with spaces, to ensure there are no issues
// related to quoting of argv0
const copyPath = `${tmpPath}\\${shellName}`;
fs.copyFileSync(shellPath, copyPath);
test(copyPath);
};

const system32 = `${process.env.SystemRoot}\\System32`;

// Test CMD
test(true);
test('cmd');
testCopy('cmd.exe', `${system32}\\cmd.exe`);
test('cmd.exe');
test('CMD');

// Test PowerShell
test('powershell');
testCopy('powershell.exe',
`${system32}\\WindowsPowerShell\\v1.0\\powershell.exe`);
fs.writeFile(`${tmpPath}\\test file`, 'Test', common.mustCall((err) => {
assert.ifError(err);
cp.exec(`Get-ChildItem "${tmpPath}" | Select-Object -Property Name`,
{ shell: 'PowerShell' },
common.mustCall((error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes(
'test file'));
}));
}));

// Test Bash (from WSL and Git), if available
cp.exec('where bash', common.mustCall((error, stdout) => {
if (error) {
return;
}
const lines = stdout.trim().split(/[\r\n]+/g);
for (let i = 0; i < lines.length; ++i) {
const bashPath = lines[i].trim();
test(bashPath);
testCopy(`bash_${i}.exe`, bashPath);
}
}));
9 changes: 5 additions & 4 deletions test/parallel/test-child-process-spawnsync-shell.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,12 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz');

function test(testPlatform, shell, shellOutput) {
platform = testPlatform;

const isCmd = /^(?:.*\\)?cmd(?:\.exe)?$/i.test(shellOutput);
const cmd = 'not_a_real_command';
const shellFlags = platform === 'win32' ? ['/d', '/s', '/c'] : ['-c'];
const outputCmd = platform === 'win32' ? `"${cmd}"` : cmd;
const windowsVerbatim = platform === 'win32' ? true : undefined;

const shellFlags = isCmd ? ['/d', '/s', '/c'] : ['-c'];
const outputCmd = isCmd ? `"${cmd}"` : cmd;
const windowsVerbatim = isCmd ? true : undefined;
internalCp.spawnSync = common.mustCall(function(opts) {
assert.strictEqual(opts.file, shellOutput);
assert.deepStrictEqual(opts.args,
Expand Down

0 comments on commit af883e1

Please sign in to comment.