From 4f9cd2770a5f04b4e83157c71a3a9f1bf5b42d84 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Fri, 24 May 2019 11:00:45 -0400 Subject: [PATCH] child_process: simplify spawn argument parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit simplifies the object returned by normalizeSpawnArguments(). This does impact monkey patching, as illustrated by the changes in tests. PR-URL: https://github.com/nodejs/node/pull/27854 Reviewed-By: Ruben Bridgewater Reviewed-By: Michaël Zasso Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott --- lib/child_process.js | 66 +++++++------------ lib/internal/child_process.js | 9 ++- ...est-child-process-spawnsync-kill-signal.js | 6 +- .../test-child-process-spawnsync-shell.js | 12 ++-- .../test-child-process-windows-hide.js | 2 +- 5 files changed, 38 insertions(+), 57 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 0fd372e3112736..d5037d96fc78a9 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -460,16 +460,14 @@ function normalizeSpawnArguments(file, args, options) { } // Validate windowsVerbatimArguments, if present. - if (options.windowsVerbatimArguments != null && - typeof options.windowsVerbatimArguments !== 'boolean') { + let { windowsVerbatimArguments } = options; + if (windowsVerbatimArguments != null && + typeof windowsVerbatimArguments !== 'boolean') { throw new ERR_INVALID_ARG_TYPE('options.windowsVerbatimArguments', 'boolean', - options.windowsVerbatimArguments); + windowsVerbatimArguments); } - // Make a shallow copy so we don't clobber the user's options object. - options = { ...options }; - if (options.shell) { const command = [file].concat(args).join(' '); // Set the shell, switches, and commands. @@ -481,7 +479,7 @@ 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; + windowsVerbatimArguments = true; } else { args = ['-c', command]; } @@ -521,47 +519,35 @@ function normalizeSpawnArguments(file, args, options) { } return { - file: file, - args: args, - options: options, - envPairs: envPairs + // Make a shallow copy so we don't clobber the user's options object. + ...options, + args, + detached: !!options.detached, + envPairs, + file, + windowsHide: !!options.windowsHide, + windowsVerbatimArguments: !!windowsVerbatimArguments }; } var spawn = exports.spawn = function spawn(file, args, options) { - const opts = normalizeSpawnArguments(file, args, options); const child = new ChildProcess(); - options = opts.options; - debug('spawn', opts.args, options); - - child.spawn({ - file: opts.file, - args: opts.args, - cwd: options.cwd, - windowsHide: !!options.windowsHide, - windowsVerbatimArguments: !!options.windowsVerbatimArguments, - detached: !!options.detached, - envPairs: opts.envPairs, - stdio: options.stdio, - uid: options.uid, - gid: options.gid - }); + options = normalizeSpawnArguments(file, args, options); + debug('spawn', options); + child.spawn(options); return child; }; function spawnSync(file, args, options) { - const opts = normalizeSpawnArguments(file, args, options); - - const defaults = { + options = { maxBuffer: MAX_BUFFER, - ...opts.options + ...normalizeSpawnArguments(file, args, options) }; - options = opts.options = defaults; - debug('spawnSync', opts.args, options); + debug('spawnSync', options); // Validate the timeout, if present. validateTimeout(options.timeout); @@ -569,10 +555,6 @@ function spawnSync(file, args, options) { // Validate maxBuffer, if present. validateMaxBuffer(options.maxBuffer); - options.file = opts.file; - options.args = opts.args; - options.envPairs = opts.envPairs; - // Validate and translate the kill signal, if present. options.killSignal = sanitizeKillSignal(options.killSignal); @@ -603,7 +585,7 @@ function spawnSync(file, args, options) { } } - return child_process.spawnSync(opts); + return child_process.spawnSync(options); } exports.spawnSync = spawnSync; @@ -628,15 +610,15 @@ function checkExecSyncError(ret, args, cmd) { function execFileSync(command, args, options) { - const opts = normalizeSpawnArguments(command, args, options); - const inheritStderr = !opts.options.stdio; + options = normalizeSpawnArguments(command, args, options); - const ret = spawnSync(opts.file, opts.args.slice(1), opts.options); + const inheritStderr = !options.stdio; + const ret = spawnSync(options.file, options.args.slice(1), options); if (inheritStderr && ret.stderr) process.stderr.write(ret.stderr); - const err = checkExecSyncError(ret, opts.args, undefined); + const err = checkExecSyncError(ret, options.args, undefined); if (err) throw err; diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index a0f083d8deb58e..71eeed2994fbde 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -1022,8 +1022,7 @@ function maybeClose(subprocess) { } } -function spawnSync(opts) { - const options = opts.options; +function spawnSync(options) { const result = spawn_sync.spawn(options); if (result.output && options.encoding && options.encoding !== 'buffer') { @@ -1038,9 +1037,9 @@ function spawnSync(opts) { result.stderr = result.output && result.output[2]; if (result.error) { - result.error = errnoException(result.error, 'spawnSync ' + opts.file); - result.error.path = opts.file; - result.error.spawnargs = opts.args.slice(1); + result.error = errnoException(result.error, 'spawnSync ' + options.file); + result.error.path = options.file; + result.error.spawnargs = options.args.slice(1); } return result; diff --git a/test/parallel/test-child-process-spawnsync-kill-signal.js b/test/parallel/test-child-process-spawnsync-kill-signal.js index b014a384f65f37..f2decf01b357c9 100644 --- a/test/parallel/test-child-process-spawnsync-kill-signal.js +++ b/test/parallel/test-child-process-spawnsync-kill-signal.js @@ -36,7 +36,7 @@ if (process.argv[2] === 'child') { // Verify that the default kill signal is SIGTERM. { const child = spawn(undefined, (opts) => { - assert.strictEqual(opts.options.killSignal, undefined); + assert.strictEqual(opts.killSignal, undefined); }); assert.strictEqual(child.signal, 'SIGTERM'); @@ -45,7 +45,7 @@ if (process.argv[2] === 'child') { // Verify that a string signal name is handled properly. { const child = spawn('SIGKILL', (opts) => { - assert.strictEqual(opts.options.killSignal, SIGKILL); + assert.strictEqual(opts.killSignal, SIGKILL); }); assert.strictEqual(child.signal, 'SIGKILL'); @@ -56,7 +56,7 @@ if (process.argv[2] === 'child') { assert.strictEqual(typeof SIGKILL, 'number'); const child = spawn(SIGKILL, (opts) => { - assert.strictEqual(opts.options.killSignal, SIGKILL); + assert.strictEqual(opts.killSignal, SIGKILL); }); assert.strictEqual(child.signal, 'SIGKILL'); diff --git a/test/parallel/test-child-process-spawnsync-shell.js b/test/parallel/test-child-process-spawnsync-shell.js index 01c5aef9e8d919..c70949729e78ed 100644 --- a/test/parallel/test-child-process-spawnsync-shell.js +++ b/test/parallel/test-child-process-spawnsync-shell.js @@ -64,12 +64,12 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz'); assert.strictEqual(opts.file, shellOutput); assert.deepStrictEqual(opts.args, [shellOutput, ...shellFlags, outputCmd]); - assert.strictEqual(opts.options.shell, shell); - assert.strictEqual(opts.options.file, opts.file); - assert.deepStrictEqual(opts.options.args, opts.args); - assert.strictEqual(opts.options.windowsHide, undefined); - assert.strictEqual(opts.options.windowsVerbatimArguments, - windowsVerbatim); + assert.strictEqual(opts.shell, shell); + assert.strictEqual(opts.file, opts.file); + assert.deepStrictEqual(opts.args, opts.args); + assert.strictEqual(opts.windowsHide, false); + assert.strictEqual(opts.windowsVerbatimArguments, + !!windowsVerbatim); }); cp.spawnSync(cmd, { shell }); internalCp.spawnSync = oldSpawnSync; diff --git a/test/parallel/test-child-process-windows-hide.js b/test/parallel/test-child-process-windows-hide.js index 67ed18d7338d83..42bca6471ccdbc 100644 --- a/test/parallel/test-child-process-windows-hide.js +++ b/test/parallel/test-child-process-windows-hide.js @@ -19,7 +19,7 @@ internalCp.ChildProcess.prototype.spawn = common.mustCall(function(options) { }, 2); internalCp.spawnSync = common.mustCall(function(options) { - assert.strictEqual(options.options.windowsHide, true); + assert.strictEqual(options.windowsHide, true); return originalSpawnSync.apply(this, arguments); });