child_process: simplify spawn argument parsing

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 <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>

5 files changed, 38 insertions, 57 deletions

diff --git a/lib/child_process.js b/lib/child_process.js
index 0fd372e311..d5037d96fc 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 a0f083d8de..71eeed2994 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 b014a384f6..f2decf01b3 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 01c5aef9e8..c70949729e 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 67ed18d733..42bca6471c 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);
 });