Skip to content

Commit

Permalink
child_process: improve argument validation
Browse files Browse the repository at this point in the history 
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
@Trott
Trott committed Dec 30, 2021
1 parent db02f6f commit 26c973d
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 52 deletions.
55 changes: 22 additions & 33 deletions lib/child_process.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ const { getValidatedPath } = require('internal/fs/utils');
const {
isInt32,
validateAbortSignal,
validateArray,
validateBoolean,
validateFunction,
validateObject,
validateString,
} = require('internal/validators');
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -391,7 +380,7 @@ function execFile(file, args = [], options, callback) {
return;
}

if (args.length !== 0)
if (args?.length)
cmd += ` ${ArrayPrototypeJoin(args, ' ')}`;

if (!ex) {
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-child-process-fork-args.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}
);
Expand Down Expand Up @@ -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'
}
);
Expand Down
36 changes: 19 additions & 17 deletions test/parallel/test-child-process-spawn-typeerror.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand All @@ -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);

0 comments on commit 26c973d

Please sign in to comment.