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.
nodejs · Dec 25, 2021 · 55ad9b1 · 55ad9b1
1 parent 5cc4b69
commit 55ad9b1
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 42 deletions.
diff --git a/lib/child_process.js b/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,17 @@ 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', { allowArray: true });
   }
+  options = { ...options };
 
   // Prepare arguments for fork:
   execArgv = options.execArgv || process.execArgv;
@@ -276,33 +275,26 @@ ObjectDefineProperty(exec, promisify.custom, {
  * @returns {ChildProcess}
  */
 function execFile(file, args = [], options, callback) {
-  if (args == null) {
+  if (args != null && typeof args === 'object' && !ArrayIsArray(args)) {
+    callback = options;
+    options = args;
     args = [];
-  } else if (typeof args === 'object') {
-    if (!ArrayIsArray(args)) {
-      callback = options;
-      options = args;
-      args = [];
-    }
   } else if (typeof args === 'function') {
     callback = args;
-    options = {};
+    options = null;
     args = [];
-  } else {
-    throw new ERR_INVALID_ARG_VALUE('args', args);
   }
 
-  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) {
+    // TODO: Should this have allowArray: true set?
+    validateObject(options, 'options');
   }
 
-  if (callback && typeof callback !== 'function') {
-    throw new ERR_INVALID_ARG_VALUE('callback', callback);
+  if (callback != null) {
+    validateFunction(callback, 'callback');
   }
 
   options = {

diff --git a/test/parallel/test-child-process-fork-args.js b/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'
       }
     );

diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js
@@ -158,17 +158,17 @@ 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(function() { execFile(cmd, s, o, c); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, a, s, c); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, a, o, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, a, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, o, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, u, u, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, n, n, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, a, u, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, a, n, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, u, o, s); }, invalidArgTypeError);
+assert.throws(function() { execFile(cmd, n, o, s); }, invalidArgTypeError);
 
 execFile(cmd, c, s); // Should not throw.
 
@@ -190,5 +190,5 @@ 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(function() { fork(empty, s); }, invalidArgTypeError);
+assert.throws(function() { fork(empty, a, s); }, invalidArgTypeError);