From d8e5d05a440c59d390d6799f65ecbf43b1071553 Mon Sep 17 00:00:00 2001 From: Chuck Langford Date: Wed, 30 Dec 2015 16:27:56 -0500 Subject: [PATCH 1/2] child_process: validate execFile & fork arguments 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: #2681 --- lib/child_process.js | 114 +++++++++++++----- .../test-child-process-spawn-typeerror.js | 35 +++++- 2 files changed, 116 insertions(+), 33 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index ee73562d24e80f..afef9e47b5b00e 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -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; @@ -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; @@ -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, diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index bf59779a75ff5d..20b6e4691055f4 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -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 @@ -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); From 54d1461d22e7778101058b2909efe80e1e14d1fe Mon Sep 17 00:00:00 2001 From: Chuck Langford Date: Tue, 12 Jan 2016 17:19:32 -0500 Subject: [PATCH 2/2] child_process: remove unused variable Updated tests caught an unused variable. Removed it. --- lib/child_process.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/child_process.js b/lib/child_process.js index afef9e47b5b00e..f15f1bf2170c95 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -110,7 +110,7 @@ exports.exec = function(command /*, options, callback*/) { }; function normalizeArgsOptions(allArgs, defaultOption) { - var pos = 1, arrayPos = -1, objectPos = -1, args, options; + var pos = 1, arrayPos = -1, objectPos = -1, args; if (pos < allArgs.length && Array.isArray(allArgs[pos])) { arrayPos = pos++; args = allArgs[arrayPos];