Do not use /bin/sh to create child processes.

Instead directly call execvp(). This change is needed for the
soon-to-be-added signal handlers because the /bin/sh parent process does not
pass all signals to it's children, particularly SIGUSR1 on Linux.

The parameters of createChildProcess had to be changed slightly.
utils.exec() also has a changed implementation. A bug involving quoted
arguments was knowingly introduced into utils.exec(). Will fix later.

doc/api.txt

@@ -398,15 +398,19 @@ Node provides a tridirectional +popen(3)+ facility through the class
                             +"error"+ callbacks will no longer be made.
 |=========================================================
 
-+node.createChildProcess(command)+::
-Launches a new process with the given +command+. For example:
++node.createChildProcess(command, args=[], env=ENV)+::
+Launches a new process with the given +command+, command line arguments, and
+environmental variables. For example:
 +
 ----------------------------------------
-var ls = node.createChildProcess("ls -lh /usr");
+var ls = node.createChildProcess("ls", ["-lh", "/usr"]);
 ls.addListener("output", function (data) {
   puts(data);
 });
 ----------------------------------------
++
+Note, if you just want to buffer the output of a command and return it, then
++exec()+ in +/utils.js+ might be better.
 
 
 +child.pid+ ::

lib/utils.js

@@ -44,7 +44,15 @@ exports.p = function (x) {
 };
 
 exports.exec = function (command) {
-  var child = node.createChildProcess(command);
+  // TODO TODO TODO
+  // The following line needs to be replaced with proper command line
+  // parsing. at the moment quoted strings will not be picked up as a single
+  // argument.
+  var args = command.split(/\s+/);
+
+  var file = args.shift();
+
+  var child = node.createChildProcess(file, args);
   var stdout = "";
   var stderr = "";
   var promise = new node.Promise();

src/child_process.cc

@@ -2,6 +2,7 @@
 #include <child_process.h>
 
 #include <assert.h>
+#include <string.h>
 #include <stdlib.h>
 #include <errno.h>
 #include <unistd.h>
@@ -43,17 +44,58 @@ Handle<Value> ChildProcess::New(const Arguments& args) {
   return args.This();
 }
 
+// This is an internal function. The third argument should be an array
+// of key value pairs seperated with '='.
 Handle<Value> ChildProcess::Spawn(const Arguments& args) {
-  if (args.Length() == 0 || !args[0]->IsString()) {
+  HandleScope scope;
+
+  if ( args.Length() != 3
+    || !args[0]->IsString()
+    || !args[1]->IsArray()
+    || !args[2]->IsArray()
+     )
+  {
     return ThrowException(Exception::Error(String::New("Bad argument.")));
   }
 
-  HandleScope scope;
   ChildProcess *child = ObjectWrap::Unwrap<ChildProcess>(args.Holder());
 
-  String::Utf8Value command(args[0]->ToString());
+  String::Utf8Value file(args[0]->ToString());
+
+  int i;
+
+  // Copy second argument args[1] into a c-string array called argv.
+  // The array must be null terminated, and the first element must be
+  // the name of the executable -- hence the complication.
+  Local<Array> argv_handle = Local<Array>::Cast(args[1]);
+  int argc = argv_handle->Length();
+  int argv_length = argc + 1 + 1;
+  char **argv = new char*[argv_length]; // heap allocated to detect errors
+  argv[0] = strdup(*file); // + 1 for file
+  argv[argv_length-1] = NULL;  // + 1 for NULL;
+  for (i = 0; i < argc; i++) {
+    String::Utf8Value arg(argv_handle->Get(Integer::New(i))->ToString());
+    argv[i+1] = strdup(*arg);
+  }
+
+  // Copy third argument, args[2], into a c-string array called env.
+  Local<Array> env_handle = Local<Array>::Cast(args[2]);
+  int envc = env_handle->Length();
+  char **env = new char*[envc+1]; // heap allocated to detect errors
+  env[envc] = NULL;
+  for (int i = 0; i < envc; i++) {
+    String::Utf8Value pair(env_handle->Get(Integer::New(i))->ToString());
+    env[i] = strdup(*pair);
+  }
+
+  int r = child->Spawn(argv[0], argv, env);
+
+  for (i = 0; i < argv_length; i++) free(argv[i]);
+  delete [] argv;
+
+  for (i = 0; i < envc; i++) free(env[i]);
+  delete [] env;
 
-  int r = child->Spawn(*command);
   if (r != 0) {
     return ThrowException(Exception::Error(String::New("Error spawning")));
   }
@@ -94,8 +136,7 @@ Handle<Value> ChildProcess::Kill(const Arguments& args) {
   if (args[0]->IsInt32()) sig = args[0]->Int32Value();
 
   if (child->Kill(sig) != 0) {
-    return ThrowException(Exception::Error(
-          String::New("ChildProcess already dead")));
+    return ThrowException(Exception::Error(String::New(strerror(errno))));
   }
 
   return Undefined();
@@ -208,7 +249,9 @@ static inline int SetNonBlocking(int fd) {
   return r;
 }
 
-int ChildProcess::Spawn(const char *command) {
+// Note that args[0] must be the same as the "file" param.  This is an
+// execvp() requirement.
+int ChildProcess::Spawn(const char *file, char *const args[], char *const env[]) {
   assert(pid_ == 0);
   assert(stdout_fd_ == -1);
   assert(stderr_fd_ == -1);
@@ -247,8 +290,11 @@ int ChildProcess::Spawn(const char *command) {
       close(stdin_pipe[1]);  // close write end
       dup2(stdin_pipe[0],  STDIN_FILENO);
 
-      execl("/bin/sh", "sh", "-c", command, NULL);
+      execvp(file, args);
+      perror("execvp()");
       _exit(127);
+
+      // TODO search PATH and use: execve(file, argv, env);
   }
 
   // Parent.

src/child_process.h

@@ -28,7 +28,7 @@ class ChildProcess : EventEmitter {
   ChildProcess();
   ~ChildProcess();
 
-  int Spawn(const char *command);
+  int Spawn(const char *file, char *const argv[], char *const env[]);
   int Write(const char *str, size_t len);
   int Close(void);
   int Kill(int sig);

src/node.js

@@ -6,9 +6,20 @@ node.createProcess = function () {
   throw "node.createProcess() has been changed to node.createChildProcess() update your code";
 };
 
-node.createChildProcess = function (command) {
+node.createChildProcess = function (file, args, env) {
   var child = new node.ChildProcess();
-  child.spawn(command);
+  args = args || [];
+  env = env || process.ENV;
+  var envPairs = [];
+  for (var key in env) {
+    if (env.hasOwnProperty(key)) {
+      envPairs.push(key + "=" + env[key]);
+    }
+  }
+  // TODO Note envPairs is not currently used in child_process.cc. The PATH
+  // needs to be searched for the 'file' command if 'file' does not contain
+  // a '/' character.
+  child.spawn(file, args, envPairs);
   return child;
 };
 

test/mjsunit/test-multipart.js

@@ -44,7 +44,7 @@ var server = http.createServer(function(req, res) {
 });
 server.listen(port);
 
-var cmd = 'curl -H "Expect:" -F "test-field=foobar" -F test-file=@'+__filename+' http://localhost:'+port+'/';
+var cmd = 'curl -H Expect: -F test-field=foobar -F test-file=@'+__filename+' http://localhost:'+port+'/';
 var result = exec(cmd).wait();
 
 process.addListener('exit', function() {

test/mjsunit/test-process-spawn-loop.js

@@ -4,15 +4,21 @@ var N = 40;
 var finished = false;
 
 function spawn (i) {
-  var child = node.createChildProcess('python -c "print 500 * 1024 * \'C\'"');
+  var child = node.createChildProcess( 'python'
+                                     , ['-c', 'print 500 * 1024 * "C"']
+                                     );
   var output = "";
 
   child.addListener("output", function(chunk) {
     if (chunk) output += chunk;
   });
 
+  child.addListener("error", function(chunk) {
+    if (chunk) error(chunk)
+  });
+
   child.addListener("exit", function () {
-    //puts(output);
+    puts(output);
     if (i < N)
       spawn(i+1);
     else