Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: validate execFile & fork arguments #4508

Closed
wants to merge 2 commits into from
Closed

child_process: validate execFile & fork arguments #4508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8e5d05
child_process: validate execFile & fork arguments
ChuckLangford Dec 30, 2015
54d1461
child_process: remove unused variable
ChuckLangford Jan 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 84 additions & 30 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,10 @@ exports.fork = function(modulePath /*, args, options*/) {

// Get options and args arguments.
var options, args, execArgv;
if (Array.isArray(arguments[1])) {
args = arguments[1];
options = util._extend({}, arguments[2]);
} else if (arguments[1] && typeof arguments[1] !== 'object') {
throw new TypeError('Incorrect value of args option');
} else {
args = [];
options = util._extend({}, arguments[1]);
}

var opts = normalizeForkArgs(arguments, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a normalizeForkArgs(), as this code is specific to fork(). If the logic needs to be tightened up, it can be done here. fork() is one of the simpler cases, as we only need to validate an optional array and an optional object.

args = opts.args;
options = opts.options;

// Prepare arguments for fork:
execArgv = options.execArgv || process.execArgv;
Expand Down Expand Up @@ -114,6 +109,82 @@ exports.exec = function(command /*, options, callback*/) {
opts.callback);
};

function normalizeArgsOptions(allArgs, defaultOption) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it is going to be a nightmare to maintain. If we can't do the validation more simply, I think we'd be better off skipping it or loosening its strictness.

var pos = 1, arrayPos = -1, objectPos = -1, args;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're using let and const for new code these days, best switch where appropriate

if (pos < allArgs.length && Array.isArray(allArgs[pos])) {
arrayPos = pos++;
args = allArgs[arrayPos];
} else if (pos < allArgs.length && allArgs[pos] == null) {
arrayPos = pos++;
}

if (pos < allArgs.length && typeof allArgs[pos] === 'object') {
objectPos = pos++;
defaultOption = util._extend(defaultOption, allArgs[objectPos]);
} else if (pos < allArgs.length && allArgs[pos] == null) {
objectPos = pos++;
}

return {
pos: pos,
args: args || [],
options: defaultOption,
arrayPos: arrayPos,
objectPos: objectPos
};
}

function normalizeForkArgs(allArgs, defaultOption) {
if (!defaultOption) {
defaultOption = {};
}

var opts = normalizeArgsOptions(allArgs, defaultOption);

if (opts.pos === 2 &&
allArgs.length > 2 &&
opts.arrayPos > -1) {
throw new TypeError('Incorrect value of options option');
}

if (opts.pos === 1 && allArgs.length > 1) {
throw new TypeError('Incorrect value of args option');
}

return opts;
}

function normalizeExecFileArgs(allArgs, defaultOption) {
var opts = normalizeArgsOptions(allArgs, defaultOption);

if (opts.pos < allArgs.length && typeof allArgs[opts.pos] === 'function') {
opts.callback = allArgs[opts.pos++];
}

if (opts.pos === 3 &&
allArgs.length > 3 &&
typeof opts.callback !== 'function' &&
allArgs[3] !== undefined &&
allArgs[3] !== null) {
throw new TypeError('Incorrect value of callback option');
}

if (opts.pos === 2 && allArgs.length > 2) {
if (opts.arrayPos > -1) {
throw new TypeError('Incorrect value of options option');
} else if (opts.objectPos > -1 &&
allArgs[opts.pos] !== null &&
allArgs[opts.pos] !== undefined) {
throw new TypeError('Incorrect value of callback option');
}
}

if (opts.pos === 1 && allArgs.length > 1) {
throw new TypeError('Incorrect value of args option');
}

return opts;
}

