From 56bb9421783a9f42e5fc791c0f16f9efbd9ec70c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 2 Sep 2015 14:16:24 -0700 Subject: [PATCH 1/2] 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. --- lib/child_process.js | 30 +++-- .../test-child-process-spawn-typeerror.js | 119 ++++++++++++++++-- 2 files changed, 128 insertions(+), 21 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index a92347784612a5..0fe9ca75c7794a 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 348ab2491dd2f8..f6292db2f47eb1 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -1,15 +1,28 @@ '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 -assert.throws(function() { - spawn(cmd, 'this is not an array'); -}, TypeError); +var assert = require('assert'), + child_process = require('child_process'), + spawn = child_process.spawn, + fork = child_process.fork, + execFile = child_process.execFile, + windows = (process.platform === 'win32'), + cmd = (windows) ? 'rundll32' : 'ls', + invalidcmd = 'hopefully_you_dont_have_this_on_your_machine', + invalidArgsMsg = /Incorrect value of args option/, + invalidOptionsMsg = /options argument must be an object/, + empty = require('../common').fixturesDir + '/empty.js', + errors = 0; + +try { + // Ensure this throws a TypeError + var child = spawn(invalidcmd, 'this is not an array'); + + child.on('error', function(err) { + errors++; + }); + +} catch (e) { + assert.equal(e instanceof TypeError, true); +} // verify that valid argument combinations do not throw assert.doesNotThrow(function() { @@ -49,3 +62,87 @@ assert.throws(function() { spawn(cmd, [], 1); }, invalidOptionsMsg); +process.on('exit', function() { + assert.equal(errors, 0); +}); + +// Argument types for combinatorics +var a = [], + o = {}, + c = (function callback() {}), + s = 'string', + u = undefined, + 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); From 7e5710a58ea75eb53b70b8808ecf62336fedf4eb Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 2 Sep 2015 14:59:18 -0700 Subject: [PATCH 2/2] test: fix style nits in test-child-process-spawn-typeerror.js Fixing multiple style nits in the test pulled over from v0.12. --- .../test-child-process-spawn-typeerror.js | 54 ++++++++----------- 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index f6292db2f47eb1..44d67e86086f0b 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -1,28 +1,20 @@ 'use strict'; -var assert = require('assert'), - child_process = require('child_process'), - spawn = child_process.spawn, - fork = child_process.fork, - execFile = child_process.execFile, - windows = (process.platform === 'win32'), - cmd = (windows) ? 'rundll32' : 'ls', - invalidcmd = 'hopefully_you_dont_have_this_on_your_machine', - invalidArgsMsg = /Incorrect value of args option/, - invalidOptionsMsg = /options argument must be an object/, - empty = require('../common').fixturesDir + '/empty.js', - errors = 0; - -try { - // Ensure this throws a TypeError - var child = spawn(invalidcmd, 'this is not an array'); - - child.on('error', function(err) { - errors++; - }); +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'; -} catch (e) { - assert.equal(e instanceof TypeError, true); -} +assert.throws(function() { + var child = spawn(invalidcmd, 'this is not an array'); + child.on('error', assert.fail); +}, TypeError); // verify that valid argument combinations do not throw assert.doesNotThrow(function() { @@ -62,17 +54,13 @@ assert.throws(function() { spawn(cmd, [], 1); }, invalidOptionsMsg); -process.on('exit', function() { - assert.equal(errors, 0); -}); - // Argument types for combinatorics -var a = [], - o = {}, - c = (function callback() {}), - s = 'string', - u = undefined, - n = null; +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)