child_process: improve spawn() argument handling

Add stricter argument type checking to normalizeSpawnArguments().

Removes a number of extraneous checks in spawn().

Fix regression in handling of the optional args argument.

Add more thorough testing of spawn() arguments.

Reviewed-by: Trevor Norris <trev.norris@gmail.com>

diff --git a/lib/child_process.js b/lib/child_process.js
index 2396382..4840c9e 100644 (file)
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -930,28 +930,29 @@ function _validateStdio(stdio, sync) {
 }
 
 
-function normalizeSpawnArguments(/*file, args, options*/) {
+function normalizeSpawnArguments(file /*, args, options*/) {
   var args, options;
 
-  var file = arguments[0];
-
   if (Array.isArray(arguments[1])) {
     args = arguments[1].slice(0);
     options = arguments[2];
-  } else if (arguments[1] && !Array.isArray(arguments[1])) {
+  } else if (arguments[1] !== undefined && !util.isObject(arguments[1])) {
     throw new TypeError('Incorrect value of args option');
   } else {
     args = [];
     options = arguments[1];
   }
 
-  if (!options)
+  if (options === undefined)
     options = {};
+  else if (!util.isObject(options))
+    throw new TypeError('options argument must be an object');
 
   args.unshift(file);
 
-  var env = (options && options.env ? options.env : null) || process.env;
+  var env = options.env || process.env;
   var envPairs = [];
+
   for (var key in env) {
     envPairs.push(key + '=' + env[key]);
   }
@@ -969,24 +970,19 @@ function normalizeSpawnArguments(/*file, args, options*/) {
 
 var spawn = exports.spawn = function(/*file, args, options*/) {
   var opts = normalizeSpawnArguments.apply(null, arguments);
-
-  var file = opts.file;
-  var args = opts.args;
   var options = opts.options;
-  var envPairs = opts.envPairs;
-
   var child = new ChildProcess();
 
   child.spawn({
-    file: file,
-    args: args,
-    cwd: options ? options.cwd : null,
-    windowsVerbatimArguments: !!(options && options.windowsVerbatimArguments),
-    detached: !!(options && options.detached),
-    envPairs: envPairs,
-    stdio: options ? options.stdio : null,
-    uid: options ? options.uid : null,
-    gid: options ? options.gid : null
+    file: opts.file,
+    args: opts.args,
+    cwd: options.cwd,
+    windowsVerbatimArguments: !!options.windowsVerbatimArguments,
+    detached: !!options.detached,
+    envPairs: opts.envPairs,
+    stdio: options.stdio,
+    uid: options.uid,
+    gid: options.gid
   });
 
   return child;
@@ -1340,7 +1336,7 @@ function checkExecSyncError(ret) {
 
 function execFileSync(/*command, options*/) {
   var opts = normalizeSpawnArguments.apply(null, arguments);
-  var inheritStderr = !!!opts.options.stdio;
+  var inheritStderr = !opts.options.stdio;
 
   var ret = spawnSync(opts.file, opts.args.slice(1), opts.options);
 
@@ -1359,7 +1355,7 @@ exports.execFileSync = execFileSync;
 
 function execSync(/*comand, options*/) {
   var opts = normalizeExecArgs.apply(null, arguments);
-  var inheritStderr = opts.options ? !!!opts.options.stdio : true;
+  var inheritStderr = opts.options ? !opts.options.stdio : true;
 
   var ret = spawnSync(opts.file, opts.args, opts.options);
   ret.cmd = opts.cmd;

diff --git a/test/simple/test-child-process-spawn-typeerror.js b/test/simple/test-child-process-spawn-typeerror.js
index 791adcb..49bc74d 100644 (file)
--- a/test/simple/test-child-process-spawn-typeerror.js
+++ b/test/simple/test-child-process-spawn-typeerror.js
@@ -22,12 +22,15 @@
 var spawn = require('child_process').spawn,
   assert = require('assert'),
   windows = (process.platform === 'win32'),
-  cmd = (windows) ? 'ls' : 'dir',
+  cmd = (windows) ? 'dir' : 'ls',
+  invalidcmd = (windows) ? 'ls' : 'dir',
+  invalidArgsMsg = /Incorrect value of args option/,
+  invalidOptionsMsg = /options argument must be an object/,
   errors = 0;
 
 try {
   // Ensure this throws a TypeError
-  var child = spawn(cmd, 'this is not an array');
+  var child = spawn(invalidcmd, 'this is not an array');
 
   child.on('error', function (err) {
     errors++;
@@ -37,6 +40,44 @@ try {
   assert.equal(e instanceof TypeError, true);
 }
 
+// verify that valid argument combinations do not throw
+assert.doesNotThrow(function() {
+  spawn(cmd);
+});
+
+assert.doesNotThrow(function() {
+  spawn(cmd, []);
+});
+
+assert.doesNotThrow(function() {
+  spawn(cmd, {});
+});
+
+assert.doesNotThrow(function() {
+  spawn(cmd, [], {});
+});
+
+// verify that invalid argument combinations throw
+assert.throws(function() {
+  spawn();
+}, /Bad argument/);
+
+assert.throws(function() {
+  spawn(cmd, null);
+}, invalidArgsMsg);
+
+assert.throws(function() {
+  spawn(cmd, true);
+}, invalidArgsMsg);
+
+assert.throws(function() {
+  spawn(cmd, [], null);
+}, invalidOptionsMsg);
+
+assert.throws(function() {
+  spawn(cmd, [], 1);
+}, invalidOptionsMsg);
+
 process.on('exit', function() {
   assert.equal(errors, 0);
 });