From b42be2815c1416567e9017576326ec32da77fd51 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Sat, 21 Jul 2018 21:24:08 +0900 Subject: [PATCH 01/16] child-process: fix switches for alternative shells on Windows 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: https://github.com/nodejs/node/issues/21905 --- lib/child_process.js | 51 ++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 2e75ace1ba0c84..2ab491539bfd19 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -146,8 +146,8 @@ function normalizeExecArgs(command, options, callback) { exports.exec = function exec(/* command , options, callback */) { const opts = normalizeExecArgs.apply(null, arguments); return exports.execFile(opts.file, - opts.options, - opts.callback); + opts.options, + opts.callback); }; const customPromiseExecFunction = (orig) => { @@ -383,7 +383,7 @@ const _deprecatedCustomFds = deprecate( return fd === -1 ? 'pipe' : fd; }); }, 'child_process: options.customFds option is deprecated. ' + - 'Use options.stdio instead.', 'DEP0006'); + 'Use options.stdio instead.', 'DEP0006'); function _convertCustomFds(options) { if (options.customFds && !options.stdio) { @@ -400,7 +400,7 @@ function normalizeSpawnArguments(file, args, options) { if (Array.isArray(args)) { args = args.slice(0); } else if (args !== undefined && - (args === null || typeof args !== 'object')) { + (args === null || typeof args !== 'object')) { throw new ERR_INVALID_ARG_TYPE('args', 'object', args); } else { options = args; @@ -414,15 +414,15 @@ function normalizeSpawnArguments(file, args, options) { // Validate the cwd, if present. if (options.cwd != null && - typeof options.cwd !== 'string') { + typeof options.cwd !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.cwd', 'string', options.cwd); } // Validate detached, if present. if (options.detached != null && - typeof options.detached !== 'boolean') { + typeof options.detached !== 'boolean') { throw new ERR_INVALID_ARG_TYPE('options.detached', - 'boolean', options.detached); + 'boolean', options.detached); } // Validate the uid, if present. @@ -437,31 +437,31 @@ function normalizeSpawnArguments(file, args, options) { // Validate the shell, if present. if (options.shell != null && - typeof options.shell !== 'boolean' && - typeof options.shell !== 'string') { + typeof options.shell !== 'boolean' && + typeof options.shell !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.shell', - ['boolean', 'string'], options.shell); + ['boolean', 'string'], options.shell); } // Validate argv0, if present. if (options.argv0 != null && - typeof options.argv0 !== 'string') { + typeof options.argv0 !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.argv0', 'string', options.argv0); } // Validate windowsHide, if present. if (options.windowsHide != null && - typeof options.windowsHide !== 'boolean') { + typeof options.windowsHide !== 'boolean') { throw new ERR_INVALID_ARG_TYPE('options.windowsHide', - 'boolean', options.windowsHide); + 'boolean', options.windowsHide); } // Validate windowsVerbatimArguments, if present. if (options.windowsVerbatimArguments != null && - typeof options.windowsVerbatimArguments !== 'boolean') { + typeof options.windowsVerbatimArguments !== 'boolean') { throw new ERR_INVALID_ARG_TYPE('options.windowsVerbatimArguments', - 'boolean', - options.windowsVerbatimArguments); + 'boolean', + options.windowsVerbatimArguments); } // Make a shallow copy so we don't clobber the user's options object. @@ -469,13 +469,18 @@ 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}"`]; + // "/d /s /c" is used only for cmd.exe. + if (file.endsWith("cmd.exe") || file.endsWith("cmd")) { + args = ['/d', '/s', '/c', `"${command}"`]; + } else { + args = ['-c', `"${command}"`]; + } options.windowsVerbatimArguments = true; } else { if (typeof options.shell === 'string') @@ -583,7 +588,7 @@ function spawnSync(/* file, args, options */) { 'TypedArray', 'DataView', 'string'], - input); + input); } } } @@ -660,8 +665,8 @@ function validateTimeout(timeout) { function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { throw new ERR_OUT_OF_RANGE('options.maxBuffer', - 'a positive number', - maxBuffer); + 'a positive number', + maxBuffer); } } @@ -671,7 +676,7 @@ function sanitizeKillSignal(killSignal) { return convertToValidSignal(killSignal); } else if (killSignal != null) { throw new ERR_INVALID_ARG_TYPE('options.killSignal', - ['string', 'number'], - killSignal); + ['string', 'number'], + killSignal); } } From e9863e66f643ccd8f478f423830991608f19b230 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Sun, 22 Jul 2018 00:22:27 +0900 Subject: [PATCH 02/16] child-process: fix lint errors Modify indent, and replace doublequote to singlequote. --- lib/child_process.js | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 2ab491539bfd19..a8fe0d21f2359a 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -145,7 +145,8 @@ function normalizeExecArgs(command, options, callback) { exports.exec = function exec(/* command , options, callback */) { const opts = normalizeExecArgs.apply(null, arguments); - return exports.execFile(opts.file, + return exports.execFile( + opts.file, opts.options, opts.callback); }; @@ -421,7 +422,8 @@ function normalizeSpawnArguments(file, args, options) { // Validate detached, if present. if (options.detached != null && typeof options.detached !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE('options.detached', + throw new ERR_INVALID_ARG_TYPE( + 'options.detached', 'boolean', options.detached); } @@ -439,7 +441,8 @@ function normalizeSpawnArguments(file, args, options) { if (options.shell != null && typeof options.shell !== 'boolean' && typeof options.shell !== 'string') { - throw new ERR_INVALID_ARG_TYPE('options.shell', + throw new ERR_INVALID_ARG_TYPE( + 'options.shell', ['boolean', 'string'], options.shell); } @@ -452,14 +455,17 @@ function normalizeSpawnArguments(file, args, options) { // Validate windowsHide, if present. if (options.windowsHide != null && typeof options.windowsHide !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE('options.windowsHide', - 'boolean', options.windowsHide); + throw new ERR_INVALID_ARG_TYPE( + 'options.windowsHide', + 'boolean', + options.windowsHide); } // Validate windowsVerbatimArguments, if present. if (options.windowsVerbatimArguments != null && typeof options.windowsVerbatimArguments !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE('options.windowsVerbatimArguments', + throw new ERR_INVALID_ARG_TYPE( + 'options.windowsVerbatimArguments', 'boolean', options.windowsVerbatimArguments); } @@ -476,7 +482,7 @@ function normalizeSpawnArguments(file, args, options) { else file = process.env.comspec || 'cmd.exe'; // "/d /s /c" is used only for cmd.exe. - if (file.endsWith("cmd.exe") || file.endsWith("cmd")) { + if (file.endsWith('cmd.exe') || file.endsWith('cmd')) { args = ['/d', '/s', '/c', `"${command}"`]; } else { args = ['-c', `"${command}"`]; @@ -583,7 +589,7 @@ function spawnSync(/* file, args, options */) { } else if (typeof input === 'string') { pipe.input = Buffer.from(input, options.encoding); } else { - throw new ERR_INVALID_ARG_TYPE(`options.stdio[${i}]`, + throw new ERR_INVALID_ARG_TYPE( ['Buffer', 'TypedArray', 'DataView', @@ -664,7 +670,8 @@ function validateTimeout(timeout) { function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { - throw new ERR_OUT_OF_RANGE('options.maxBuffer', + throw new ERR_OUT_OF_RANGE( + 'options.maxBuffer', 'a positive number', maxBuffer); } @@ -675,7 +682,8 @@ function sanitizeKillSignal(killSignal) { if (typeof killSignal === 'string' || typeof killSignal === 'number') { return convertToValidSignal(killSignal); } else if (killSignal != null) { - throw new ERR_INVALID_ARG_TYPE('options.killSignal', + throw new ERR_INVALID_ARG_TYPE( + 'options.killSignal', ['string', 'number'], killSignal); } From 6f3bccccfb807661d606d70101e5fd12a1d2508d Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Sun, 22 Jul 2018 01:39:05 +0900 Subject: [PATCH 03/16] test: add exec-any-shells test This test ensure that child_process.exec can work with any shells. Giving a shell name if $path is defined, or full path is given. Testing with cmd, powershell, and git-bash, but the test fails when testing with git-bash. Git-bash is usually placed at 'C:\Program Files\Git\bin\', and it has a space that cause the failure. --- ...t-child-process-exec-any-shells-windows.js | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 test/parallel/test-child-process-exec-any-shells-windows.js diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js new file mode 100644 index 00000000000000..d597ee4a02b5e3 --- /dev/null +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); + +// 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. + +const test = (shell) => { + cp.exec('echo foo bar', { shell: shell }, (error, stdout, stderror) => { + assert.ok(!error && !stderror); + assert.ok(stdout.startsWith('foo bar')); + }); +}; + +test('cmd'); +test('cmd.exe'); +test('C:\\WINDOWS\\system32\\cmd.exe'); +test('powershell'); +test('C:\\Program Files\\Git\\bin\\bash.exe'); From 6b9b090d6914b9555ce2f3e22b0a62d8c8aa7c06 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Mon, 23 Jul 2018 18:29:48 +0900 Subject: [PATCH 04/16] test: fix exec-any-shell test for powershell When giving 'echo foo bar' to powershell, the result contains a new line like: foo bar This commit enables to test with poweshell correctly. --- test/parallel/test-child-process-exec-any-shells-windows.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index d597ee4a02b5e3..42407be2afcf8c 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -12,7 +12,7 @@ if (!common.isWindows) const test = (shell) => { cp.exec('echo foo bar', { shell: shell }, (error, stdout, stderror) => { assert.ok(!error && !stderror); - assert.ok(stdout.startsWith('foo bar')); + assert.ok(stdout.includes('foo') && stdout.includes('bar')); }); }; From 1dcea345314171c24f68167d2645661af0324862 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Mon, 23 Jul 2018 18:37:34 +0900 Subject: [PATCH 05/16] child-process: enable to accept full path that contains spaces Before this commit, if given path contained spaces, like 'Program Files', exec failed to find a shell. 'options.windowsVerbatimArguments' is used for cmd.exe only, and spaces are escaped correctly. --- lib/child_process.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index a8fe0d21f2359a..9d3be69b84db6a 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -481,13 +481,13 @@ function normalizeSpawnArguments(file, args, options) { file = options.shell; else file = process.env.comspec || 'cmd.exe'; - // "/d /s /c" is used only for cmd.exe. + // '/d /s /c' is used only for cmd.exe. if (file.endsWith('cmd.exe') || file.endsWith('cmd')) { args = ['/d', '/s', '/c', `"${command}"`]; + options.windowsVerbatimArguments = true; } else { - args = ['-c', `"${command}"`]; + args = ['-c', command]; } - options.windowsVerbatimArguments = true; } else { if (typeof options.shell === 'string') file = options.shell; From c7163f872bf1753c34c24680a88337b272dbc9c5 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Tue, 24 Jul 2018 16:53:23 +0900 Subject: [PATCH 06/16] child-process: undo unrelated style changes Auto-formatting cause unnecessary indentations. These style changes are removed. --- lib/child_process.js | 64 +++++++++++++++++++------------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 9d3be69b84db6a..78d6d6e6966f9f 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -145,10 +145,9 @@ function normalizeExecArgs(command, options, callback) { exports.exec = function exec(/* command , options, callback */) { const opts = normalizeExecArgs.apply(null, arguments); - return exports.execFile( - opts.file, - opts.options, - opts.callback); + return exports.execFile(opts.file, + opts.options, + opts.callback); }; const customPromiseExecFunction = (orig) => { @@ -384,7 +383,7 @@ const _deprecatedCustomFds = deprecate( return fd === -1 ? 'pipe' : fd; }); }, 'child_process: options.customFds option is deprecated. ' + - 'Use options.stdio instead.', 'DEP0006'); + 'Use options.stdio instead.', 'DEP0006'); function _convertCustomFds(options) { if (options.customFds && !options.stdio) { @@ -401,7 +400,7 @@ function normalizeSpawnArguments(file, args, options) { if (Array.isArray(args)) { args = args.slice(0); } else if (args !== undefined && - (args === null || typeof args !== 'object')) { + (args === null || typeof args !== 'object')) { throw new ERR_INVALID_ARG_TYPE('args', 'object', args); } else { options = args; @@ -415,16 +414,15 @@ function normalizeSpawnArguments(file, args, options) { // Validate the cwd, if present. if (options.cwd != null && - typeof options.cwd !== 'string') { + typeof options.cwd !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.cwd', 'string', options.cwd); } // Validate detached, if present. if (options.detached != null && - typeof options.detached !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE( - 'options.detached', - 'boolean', options.detached); + typeof options.detached !== 'boolean') { + throw new ERR_INVALID_ARG_TYPE('options.detached', + 'boolean', options.detached); } // Validate the uid, if present. @@ -439,35 +437,31 @@ function normalizeSpawnArguments(file, args, options) { // Validate the shell, if present. if (options.shell != null && - typeof options.shell !== 'boolean' && - typeof options.shell !== 'string') { - throw new ERR_INVALID_ARG_TYPE( - 'options.shell', - ['boolean', 'string'], options.shell); + typeof options.shell !== 'boolean' && + typeof options.shell !== 'string') { + throw new ERR_INVALID_ARG_TYPE('options.shell', + ['boolean', 'string'], options.shell); } // Validate argv0, if present. if (options.argv0 != null && - typeof options.argv0 !== 'string') { + typeof options.argv0 !== 'string') { throw new ERR_INVALID_ARG_TYPE('options.argv0', 'string', options.argv0); } // Validate windowsHide, if present. if (options.windowsHide != null && typeof options.windowsHide !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE( - 'options.windowsHide', - 'boolean', - options.windowsHide); + throw new ERR_INVALID_ARG_TYPE('options.windowsHide', + 'boolean', options.windowsHide); } // Validate windowsVerbatimArguments, if present. if (options.windowsVerbatimArguments != null && - typeof options.windowsVerbatimArguments !== 'boolean') { - throw new ERR_INVALID_ARG_TYPE( - 'options.windowsVerbatimArguments', - 'boolean', - options.windowsVerbatimArguments); + typeof options.windowsVerbatimArguments !== 'boolean') { + throw new ERR_INVALID_ARG_TYPE('options.windowsVerbatimArguments', + 'boolean', + options.windowsVerbatimArguments); } // Make a shallow copy so we don't clobber the user's options object. @@ -589,12 +583,12 @@ function spawnSync(/* file, args, options */) { } else if (typeof input === 'string') { pipe.input = Buffer.from(input, options.encoding); } else { - throw new ERR_INVALID_ARG_TYPE( + throw new ERR_INVALID_ARG_TYPE(`options.stdio[${i}]`, ['Buffer', 'TypedArray', 'DataView', 'string'], - input); + input); } } } @@ -670,10 +664,9 @@ function validateTimeout(timeout) { function validateMaxBuffer(maxBuffer) { if (maxBuffer != null && !(typeof maxBuffer === 'number' && maxBuffer >= 0)) { - throw new ERR_OUT_OF_RANGE( - 'options.maxBuffer', - 'a positive number', - maxBuffer); + throw new ERR_OUT_OF_RANGE('options.maxBuffer', + 'a positive number', + maxBuffer); } } @@ -682,9 +675,8 @@ function sanitizeKillSignal(killSignal) { if (typeof killSignal === 'string' || typeof killSignal === 'number') { return convertToValidSignal(killSignal); } else if (killSignal != null) { - throw new ERR_INVALID_ARG_TYPE( - 'options.killSignal', - ['string', 'number'], - killSignal); + throw new ERR_INVALID_ARG_TYPE('options.killSignal', + ['string', 'number'], + killSignal); } } From a31434084be7037e1143d61f82686b4dcd801faf Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 26 Jul 2018 16:48:11 +0900 Subject: [PATCH 07/16] test: remove hard coding path test-child-process-any-shell uses `where` before testing with bash to remove hard coding path. --- .../test-child-process-exec-any-shells-windows.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index 42407be2afcf8c..735f38b6f19fab 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -18,6 +18,10 @@ const test = (shell) => { test('cmd'); test('cmd.exe'); -test('C:\\WINDOWS\\system32\\cmd.exe'); test('powershell'); -test('C:\\Program Files\\Git\\bin\\bash.exe'); +cp.exec('where bash', (error, stdout) => { + if (error) { + return; + } + test(stdout.trim()); +}); From ce7999ac074d33aabbe988fbd74405438efa2819 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 26 Jul 2018 22:07:07 +0900 Subject: [PATCH 08/16] test: fix spawnsync-shell to check correct options on Windows This test expected to use cmd.exe options even if the shell was powershell. This commit fixes to check correct options for shells other than cmd.exe. --- lib/child_process.js | 2 +- test/parallel/test-child-process-spawnsync-shell.js | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 78d6d6e6966f9f..7ee4918b76f4de 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -451,7 +451,7 @@ function normalizeSpawnArguments(file, args, options) { // Validate windowsHide, if present. if (options.windowsHide != null && - typeof options.windowsHide !== 'boolean') { + typeof options.windowsHide !== 'boolean') { throw new ERR_INVALID_ARG_TYPE('options.windowsHide', 'boolean', options.windowsHide); } diff --git a/test/parallel/test-child-process-spawnsync-shell.js b/test/parallel/test-child-process-spawnsync-shell.js index 637cb12a2332bd..a4118168183250 100644 --- a/test/parallel/test-child-process-spawnsync-shell.js +++ b/test/parallel/test-child-process-spawnsync-shell.js @@ -54,11 +54,13 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz'); function test(testPlatform, shell, shellOutput) { platform = testPlatform; - + const isCmd = shellOutput.endsWith('cmd.exe') || + shellOutput.endsWith('cmd'); 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, From 097c90503631f28442eb1bbd93d11bd9a9ab2ec8 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 2 Aug 2018 14:34:42 +0900 Subject: [PATCH 09/16] child-process: set windowsVerbatimArguments true for any shell --- lib/child_process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index 7ee4918b76f4de..71d8d7c565a823 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -478,10 +478,10 @@ function normalizeSpawnArguments(file, args, options) { // '/d /s /c' is used only for cmd.exe. if (file.endsWith('cmd.exe') || file.endsWith('cmd')) { args = ['/d', '/s', '/c', `"${command}"`]; - options.windowsVerbatimArguments = true; } else { args = ['-c', command]; } + options.windowsVerbatimArguments = true; } else { if (typeof options.shell === 'string') file = options.shell; From a4122b7f10cb0f72a0793d636a25218ab391d0f4 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 2 Aug 2018 16:05:25 +0900 Subject: [PATCH 10/16] child-process: use case-insensitive comparison to check shell name --- lib/child_process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index 71d8d7c565a823..5364ebb103999f 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -476,7 +476,7 @@ function normalizeSpawnArguments(file, args, options) { else file = process.env.comspec || 'cmd.exe'; // '/d /s /c' is used only for cmd.exe. - if (file.endsWith('cmd.exe') || file.endsWith('cmd')) { + if (/^(?:.*\\)?cmd(?:\.exe)?$/i.test(file)) { args = ['/d', '/s', '/c', `"${command}"`]; } else { args = ['-c', command]; From 27ba48f1b50dbe3ca9733130c072d72cfde2d582 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 2 Aug 2018 17:59:01 +0900 Subject: [PATCH 11/16] test: add testcases to ensure uppercase shell name and complex commands --- .../test-child-process-exec-any-shells-windows.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index 735f38b6f19fab..4bfc8f886b872a 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -18,10 +18,19 @@ const test = (shell) => { test('cmd'); test('cmd.exe'); +test('CMD'); test('powershell'); + cp.exec('where bash', (error, stdout) => { if (error) { return; } test(stdout.trim()); }); + +cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`, + { shell: 'PowerShell' }, (error, stdout, stderror) => { + assert.ok(!error && !stderror); + assert.ok(stdout.includes( + 'test-child-process-exec-any-shells-windows.js')); + }); From f93c7961e9ebc86ac40fd0270d3b44058c1610d5 Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Thu, 2 Aug 2018 23:39:47 +0900 Subject: [PATCH 12/16] test: fix to accept multiple lines output of `where bash` --- test/parallel/test-child-process-exec-any-shells-windows.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index 4bfc8f886b872a..87bec7d5ea87d2 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -25,7 +25,7 @@ cp.exec('where bash', (error, stdout) => { if (error) { return; } - test(stdout.trim()); + test(stdout.split(/[\r\n]+/g)[0].trim()); }); cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`, From ade01d90ece33bab3b485924e984e6732175ed6b Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Fri, 3 Aug 2018 01:49:58 +0900 Subject: [PATCH 13/16] test: revert windowsVerbatim flag, fix `isCmd` check in spawnsync-shell --- test/parallel/test-child-process-spawnsync-shell.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-child-process-spawnsync-shell.js b/test/parallel/test-child-process-spawnsync-shell.js index a4118168183250..b47b8cfeac5d1c 100644 --- a/test/parallel/test-child-process-spawnsync-shell.js +++ b/test/parallel/test-child-process-spawnsync-shell.js @@ -54,13 +54,12 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz'); function test(testPlatform, shell, shellOutput) { platform = testPlatform; - const isCmd = shellOutput.endsWith('cmd.exe') || - shellOutput.endsWith('cmd'); + const isCmd = /^(?:.*\\)?cmd(?:\.exe)?$/i.test(shellOutput); const cmd = 'not_a_real_command'; const shellFlags = isCmd ? ['/d', '/s', '/c'] : ['-c']; const outputCmd = isCmd ? `"${cmd}"` : cmd; - const windowsVerbatim = isCmd ? true : undefined; + const windowsVerbatim = platform === 'win32' ? true : undefined; internalCp.spawnSync = common.mustCall(function(opts) { assert.strictEqual(opts.file, shellOutput); assert.deepStrictEqual(opts.args, From 5a2c13091d03128f6f80f416bb799ef9f7051d3e Mon Sep 17 00:00:00 2001 From: Tessei Kameyama Date: Tue, 7 Aug 2018 17:06:02 +0900 Subject: [PATCH 14/16] doc: update shell requirements in child_process --- doc/api/child_process.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 5b2b16f253f7f8..072dbfcba623b6 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -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 From e2192a324fc178d803c112ce52b5265df8c030ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Tue, 14 Aug 2018 13:59:13 +0100 Subject: [PATCH 15/16] fixup: windowsVerbatimArguments --- doc/api/child_process.md | 4 +- lib/child_process.js | 2 +- ...t-child-process-exec-any-shells-windows.js | 56 ++++++++++++++----- .../test-child-process-spawnsync-shell.js | 2 +- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index 072dbfcba623b6..9cee76c02ea0f9 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -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} @@ -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} diff --git a/lib/child_process.js b/lib/child_process.js index 5364ebb103999f..76fdc980249478 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -478,10 +478,10 @@ function normalizeSpawnArguments(file, args, options) { // '/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]; } - options.windowsVerbatimArguments = true; } else { if (typeof options.shell === 'string') file = options.shell; diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index 87bec7d5ea87d2..f575d918e92d95 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -2,6 +2,8 @@ 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) @@ -9,28 +11,54 @@ if (!common.isWindows) // 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 }, (error, stdout, stderror) => { - assert.ok(!error && !stderror); - assert.ok(stdout.includes('foo') && stdout.includes('bar')); - }); + 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'); - -cp.exec('where bash', (error, stdout) => { - if (error) { - return; - } - test(stdout.split(/[\r\n]+/g)[0].trim()); -}); +// Test PowerShell +test('powershell'); +testCopy('powershell.exe', + `${system32}\\WindowsPowerShell\\v1.0\\powershell.exe`); cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`, - { shell: 'PowerShell' }, (error, stdout, stderror) => { + { shell: 'PowerShell' }, common.mustCall((error, stdout, stderror) => { assert.ok(!error && !stderror); assert.ok(stdout.includes( 'test-child-process-exec-any-shells-windows.js')); - }); + })); + +// 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); + } +})); diff --git a/test/parallel/test-child-process-spawnsync-shell.js b/test/parallel/test-child-process-spawnsync-shell.js index b47b8cfeac5d1c..6f7c0ec0f14a26 100644 --- a/test/parallel/test-child-process-spawnsync-shell.js +++ b/test/parallel/test-child-process-spawnsync-shell.js @@ -59,7 +59,7 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz'); const shellFlags = isCmd ? ['/d', '/s', '/c'] : ['-c']; const outputCmd = isCmd ? `"${cmd}"` : cmd; - const windowsVerbatim = platform === 'win32' ? true : undefined; + const windowsVerbatim = isCmd ? true : undefined; internalCp.spawnSync = common.mustCall(function(opts) { assert.strictEqual(opts.file, shellOutput); assert.deepStrictEqual(opts.args, From 7a10600f990d18eef5f65a89dc7c6d908db352cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Reis?= Date: Sat, 1 Sep 2018 23:18:41 +0100 Subject: [PATCH 16/16] fixup: don't exhaust exec buffer --- ...test-child-process-exec-any-shells-windows.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/parallel/test-child-process-exec-any-shells-windows.js b/test/parallel/test-child-process-exec-any-shells-windows.js index f575d918e92d95..8cdd03d7e510d5 100644 --- a/test/parallel/test-child-process-exec-any-shells-windows.js +++ b/test/parallel/test-child-process-exec-any-shells-windows.js @@ -43,12 +43,16 @@ test('CMD'); test('powershell'); testCopy('powershell.exe', `${system32}\\WindowsPowerShell\\v1.0\\powershell.exe`); -cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`, - { shell: 'PowerShell' }, common.mustCall((error, stdout, stderror) => { - assert.ok(!error && !stderror); - assert.ok(stdout.includes( - 'test-child-process-exec-any-shells-windows.js')); - })); +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) => {