From 26c973d4b33210111034c360bf6f6f44d1f41cdc Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 23 Dec 2021 23:02:13 +0000 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/41305 Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Minwoo Jung Reviewed-By: Antoine du Hamel --- lib/child_process.js | 55 ++++++++----------- test/parallel/test-child-process-fork-args.js | 4 +- .../test-child-process-spawn-typeerror.js | 36 ++++++------ 3 files changed, 43 insertions(+), 52 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index a7ef8ba1e4af1a..f4ada2f0aab8e3 100644 --- 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,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) { diff --git a/test/parallel/test-child-process-fork-args.js b/test/parallel/test-child-process-fork-args.js index 68863ffb14b747..2ed31fa4e9ce31 100644 --- 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 index 25d1bafe4513cc..545e2e2b3a4818 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/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);