From eeae1e117c1f7563663736159ab10a4e56fe0e65 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 27 Oct 2014 11:03:13 -0700 Subject: [PATCH 1/5] doc: document the fds behind stdin/out/err Its common knowledge on unix, but node documentation depends on knowing this, as it exposes both streams named after stdio, and the fd numbers, so make this explicit. Closes #8624 --- doc/api/process.markdown | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/process.markdown b/doc/api/process.markdown index 04da4a6f8dea..663d9c159a45 100644 --- a/doc/api/process.markdown +++ b/doc/api/process.markdown @@ -116,7 +116,7 @@ emulation with `process.kill()`, and `child_process.kill()`: ## process.stdout -A `Writable Stream` to `stdout`. +A `Writable Stream` to `stdout` (on fd `1`). Example: the definition of `console.log` @@ -150,7 +150,7 @@ See [the tty docs](tty.html#tty_tty) for more information. ## process.stderr -A writable stream to stderr. +A writable stream to stderr (on fd `2`). `process.stderr` and `process.stdout` are unlike other streams in Node in that writes to them are usually blocking. @@ -164,7 +164,7 @@ that writes to them are usually blocking. ## process.stdin -A `Readable Stream` for stdin. +A `Readable Stream` for stdin (on fd `0`). Example of opening standard input and listening for both events: From 178f0ab891fa9f8683d778452c1315e6ab189a66 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 25 Sep 2014 22:19:40 -0700 Subject: [PATCH 2/5] test: use assert.throw to test exceptions The test wasn't checking directly that an assertion was thrown. Instead, it was checking that spawn did not sucessfully spawn a non-existent command. However, the command chosen, dir, exists in GNU coreutils, so it exists on Linux (though not on BSD derived OS X). The test as written passed on Linux, even with the TypeError it is supposed to be checking for deleted from spawn(). It would also pass on Windows if a ls.exe existed. The approach is unnecessarily obscure, `assert.throw()` is for asserting code throws, using it is more clear and works regardless of what commands do or do not exist. --- .../test-child-process-spawn-typeerror.js | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/test/simple/test-child-process-spawn-typeerror.js b/test/simple/test-child-process-spawn-typeerror.js index d18ce943ec92..d13d5e62e4d5 100644 --- a/test/simple/test-child-process-spawn-typeerror.js +++ b/test/simple/test-child-process-spawn-typeerror.js @@ -19,30 +19,18 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -var spawn = require('child_process').spawn, - assert = require('assert'), - windows = (process.platform === 'win32'), - cmd = (windows) ? 'dir' : 'ls', - invalidcmd = (windows) ? 'ls' : 'dir', - errors = 0; +var assert = require('assert'); +var child_process = require('child_process'); +var spawn = child_process.spawn; +var cmd = (process.platform === 'win32') ? 'dir' : 'ls'; -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 args argument must be an array +assert.throws(function() { + spawn(cmd, 'this is not an array'); +}, TypeError); // verify that args argument is optional assert.doesNotThrow(function() { spawn(cmd, {}); }); - -process.on('exit', function() { - assert.equal(errors, 0); -}); From 40daa7515dccf4724d4a29bc9a74bc057ae2b41a Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 25 Sep 2014 22:28:33 -0700 Subject: [PATCH 3/5] child_process: check execFile args is an array execFile and spawn have same API signature with respect to optional arg array and optional options object, they should have same behaviour with respect to argument validation. --- lib/child_process.js | 30 ++++++++++++------- .../test-child-process-spawn-typeerror.js | 11 +++++++ 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 0c1b4c99c1fb..53e0bb24ff47 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -591,7 +591,7 @@ exports.exec = function(command /*, options, callback */) { exports.execFile = function(file /* args, options, callback */) { - var args, optionArg, callback; + var args = [], optionArg, callback; var options = { encoding: 'utf8', timeout: 0, @@ -601,18 +601,28 @@ 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/simple/test-child-process-spawn-typeerror.js b/test/simple/test-child-process-spawn-typeerror.js index d13d5e62e4d5..b5d1249e5883 100644 --- a/test/simple/test-child-process-spawn-typeerror.js +++ b/test/simple/test-child-process-spawn-typeerror.js @@ -22,6 +22,7 @@ var assert = require('assert'); var child_process = require('child_process'); var spawn = child_process.spawn; +var execFile = child_process.execFile; var cmd = (process.platform === 'win32') ? 'dir' : 'ls'; @@ -34,3 +35,13 @@ assert.throws(function() { assert.doesNotThrow(function() { spawn(cmd, {}); }); + + +// verify that execFile has same argument parsing behaviour as spawn +assert.throws(function() { + execFile(cmd, 'this is not an array'); +}, TypeError); + +assert.doesNotThrow(function() { + execFile(cmd, {}); +}); From ce20b65c92c9b965c5cf0bfae919937ac62dd041 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Mon, 29 Sep 2014 11:26:43 -0700 Subject: [PATCH 4/5] child_process: check fork args is an array Optional fork args should be type-checked with same behaviour as the equivalent argument to spawn. --- lib/child_process.js | 2 ++ test/simple/test-child-process-spawn-typeerror.js | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/child_process.js b/lib/child_process.js index 53e0bb24ff47..640f68e617af 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -525,6 +525,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]); diff --git a/test/simple/test-child-process-spawn-typeerror.js b/test/simple/test-child-process-spawn-typeerror.js index b5d1249e5883..e28bd2a519a4 100644 --- a/test/simple/test-child-process-spawn-typeerror.js +++ b/test/simple/test-child-process-spawn-typeerror.js @@ -22,8 +22,10 @@ var assert = require('assert'); var child_process = require('child_process'); var spawn = child_process.spawn; +var fork = child_process.fork; var execFile = child_process.execFile; var cmd = (process.platform === 'win32') ? 'dir' : 'ls'; +var empty = require('../common').fixturesDir + '/empty.js'; // verify that args argument must be an array @@ -45,3 +47,12 @@ assert.throws(function() { assert.doesNotThrow(function() { execFile(cmd, {}); }); + +// verify that fork has same argument parsing behaviour as spawn +assert.throws(function() { + fork(empty, 'this is not an array'); +}, TypeError); + +assert.doesNotThrow(function() { + execFile(empty, {}); +}); From df567ab94ed60629be2939e0e3486fe7ced6088a Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 8 Oct 2014 13:30:38 -0700 Subject: [PATCH 5/5] fixup! thoroughly test all parameter positions --- .../test-child-process-spawn-typeerror.js | 93 ++++++++++++++----- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/test/simple/test-child-process-spawn-typeerror.js b/test/simple/test-child-process-spawn-typeerror.js index e28bd2a519a4..4a136db86c08 100644 --- a/test/simple/test-child-process-spawn-typeerror.js +++ b/test/simple/test-child-process-spawn-typeerror.js @@ -24,35 +24,86 @@ var child_process = require('child_process'); var spawn = child_process.spawn; var fork = child_process.fork; var execFile = child_process.execFile; -var cmd = (process.platform === 'win32') ? 'dir' : 'ls'; +var cmd = (process.platform === 'win32') ? 'rundll32' : 'ls'; var empty = require('../common').fixturesDir + '/empty.js'; +// Argument types for combinatorics +var a=[], o={}, c=(function callback(){}), s='string', u=undefined, n=null; -// verify that args argument must be an array -assert.throws(function() { - spawn(cmd, 'this is not an array'); -}, TypeError); +// 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); }); -// verify that args argument is optional -assert.doesNotThrow(function() { - spawn(cmd, {}); -}); +// Variants of undefined as explicit 'no argument' at a position +assert.doesNotThrow(function() { execFile(empty, u, o); }); +assert.doesNotThrow(function() { execFile(empty, a, u); }); +assert.doesNotThrow(function() { execFile(empty, n, o); }); +assert.doesNotThrow(function() { execFile(empty, a, n); }); + +assert.throws(function() { spawn(cmd, s); }, TypeError); +assert.doesNotThrow(function() { spawn(cmd, a, s); }, TypeError); // verify that execFile has same argument parsing behaviour as spawn -assert.throws(function() { - execFile(cmd, 'this is not an array'); -}, TypeError); +// +// 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); }); -assert.doesNotThrow(function() { - execFile(cmd, {}); -}); // verify that fork has same argument parsing behaviour as spawn -assert.throws(function() { - fork(empty, 'this is not an array'); -}, TypeError); +// +// 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); }); + +// Variants of undefined as explicit 'no argument' at a position +assert.doesNotThrow(function() { execFile(empty, u, o); }); +assert.doesNotThrow(function() { execFile(empty, a, u); }); +assert.doesNotThrow(function() { execFile(empty, n, o); }); +assert.doesNotThrow(function() { execFile(empty, a, n); }); -assert.doesNotThrow(function() { - execFile(empty, {}); -}); +assert.throws(function() { fork(empty, s); }, TypeError); +assert.doesNotThrow(function() { fork(empty, a, s); }, TypeError);