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 · 8557b8b · 8557b8b
1 parent 5cc4b69
commit 8557b8b
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 37 deletions.
diff --git a/lib/child_process.js b/lib/child_process.js
@@ -76,6 +76,7 @@ const {
   isInt32,
   validateAbortSignal,
   validateBoolean,
+  validateFunction,
   validateObject,
   validateString,
 } = require('internal/validators');
@@ -126,13 +127,10 @@ function fork(modulePath, args = [], options) {
     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 +274,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
@@ -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.
 
@@ -191,4 +191,4 @@ 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, a, s); }, invalidArgTypeError);