From f764bff699e9897c3ed795a838acd9c1c8ec8133 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Thu, 31 Oct 2013 11:52:55 -0700 Subject: [PATCH] unix: return exec errors from uv_spawn, not async If spawning a process fails due to an exec() failure (but it succeeded in forking), then this should be considered a spawn failure instead of an asynchronous termination of the process. This allows to check for common exec() failure conditions such as a bad path quickly instead of having to rely on keeping track of the async callback. Additionally, the meaning of the two fields returned in the callback are now exactly what they advertise to be. The process exit argument is not one of two values depending on what happened to the child. Fixes #978. --- include/uv-unix.h | 1 - include/uv-win.h | 1 - include/uv.h | 12 ++++++++++- src/unix/process.c | 21 +++++++++---------- src/win/process.c | 23 +++++++-------------- test/test-spawn.c | 50 +++++++++++++--------------------------------- 6 files changed, 43 insertions(+), 65 deletions(-) diff --git a/include/uv-unix.h b/include/uv-unix.h index 965fbafb..45006092 100644 --- a/include/uv-unix.h +++ b/include/uv-unix.h @@ -287,7 +287,6 @@ typedef struct { #define UV_PROCESS_PRIVATE_FIELDS \ void* queue[2]; \ - int errorno; \ int status; \ #define UV_FS_PRIVATE_FIELDS \ diff --git a/include/uv-win.h b/include/uv-win.h index 512d7f89..e4e1f839 100644 --- a/include/uv-win.h +++ b/include/uv-win.h @@ -533,7 +533,6 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s); UV_REQ_FIELDS \ } exit_req; \ BYTE* child_stdio_buffer; \ - int spawn_error; \ int exit_signal; \ HANDLE wait_handle; \ HANDLE process_handle; \ diff --git a/include/uv.h b/include/uv.h index e6b290bc..c3ba2502 100644 --- a/include/uv.h +++ b/include/uv.h @@ -1489,7 +1489,17 @@ struct uv_process_s { UV_PROCESS_PRIVATE_FIELDS }; -/* Initializes uv_process_t and starts the process. */ +/* + * Initializes the uv_process_t and starts the process. If the process is + * successfully spawned, then this function will return 0. Otherwise, the + * negative error code corresponding to the reason it couldn't spawn is + * returned. + * + * Possible reasons for failing to spawn would include (but not be limited to) + * the file to execute not existing, not having permissions to use the setuid or + * setgid specified, or not having enough memory to allocate for the new + * process. + */ UV_EXTERN int uv_spawn(uv_loop_t* loop, uv_process_t* handle, const uv_process_options_t* options); diff --git a/src/unix/process.c b/src/unix/process.c index 42990ce5..6854ac11 100644 --- a/src/unix/process.c +++ b/src/unix/process.c @@ -109,9 +109,6 @@ static void uv__chld(uv_signal_t* handle, int signum) { if (WIFSIGNALED(process->status)) term_signal = WTERMSIG(process->status); - if (process->errorno != 0) - exit_status = process->errorno; /* execve() failed */ - process->exit_cb(process, exit_status, term_signal); } } @@ -359,6 +356,7 @@ int uv_spawn(uv_loop_t* loop, ssize_t r; pid_t pid; int err; + int exec_errorno; int i; assert(options->file != NULL); @@ -434,14 +432,14 @@ int uv_spawn(uv_loop_t* loop, uv__close(signal_pipe[1]); process->status = 0; - process->errorno = 0; + exec_errorno = 0; do - r = read(signal_pipe[0], &process->errorno, sizeof(process->errorno)); + r = read(signal_pipe[0], &exec_errorno, sizeof(exec_errorno)); while (r == -1 && errno == EINTR); if (r == 0) ; /* okay, EOF */ - else if (r == sizeof(process->errorno)) + else if (r == sizeof(exec_errorno)) ; /* okay, read errorno */ else if (r == -1 && errno == EPIPE) ; /* okay, got EPIPE */ @@ -461,15 +459,18 @@ int uv_spawn(uv_loop_t* loop, goto error; } - q = uv__process_queue(loop, pid); - QUEUE_INSERT_TAIL(q, &process->queue); + /* Only activate this handle if exec() happened successfully */ + if (exec_errorno == 0) { + q = uv__process_queue(loop, pid); + QUEUE_INSERT_TAIL(q, &process->queue); + uv__handle_start(process); + } process->pid = pid; process->exit_cb = options->exit_cb; - uv__handle_start(process); free(pipes); - return 0; + return exec_errorno; error: if (pipes != NULL) { diff --git a/src/win/process.c b/src/win/process.c index 218ea8f2..a5bb7437 100644 --- a/src/win/process.c +++ b/src/win/process.c @@ -127,7 +127,6 @@ static void uv_process_init(uv_loop_t* loop, uv_process_t* handle) { uv__handle_init(loop, (uv_handle_t*) handle, UV_PROCESS); handle->exit_cb = NULL; handle->pid = 0; - handle->spawn_error = 0; handle->exit_signal = 0; handle->wait_handle = INVALID_HANDLE_VALUE; handle->process_handle = INVALID_HANDLE_VALUE; @@ -752,10 +751,7 @@ void uv_process_proc_exit(uv_loop_t* loop, uv_process_t* handle) { /* callback.*/ uv__handle_stop(handle); - if (handle->spawn_error) { - /* Spawning failed. */ - exit_code = uv_translate_sys_error(handle->spawn_error); - } else if (GetExitCodeProcess(handle->process_handle, &status)) { + if (GetExitCodeProcess(handle->process_handle, &status)) { exit_code = status; } else { /* Unable to to obtain the exit code. This should never happen. */ @@ -1025,25 +1021,20 @@ int uv_spawn(uv_loop_t* loop, free(env); free(path); - process->spawn_error = err; - if (process->child_stdio_buffer != NULL) { /* Clean up child stdio handles. */ uv__stdio_destroy(process->child_stdio_buffer); process->child_stdio_buffer = NULL; } - /* Make the handle active. It will remain active until the exit callback */ - /* is made or the handle is closed, whichever happens first. */ - uv__handle_start(process); - - /* If an error happened, queue the exit req. */ - if (err) { - process->exit_cb_pending = 1; - uv_insert_pending_req(loop, (uv_req_t*) &process->exit_req); + /* Make the handle active, but only if an error didn't happen. It will */ + /* remain active until the exit callback is made or the handle is closed, */ + /* whichever happens first. */ + if (err == 0) { + uv__handle_start(process); } - return 0; + return err; /* This code path is taken when we run into an error that we want to */ /* report immediately. */ diff --git a/test/test-spawn.c b/test/test-spawn.c index 454c1816..0a355af0 100644 --- a/test/test-spawn.c +++ b/test/test-spawn.c @@ -51,7 +51,6 @@ static void close_cb(uv_handle_t* handle) { close_cb_called++; } - static void exit_cb(uv_process_t* process, int64_t exit_status, int term_signal) { @@ -63,29 +62,10 @@ static void exit_cb(uv_process_t* process, } -static void expect(uv_process_t* process, - int64_t exit_status, - int term_signal, - int err) { - printf("exit_cb\n"); - exit_cb_called++; - ASSERT(exit_status == err); - ASSERT(term_signal == 0); - uv_close((uv_handle_t*)process, close_cb); -} - - -static void exit_cb_expect_enoent(uv_process_t* process, - int64_t exit_status, - int term_signal) { - expect(process, exit_status, term_signal, UV_ENOENT); -} - - -static void exit_cb_expect_eperm(uv_process_t* process, - int64_t exit_status, - int term_signal) { - expect(process, exit_status, term_signal, UV_EPERM); +static void fail_cb(uv_process_t* process, + int64_t exit_status, + int term_signal) { + ASSERT(0 && "fail_cb called"); } @@ -166,12 +146,12 @@ static void timer_cb(uv_timer_t* handle, int status) { TEST_IMPL(spawn_fails) { - init_process_options("", exit_cb_expect_enoent); + init_process_options("", fail_cb); options.file = options.args[0] = "program-that-had-better-not-exist"; - ASSERT(0 == uv_spawn(uv_default_loop(), &process, &options)); - ASSERT(1 == uv_is_active((uv_handle_t*) &process)); + + ASSERT(UV_ENOENT == uv_spawn(uv_default_loop(), &process, &options)); + ASSERT(0 == uv_is_active((uv_handle_t*) &process)); ASSERT(0 == uv_run(uv_default_loop(), UV_RUN_DEFAULT)); - ASSERT(1 == exit_cb_called); MAKE_VALGRIND_HAPPY(); return 0; @@ -851,19 +831,18 @@ TEST_IMPL(spawn_setuid_fails) { ASSERT(0 == setuid(pw->pw_uid)); } - init_process_options("spawn_helper1", exit_cb_expect_eperm); + init_process_options("spawn_helper1", fail_cb); options.flags |= UV_PROCESS_SETUID; options.uid = 0; r = uv_spawn(uv_default_loop(), &process, &options); - ASSERT(r == 0); + ASSERT(r == UV_EPERM); r = uv_run(uv_default_loop(), UV_RUN_DEFAULT); ASSERT(r == 0); - ASSERT(exit_cb_called == 1); - ASSERT(close_cb_called == 1); + ASSERT(close_cb_called == 0); MAKE_VALGRIND_HAPPY(); return 0; @@ -883,19 +862,18 @@ TEST_IMPL(spawn_setgid_fails) { ASSERT(0 == setuid(pw->pw_uid)); } - init_process_options("spawn_helper1", exit_cb_expect_eperm); + init_process_options("spawn_helper1", fail_cb); options.flags |= UV_PROCESS_SETGID; options.gid = 0; r = uv_spawn(uv_default_loop(), &process, &options); - ASSERT(r == 0); + ASSERT(r == UV_EPERM); r = uv_run(uv_default_loop(), UV_RUN_DEFAULT); ASSERT(r == 0); - ASSERT(exit_cb_called == 1); - ASSERT(close_cb_called == 1); + ASSERT(close_cb_called == 0); MAKE_VALGRIND_HAPPY(); return 0;