From 609db5a1ddcd93ecbdc99bdc8bb32e79532071dd Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 2 Sep 2015 14:16:24 -0700 Subject: [PATCH] child_process: check execFile and fork args Port of joyent/node commits: * https://github.com/nodejs/node-v0.x-archive/commit/e17c5a72b23f920f291d61f2780068c18768cb92 * https://github.com/nodejs/node-v0.x-archive/commit/70dafa7b624abd43432e03304d65cc527fbecc11 Pull over test-child-process-spawn-typeerror.js from v0.12, replacing the existing test in master. The new test includes a broader set of tests on the various arg choices and throws. Reviewed-By: trevnorris - Trevor Norris Reviewed-By: cjihrig - Colin Ihrig Reviewed-By: thefourtheye - Sakthipriyan Vairamani PR-URL: https://github.com/nodejs/node/pull/2667 Fixes: https://github.com/nodejs/node/issues/2515 --- lib/child_process.js | 30 +++-- .../test-child-process-spawn-typeerror.js | 103 ++++++++++++++++-- 2 files changed, 114 insertions(+), 19 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index a923477846..0fe9ca75c7 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -23,6 +23,8 @@ exports.fork = function(modulePath /*, args, options*/) { 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]); @@ -104,7 +106,7 @@ exports.exec = function(command /*, options, callback*/) { exports.execFile = function(file /*, args, options, callback*/) { - var args, callback; + var args = [], callback; var options = { encoding: 'utf8', timeout: 0, @@ -114,18 +116,26 @@ exports.execFile = function(file /*, args, options, callback*/) { env: null }; - // Parse the parameters. + // 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 (typeof arguments[arguments.length - 1] === 'function') { - callback = arguments[arguments.length - 1]; + if (pos < arguments.length && typeof arguments[pos] === 'object') { + options = util._extend(options, arguments[pos++]); + } else if (pos < arguments.length && arguments[pos] == null) { + pos++; } - if (Array.isArray(arguments[1])) { - args = arguments[1]; - options = util._extend(options, arguments[2]); - } else { - args = []; - options = util._extend(options, arguments[1]); + 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 child = spawn(file, args, { diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 348ab2491d..44d67e8608 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -1,14 +1,19 @@ 'use strict'; -var assert = require('assert'); -var child_process = require('child_process'); -var spawn = child_process.spawn; -var cmd = require('../common').isWindows ? 'rundll32' : 'ls'; -var invalidArgsMsg = /Incorrect value of args option/; -var invalidOptionsMsg = /options argument must be an object/; - -// verify that args argument must be an array +const assert = require('assert'); +const child_process = require('child_process'); +const spawn = child_process.spawn; +const fork = child_process.fork; +const execFile = child_process.execFile; +const common = require('../common'); +const cmd = common.isWindows ? 'rundll32' : 'ls'; +const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; +const invalidArgsMsg = /Incorrect value of args option/; +const invalidOptionsMsg = /options argument must be an object/; +const empty = common.fixturesDir + '/empty.js'; + assert.throws(function() { - spawn(cmd, 'this is not an array'); + var child = spawn(invalidcmd, 'this is not an array'); + child.on('error', assert.fail); }, TypeError); // verify that valid argument combinations do not throw @@ -49,3 +54,83 @@ assert.throws(function() { spawn(cmd, [], 1); }, invalidOptionsMsg); +// Argument types for combinatorics +const a = []; +const o = {}; +const c = function callback() {}; +const s = 'string'; +const u = undefined; +const n = null; + +// function spawn(file=f [,args=a] [, options=o]) has valid combinations: +// (f) +// (f, a) +// (f, a, o) +// (f, o) +assert.doesNotThrow(function() { spawn(cmd); }); +assert.doesNotThrow(function() { spawn(cmd, a); }); +assert.doesNotThrow(function() { spawn(cmd, a, o); }); +assert.doesNotThrow(function() { spawn(cmd, o); }); + +// Variants of undefined as explicit 'no argument' at a position +assert.doesNotThrow(function() { spawn(cmd, u, o); }); +assert.doesNotThrow(function() { spawn(cmd, a, u); }); + +assert.throws(function() { spawn(cmd, n, o); }, TypeError); +assert.throws(function() { spawn(cmd, a, n); }, TypeError); + +assert.throws(function() { spawn(cmd, s); }, TypeError); +assert.throws(function() { spawn(cmd, a, s); }, TypeError); + + +// verify that execFile has same argument parsing behaviour as spawn +// +// function execFile(file=f [,args=a] [, options=o] [, callback=c]) has valid +// combinations: +// (f) +// (f, a) +// (f, a, o) +// (f, a, o, c) +// (f, a, c) +// (f, o) +// (f, o, c) +// (f, c) +assert.doesNotThrow(function() { execFile(cmd); }); +assert.doesNotThrow(function() { execFile(cmd, a); }); +assert.doesNotThrow(function() { execFile(cmd, a, o); }); +assert.doesNotThrow(function() { execFile(cmd, a, o, c); }); +assert.doesNotThrow(function() { execFile(cmd, a, c); }); +assert.doesNotThrow(function() { execFile(cmd, o); }); +assert.doesNotThrow(function() { execFile(cmd, o, c); }); +assert.doesNotThrow(function() { execFile(cmd, c); }); + +// Variants of undefined as explicit 'no argument' at a position +assert.doesNotThrow(function() { execFile(cmd, u, o, c); }); +assert.doesNotThrow(function() { execFile(cmd, a, u, c); }); +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); }); + +// 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); }); + + +// verify that fork has same argument parsing behaviour as spawn +// +// function fork(file=f [,args=a] [, options=o]) has valid combinations: +// (f) +// (f, a) +// (f, a, o) +// (f, o) +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.throws(function() { fork(empty, s); }, TypeError); +assert.doesNotThrow(function() { fork(empty, a, s); }, TypeError);