From 82465fc4b1f458e0c211d280fc25be1b75982f05 Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Wed, 7 Oct 2009 01:04:27 +0200 Subject: [PATCH] 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 | 10 ++-- lib/utils.js | 10 +++- src/child_process.cc | 62 +++++++++++++++++++++---- src/child_process.h | 2 +- src/node.js | 15 +++++- test/mjsunit/test-multipart.js | 2 +- test/mjsunit/test-process-spawn-loop.js | 10 +++- 7 files changed, 93 insertions(+), 18 deletions(-) diff --git a/doc/api.txt b/doc/api.txt index 3b72f555c9..8a283e6566 100644 --- a/doc/api.txt +++ b/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+ :: diff --git a/lib/utils.js b/lib/utils.js index 1c44567400..d33d504e17 100644 --- a/lib/utils.js +++ b/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(); diff --git a/src/child_process.cc b/src/child_process.cc index 74b1b7a1b6..23cfdd936a 100644 --- a/src/child_process.cc +++ b/src/child_process.cc @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -43,17 +44,58 @@ Handle 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 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(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 argv_handle = Local::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 env_handle = Local::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 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. diff --git a/src/child_process.h b/src/child_process.h index d3f78cdb2a..a9c595201a 100644 --- a/src/child_process.h +++ b/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); diff --git a/src/node.js b/src/node.js index a086edd22f..38548e8afe 100644 --- a/src/node.js +++ b/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; }; diff --git a/test/mjsunit/test-multipart.js b/test/mjsunit/test-multipart.js index 262429fe53..020e94b256 100644 --- a/test/mjsunit/test-multipart.js +++ b/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() { diff --git a/test/mjsunit/test-process-spawn-loop.js b/test/mjsunit/test-process-spawn-loop.js index 2b63dea67b..b17452dc88 100644 --- a/test/mjsunit/test-process-spawn-loop.js +++ b/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