child_process: improve argument validation

For execFile() and fork(), use INVALID_ARG_TYPE as appropriate instead
of INVALID_ARG_VALUE. Use validator functions where sensible.

PR-URL: #41305
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Author: Trott

lib/child_process.js

@@ -75,7 +75,9 @@ const { getValidatedPath } = require('internal/fs/utils');
 const {
   isInt32,
   validateAbortSignal,
+  validateArray,
   validateBoolean,
+  validateFunction,
   validateObject,
   validateString,
 } = require('internal/validators');
@@ -119,20 +121,18 @@ function fork(modulePath, args = [], options) {
 
   if (args == null) {
     args = [];
-  } else if (typeof args !== 'object') {
-    throw new ERR_INVALID_ARG_VALUE('args', args);
-  } else if (!ArrayIsArray(args)) {
+  } else if (typeof args === 'object' && !ArrayIsArray(args)) {
     options = args;
     args = [];
+  } else {
+    validateArray(args, 'args');
   }
 
-  if (options == null) {
-    options = {};
-  } else if (typeof options !== 'object') {
-    throw new ERR_INVALID_ARG_VALUE('options', options);
-  } else {
-    options = { ...options };
+  if (options != null) {
+    validateObject(options, 'options');
   }
+  options = { ...options, shell: false };
+  options.execPath = options.execPath || process.execPath;
 
   // Prepare arguments for fork:
   execArgv = options.execArgv || process.execArgv;
@@ -160,9 +160,6 @@ function fork(modulePath, args = [], options) {
     throw new ERR_CHILD_PROCESS_IPC_REQUIRED('options.stdio');
   }
 
-  options.execPath = options.execPath || process.execPath;
-  options.shell = false;
-
   return spawn(options.execPath, args, options);
 }
 
@@ -276,33 +273,25 @@ ObjectDefineProperty(exec, promisify.custom, {
  * @returns {ChildProcess}
  */
 function execFile(file, args = [], options, callback) {
-  if (args == null) {
-    args = [];
-  } else if (typeof args === 'object') {
-    if (!ArrayIsArray(args)) {
-      callback = options;
-      options = args;
-      args = [];
-    }
+  if (args != null && typeof args === 'object' && !ArrayIsArray(args)) {
+    callback = options;
+    options = args;
+    args = null;
   } else if (typeof args === 'function') {
     callback = args;
-    options = {};
-    args = [];
-  } else {
-    throw new ERR_INVALID_ARG_VALUE('args', args);
+    options = null;
+    args = null;
   }
 
-  if (options == null) {
-    options = {};
-  } else if (typeof options === 'function') {
+  if (typeof options === 'function') {
     callback = options;
-    options = {};
-  } else if (typeof options !== 'object') {
-    throw new ERR_INVALID_ARG_VALUE('options', options);
+    options = null;
+  } else if (options != null) {
+    validateObject(options, 'options');
   }
 
-  if (callback && typeof callback !== 'function') {
-    throw new ERR_INVALID_ARG_VALUE('callback', callback);
+  if (callback != null) {
+    validateFunction(callback, 'callback');
   }
 
   options = {
@@ -391,7 +380,7 @@ function execFile(file, args = [], options, callback) {
       return;
     }
 
-    if (args.length !== 0)
+    if (args?.length)
       cmd += ` ${ArrayPrototypeJoin(args, ' ')}`;
 
     if (!ex) {

test/parallel/test-child-process-fork-args.js

@@ -54,7 +54,7 @@ const expectedEnv = { foo: 'bar' };
         fork(fixtures.path('child-process-echo-options.js'), arg);
       },
       {
-        code: 'ERR_INVALID_ARG_VALUE',
+        code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError'
       }
     );
@@ -97,7 +97,7 @@ const expectedEnv = { foo: 'bar' };
         fork(fixtures.path('child-process-echo-options.js'), [], arg);
       },
       {
-        code: 'ERR_INVALID_ARG_VALUE',
+        code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError'
       }
     );

test/parallel/test-child-process-spawn-typeerror.js

@@ -106,10 +106,10 @@ spawn(cmd, u, o);
 spawn(cmd, n, o);
 spawn(cmd, a, u);
 
-assert.throws(function() { spawn(cmd, a, n); }, invalidArgTypeError);
-
-assert.throws(function() { spawn(cmd, s); }, invalidArgTypeError);
-assert.throws(function() { spawn(cmd, a, s); }, invalidArgTypeError);
+assert.throws(() => { spawn(cmd, a, n); }, invalidArgTypeError);
+assert.throws(() => { spawn(cmd, s); }, invalidArgTypeError);
+assert.throws(() => { spawn(cmd, a, s); }, invalidArgTypeError);
+assert.throws(() => { spawn(cmd, a, a); }, invalidArgTypeError);
 
 
 // Verify that execFile has same argument parsing behavior as spawn.
@@ -158,17 +158,18 @@ execFile(cmd, c, n);
 // String is invalid in arg position (this may seem strange, but is
 // consistent across node API, cf. `net.createServer('not options', 'not
 // callback')`.
-assert.throws(function() { execFile(cmd, s, o, c); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, a, s, c); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, a, o, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, a, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, o, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, u, u, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, n, n, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, a, u, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, a, n, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, u, o, s); }, invalidArgValueError);
-assert.throws(function() { execFile(cmd, n, o, s); }, invalidArgValueError);
+assert.throws(() => { execFile(cmd, s, o, c); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, s, c); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, o, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, o, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, u, u, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, n, n, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, u, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, n, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, u, o, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, n, o, s); }, invalidArgTypeError);
+assert.throws(() => { execFile(cmd, a, a); }, invalidArgTypeError);
 
 execFile(cmd, c, s); // Should not throw.
 
@@ -190,5 +191,6 @@ fork(empty, n, n);
 fork(empty, n, o);
 fork(empty, a, n);
 
-assert.throws(function() { fork(empty, s); }, invalidArgValueError);
-assert.throws(function() { fork(empty, a, s); }, invalidArgValueError);
+assert.throws(() => { fork(empty, s); }, invalidArgTypeError);
+assert.throws(() => { fork(empty, a, s); }, invalidArgTypeError);
+assert.throws(() => { fork(empty, a, a); }, invalidArgTypeError);