From c030f88fcf9265bfc3ff98375bcbbbd7aa6e27d2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E6=9C=A8=E6=9D=89?= <zhmushan@qq.com>
Date: Mon, 18 Jul 2022 17:20:57 +0800
Subject: [PATCH] child_process: avoid repeated calls to
 `normalizeSpawnArguments`

PR-URL: https://github.com/nodejs/node/pull/43345
Fixes: https://github.com/nodejs/node/issues/43333
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
---
 lib/child_process.js                         | 76 +++++++++++++-------
 test/parallel/test-child-process-execfile.js | 23 +++++-
 2 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/lib/child_process.js b/lib/child_process.js
index 605bd14660f521..59259ff96a9225 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -32,6 +32,7 @@ const {
   ArrayPrototypeSort,
   ArrayPrototypeSplice,
   ArrayPrototypeUnshift,
+  ArrayPrototypePushApply,
   NumberIsInteger,
   ObjectAssign,
   ObjectDefineProperty,
@@ -249,6 +250,45 @@ ObjectDefineProperty(exec, promisify.custom, {
   value: customPromiseExecFunction(exec)
 });
 
+function normalizeExecFileArgs(file, args, options, callback) {
+  if (ArrayIsArray(args)) {
+    args = ArrayPrototypeSlice(args);
+  } else if (args != null && typeof args === 'object') {
+    callback = options;
+    options = args;
+    args = null;
+  } else if (typeof args === 'function') {
+    callback = args;
+    options = null;
+    args = null;
+  }
+
+  if (args == null) {
+    args = [];
+  }
+
+  if (typeof options === 'function') {
+    callback = options;
+  } else if (options != null) {
+    validateObject(options, 'options');
+  }
+
+  if (options == null) {
+    options = kEmptyObject;
+  }
+
+  if (callback != null) {
+    validateFunction(callback, 'callback');
+  }
+
+  // Validate argv0, if present.
+  if (options.argv0 != null) {
+    validateString(options.argv0, 'options.argv0');
+  }
+
+  return { file, args, options, callback };
+}
+
 /**
  * Spawns the specified file as a shell.
  * @param {string} file
@@ -274,27 +314,8 @@ ObjectDefineProperty(exec, promisify.custom, {
  *   ) => any} [callback]
  * @returns {ChildProcess}
  */
-function execFile(file, args = [], options, callback) {
-  if (args != null && typeof args === 'object' && !ArrayIsArray(args)) {
-    callback = options;
-    options = args;
-    args = null;
-  } else if (typeof args === 'function') {
-    callback = args;
-    options = null;
-    args = null;
-  }
-
-  if (typeof options === 'function') {
-    callback = options;
-    options = null;
-  } else if (options != null) {
-    validateObject(options, 'options');
-  }
-
-  if (callback != null) {
-    validateFunction(callback, 'callback');
-  }
+function execFile(file, args, options, callback) {
+  ({ file, args, options, callback } = normalizeExecFileArgs(file, args, options, callback));
 
   options = {
     encoding: 'utf8',
@@ -824,7 +845,7 @@ function checkExecSyncError(ret, args, cmd) {
 
 /**
  * Spawns a file as a shell synchronously.
- * @param {string} command
+ * @param {string} file
  * @param {string[]} [args]
  * @param {{
  *   cwd?: string;
@@ -842,17 +863,18 @@ function checkExecSyncError(ret, args, cmd) {
  *   }} [options]
  * @returns {Buffer | string}
  */
-function execFileSync(command, args, options) {
-  options = normalizeSpawnArguments(command, args, options);
+function execFileSync(file, args, options) {
+  ({ file, args, options } = normalizeExecFileArgs(file, args, options));
 
   const inheritStderr = !options.stdio;
-  const ret = spawnSync(options.file,
-                        ArrayPrototypeSlice(options.args, 1), options);
+  const ret = spawnSync(file, args, options);
 
   if (inheritStderr && ret.stderr)
     process.stderr.write(ret.stderr);
 
-  const err = checkExecSyncError(ret, options.args, undefined);
+  const errArgs = [options.argv0 || file];
+  ArrayPrototypePushApply(errArgs, args);
+  const err = checkExecSyncError(ret, errArgs);
 
   if (err)
     throw err;
diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js
index 40cb8cd3afab01..f51c1729c9ac79 100644
--- a/test/parallel/test-child-process-execfile.js
+++ b/test/parallel/test-child-process-execfile.js
@@ -2,10 +2,11 @@
 
 const common = require('../common');
 const assert = require('assert');
-const execFile = require('child_process').execFile;
+const { execFile, execFileSync } = require('child_process');
 const { getEventListeners } = require('events');
 const { getSystemErrorName } = require('util');
 const fixtures = require('../common/fixtures');
+const os = require('os');
 
 const fixture = fixtures.path('exit.js');
 const echoFixture = fixtures.path('echo.js');
@@ -99,3 +100,23 @@ const execOpts = { encoding: 'utf8', shell: true };
   });
   execFile(process.execPath, [fixture, 0], { signal }, callback);
 }
+
+// Verify the execFile() stdout is the same as execFileSync().
+{
+  const file = 'echo';
+  const args = ['foo', 'bar'];
+
+  // Test with and without `{ shell: true }`
+  [
+    // Skipping shell-less test on Windows because its echo command is a shell built-in command.
+    ...(common.isWindows ? [] : [{ encoding: 'utf8' }]),
+    { shell: true, encoding: 'utf8' },
+  ].forEach((options) => {
+    const execFileSyncStdout = execFileSync(file, args, options);
+    assert.strictEqual(execFileSyncStdout, `foo bar${os.EOL}`);
+
+    execFile(file, args, options, common.mustCall((_, stdout) => {
+      assert.strictEqual(stdout, execFileSyncStdout);
+    }));
+  });
+}