commit 3f0a197aad3cca6634e4eb63e8441e5507a6b77f Author: Steven Murdoch <Steven.Murdoch@cl.cam.ac.uk> Date: Sun Aug 28 23:35:02 2011 +0100 Make signature of tor_spawn_background more conventional Conventionally in Tor, structs are returned as pointers, so change tor_spawn_background() to return the process handle in a pointer rather than as return value. --- src/common/util.c | 94 +++++++++++++++++++++++++++----------------------- src/common/util.h | 4 +- src/test/test_util.c | 9 +++-- 3 files changed, 58 insertions(+), 49 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index d69d8f1..91b83db 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3044,18 +3044,20 @@ format_helper_exit_status(unsigned char child_state, int saved_errno, * The strings in <b>argv</b> will be passed as the command line arguments of * the child program (following convention, argv[0] should normally be the * filename of the executable, and this must be the case if <b>filename</b> is - * NULL). The last element of argv must be NULL. If the child program is - * launched, a handle to it will be returned. + * NULL). The last element of argv must be NULL. A handle to the child process + * will be returned in process_handle (which must be non-NULL). Read + * process_handle.status to find out if the process was successfully launched. + * For convenience, process_handle.status is returned by this function. * * Some parts of this code are based on the POSIX subprocess module from * Python, and example code from * http://msdn.microsoft.com/en-us/library/ms682499%28v=vs.85%29.aspx. */ -process_handle_t -tor_spawn_background(const char *const filename, const char **argv) +int +tor_spawn_background(const char *const filename, const char **argv, + process_handle_t *process_handle) { - process_handle_t process_handle; #ifdef MS_WINDOWS HANDLE stdout_pipe_read = NULL; HANDLE stdout_pipe_write = NULL; @@ -3070,26 +3072,30 @@ tor_spawn_background(const char *const filename, const char **argv) char *joined_argv; int i; + /* process_handle must not be NULL */ + tor_assert(process_handle != NULL); + saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); saAttr.bInheritHandle = TRUE; + /* TODO: should we set explicit security attributes? (#2046, comment 5) */ saAttr.lpSecurityDescriptor = NULL; /* Assume failure to start process */ - memset(&process_handle, 0, sizeof(process_handle)); - process_handle.status = -1; + memset(process_handle, 0, sizeof(process_handle_t)); + process_handle->status = -1; /* Set up pipe for stdout */ if (!CreatePipe(&stdout_pipe_read, &stdout_pipe_write, &saAttr, 0)) { log_warn(LD_GENERAL, "Failed to create pipe for stdout communication with child process: %s", format_win32_error(GetLastError())); - return process_handle; + return process_handle->status; } if (!SetHandleInformation(stdout_pipe_read, HANDLE_FLAG_INHERIT, 0)) { log_warn(LD_GENERAL, "Failed to configure pipe for stdout communication with child " "process: %s", format_win32_error(GetLastError())); - return process_handle; + return process_handle->status; } /* Set up pipe for stderr */ @@ -3097,13 +3103,13 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to create pipe for stderr communication with child process: %s", format_win32_error(GetLastError())); - return process_handle; + return process_handle->status; } if (!SetHandleInformation(stderr_pipe_read, HANDLE_FLAG_INHERIT, 0)) { log_warn(LD_GENERAL, "Failed to configure pipe for stderr communication with child " "process: %s", format_win32_error(GetLastError())); - return process_handle; + return process_handle->status; } /* Create the child process */ @@ -3117,7 +3123,7 @@ tor_spawn_background(const char *const filename, const char **argv) joined_argv = smartlist_join_strings(argv_list, " ", 0, NULL); - ZeroMemory(&process_handle.pid, sizeof(PROCESS_INFORMATION)); + ZeroMemory(&(process_handle->pid), sizeof(PROCESS_INFORMATION)); ZeroMemory(&siStartInfo, sizeof(STARTUPINFO)); siStartInfo.cb = sizeof(STARTUPINFO); siStartInfo.hStdError = stderr_pipe_write; @@ -3128,17 +3134,18 @@ tor_spawn_background(const char *const filename, const char **argv) /* Create the child process */ retval = CreateProcess(filename, // module name - joined_argv, // command line - NULL, // process security attributes - NULL, // primary thread security attributes - TRUE, // handles are inherited + joined_argv, // command line + /* TODO: should we set explicit security attributes? (#2046, comment 5) */ + NULL, // process security attributes + NULL, // primary thread security attributes + TRUE, // handles are inherited /*(TODO: set CREATE_NEW CONSOLE/PROCESS_GROUP to make GetExitCodeProcess() * work?) */ - 0, // creation flags - NULL, // use parent's environment - NULL, // use parent's current directory - &siStartInfo, // STARTUPINFO pointer - &process_handle.pid); // receives PROCESS_INFORMATION + 0, // creation flags + NULL, // use parent's environment + NULL, // use parent's current directory + &siStartInfo, // STARTUPINFO pointer + &(process_handle->pid)); // receives PROCESS_INFORMATION tor_free(joined_argv); @@ -3147,15 +3154,15 @@ tor_spawn_background(const char *const filename, const char **argv) "Failed to create child process %s: %s", filename?filename:argv[0], format_win32_error(GetLastError())); } else { - /* TODO: Close hProcess and hThread in process_handle.pid? */ - process_handle.stdout_pipe = stdout_pipe_read; - process_handle.stderr_pipe = stderr_pipe_read; - process_handle.status = 1; + /* TODO: Close hProcess and hThread in process_handle->pid? */ + process_handle->stdout_pipe = stdout_pipe_read; + process_handle->stderr_pipe = stderr_pipe_read; + process_handle->status = 1; } /* TODO: Close pipes on exit */ - return process_handle; + return process_handle->status; #else // MS_WINDOWS pid_t pid; int stdout_pipe[2]; @@ -3175,8 +3182,8 @@ tor_spawn_background(const char *const filename, const char **argv) static int max_fd = -1; /* Assume failure to start */ - memset(&process_handle, 0, sizeof(process_handle)); - process_handle.status = -1; + memset(process_handle, 0, sizeof(process_handle_t)); + process_handle->status = -1; /* We do the strlen here because strlen() is not signal handler safe, and we are not allowed to use unsafe functions between fork and exec */ @@ -3190,7 +3197,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to set up pipe for stdout communication with child process: %s", strerror(errno)); - return process_handle; + return process_handle->status; } retval = pipe(stderr_pipe); @@ -3198,7 +3205,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to set up pipe for stderr communication with child process: %s", strerror(errno)); - return process_handle; + return process_handle->status; } child_state = CHILD_STATE_MAXFD; @@ -3284,7 +3291,8 @@ tor_spawn_background(const char *const filename, const char **argv) (void) nbytes; _exit(255); - return process_handle; /* Never reached, but avoids compiler warning */ + /* Never reached, but avoids compiler warning */ + return process_handle->status; } /* In parent */ @@ -3295,15 +3303,15 @@ tor_spawn_background(const char *const filename, const char **argv) close(stdout_pipe[1]); close(stderr_pipe[0]); close(stderr_pipe[1]); - return process_handle; + return process_handle->status; } - process_handle.pid = pid; + process_handle->pid = pid; /* TODO: If the child process forked but failed to exec, waitpid it */ /* Return read end of the pipes to caller, and close write end */ - process_handle.stdout_pipe = stdout_pipe[0]; + process_handle->stdout_pipe = stdout_pipe[0]; retval = close(stdout_pipe[1]); if (-1 == retval) { @@ -3312,7 +3320,7 @@ tor_spawn_background(const char *const filename, const char **argv) strerror(errno)); } - process_handle.stderr_pipe = stderr_pipe[0]; + process_handle->stderr_pipe = stderr_pipe[0]; retval = close(stderr_pipe[1]); if (-1 == retval) { @@ -3321,15 +3329,15 @@ tor_spawn_background(const char *const filename, const char **argv) strerror(errno)); } - process_handle.status = 1; + process_handle->status = 1; /* Set stdout/stderr pipes to be non-blocking */ - fcntl(process_handle.stdout_pipe, F_SETFL, O_NONBLOCK); - fcntl(process_handle.stderr_pipe, F_SETFL, O_NONBLOCK); + fcntl(process_handle->stdout_pipe, F_SETFL, O_NONBLOCK); + fcntl(process_handle->stderr_pipe, F_SETFL, O_NONBLOCK); /* Open the buffered IO streams */ - process_handle.stdout_handle = fdopen(process_handle.stdout_pipe, "r"); - process_handle.stderr_handle = fdopen(process_handle.stderr_pipe, "r"); + process_handle->stdout_handle = fdopen(process_handle->stdout_pipe, "r"); + process_handle->stderr_handle = fdopen(process_handle->stderr_pipe, "r"); - return process_handle; + return process_handle->status; #endif // MS_WINDOWS } @@ -3676,9 +3684,9 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, #ifdef MS_WINDOWS /* Passing NULL as lpApplicationName makes Windows search for the .exe */ - child_handle = tor_spawn_background(NULL, argv); + tor_spawn_background(NULL, argv, &child_handle); #else - child_handle = tor_spawn_background(filename, argv); + tor_spawn_background(filename, argv, &child_handle); #endif if (child_handle.status < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", diff --git a/src/common/util.h b/src/common/util.h index 442001f..e025265 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -370,8 +370,8 @@ typedef struct process_handle_s { #endif // MS_WINDOWS } process_handle_t; -process_handle_t tor_spawn_background(const char *const filename, - const char **argv); +int tor_spawn_background(const char *const filename, const char **argv, + process_handle_t *process_handle); int tor_get_exit_code(const process_handle_t process_handle, int block, int *exit_code); #ifdef MS_WINDOWS diff --git a/src/test/test_util.c b/src/test/test_util.c index cb98030..40ab098 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1389,9 +1389,9 @@ run_util_spawn_background(const char *argv[], const char *expected_out, /* Start the program */ #ifdef MS_WINDOWS - process_handle = tor_spawn_background(NULL, argv); + tor_spawn_background(NULL, argv, &process_handle); #else - process_handle = tor_spawn_background(argv[0], argv); + tor_spawn_background(argv[0], argv, &process_handle); #endif tt_int_op(process_handle.status, ==, expected_status); @@ -1473,7 +1473,8 @@ test_util_spawn_background_fail(void *ptr) expected_status); } -/** Helper function for testing tor_spawn_background */ +/** Test that reading from a handle returns a partial read rather than + * blocking */ static void test_util_spawn_background_partial_read(void *ptr) { @@ -1499,7 +1500,7 @@ test_util_spawn_background_partial_read(void *ptr) (void)ptr; /* Start the program */ - process_handle = tor_spawn_background(NULL, argv); + tor_spawn_background(NULL, argv, &process_handle); tt_int_op(process_handle.status, ==, expected_status); /* Check stdout */