exports.execFile = function(file /*, args, options, callback*/) {
var args = [], callback;
Expand All @@ -126,27 +197,10 @@ exports.execFile = function(file /*, args, options, callback*/) {
env: null
};

// Parse the optional positional parameters.
var pos = 1;
if (pos < arguments.length && Array.isArray(arguments[pos])) {
args = arguments[pos++];
} else if (pos < arguments.length && arguments[pos] == null) {
pos++;
}

if (pos < arguments.length && typeof arguments[pos] === 'object') {
options = util._extend(options, arguments[pos++]);
} else if (pos < arguments.length && arguments[pos] == null) {
pos++;
}

if (pos < arguments.length && typeof arguments[pos] === 'function') {
callback = arguments[pos++];
}

if (pos === 1 && arguments.length > 1) {
throw new TypeError('Incorrect value of args option');
}
var opts = normalizeExecFileArgs(arguments, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't passing arguments to all of these functions cause V8 to deoptimize?

args = opts.args;
options = opts.options;
callback = opts.callback;

var child = spawn(file, args, {
cwd: options.cwd,
Expand Down
35 changes: 32 additions & 3 deletions test/parallel/test-child-process-spawn-typeerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,36 @@ assert.doesNotThrow(function() { execFile(cmd, a, o, u); });
assert.doesNotThrow(function() { execFile(cmd, n, o, c); });
assert.doesNotThrow(function() { execFile(cmd, a, n, c); });
assert.doesNotThrow(function() { execFile(cmd, a, o, n); });
assert.doesNotThrow(function() { execFile(cmd, u, u, u); });
assert.doesNotThrow(function() { execFile(cmd, u, u, c); });
assert.doesNotThrow(function() { execFile(cmd, u, o, u); });
assert.doesNotThrow(function() { execFile(cmd, a, u, u); });
assert.doesNotThrow(function() { execFile(cmd, n, n, n); });
assert.doesNotThrow(function() { execFile(cmd, n, n, c); });
assert.doesNotThrow(function() { execFile(cmd, n, o, n); });
assert.doesNotThrow(function() { execFile(cmd, a, n, n); });
assert.doesNotThrow(function() { execFile(cmd, a, u); });
assert.doesNotThrow(function() { execFile(cmd, a, n); });
assert.doesNotThrow(function() { execFile(cmd, o, u); });
assert.doesNotThrow(function() { execFile(cmd, o, n); });
assert.doesNotThrow(function() { execFile(cmd, c, u); });
assert.doesNotThrow(function() { 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); }, TypeError);
assert.doesNotThrow(function() { execFile(cmd, a, s, c); });
assert.doesNotThrow(function() { execFile(cmd, a, o, s); });
assert.throws(function() { execFile(cmd, a, s, c); }, TypeError);
assert.throws(function() { execFile(cmd, a, o, s); }, TypeError);
assert.throws(function() { execFile(cmd, a, s); }, TypeError);
assert.throws(function() { execFile(cmd, o, s); }, TypeError);
assert.throws(function() { execFile(cmd, u, u, s); }, TypeError);
assert.throws(function() { execFile(cmd, n, n, s); }, TypeError);
assert.throws(function() { execFile(cmd, a, u, s); }, TypeError);
assert.throws(function() { execFile(cmd, a, n, s); }, TypeError);
assert.throws(function() { execFile(cmd, u, o, s); }, TypeError);
assert.throws(function() { execFile(cmd, n, o, s); }, TypeError);
assert.doesNotThrow(function() { execFile(cmd, c, s); });


// verify that fork has same argument parsing behaviour as spawn
Expand All @@ -131,6 +154,12 @@ assert.doesNotThrow(function() { fork(empty); });
assert.doesNotThrow(function() { fork(empty, a); });
assert.doesNotThrow(function() { fork(empty, a, o); });
assert.doesNotThrow(function() { fork(empty, o); });
assert.doesNotThrow(function() { fork(empty, u, u); });
assert.doesNotThrow(function() { fork(empty, u, o); });
assert.doesNotThrow(function() { fork(empty, a, u); });
assert.doesNotThrow(function() { fork(empty, n, n); });
assert.doesNotThrow(function() { fork(empty, n, o); });
assert.doesNotThrow(function() { fork(empty, a, n); });

assert.throws(function() { fork(empty, s); }, TypeError);
assert.doesNotThrow(function() { fork(empty, a, s); }, TypeError);
assert.throws(function() { fork(empty, a, s); }, TypeError);