Skip to content

Commit

Permalink
child_process: validate execFile & fork arguments
Browse files Browse the repository at this point in the history 
The argument parsing for execFile and fork are inconsistent. execFile
throws on one invalid argument but not others. fork has similar logic
but the implementation is very different. This update implements
consistency for both functions.

Fixes: nodejs#2681
  • Loading branch information
@ChuckLangford
ChuckLangford committed Jan 12, 2016
1 parent eeb6fdc commit d8e5d05
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 33 deletions.
114 changes: 84 additions & 30 deletions lib/child_process.js
View file
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);
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) {
var pos = 1, arrayPos = -1, objectPos = -1, args, options;
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);
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
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);

0 comments on commit d8e5d05

Please sign in to comment.