August 2011 - tor-commits - lists.torproject.org

[tor/master] Appease "make check-spaces"
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 1da5081ae0cfaf52f11c0d93c42efa211ead35af Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Wed Aug 24 21:34:13 2011 +0100 Appease "make check-spaces" --- src/common/util.c | 83 ++++++++++++++++++++++++++----------------------- src/common/util.h | 3 +- src/test/test_util.c | 9 +++-- 3 files changed, 51 insertions(+), 44 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index c184583..d69d8f1 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3063,16 +3063,16 @@ tor_spawn_background(const char *const filename, const char **argv) HANDLE stderr_pipe_write = NULL; STARTUPINFO siStartInfo; - BOOL retval = FALSE; - + BOOL retval = FALSE; + SECURITY_ATTRIBUTES saAttr; smartlist_t *argv_list; char *joined_argv; int i; - - saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); - saAttr.bInheritHandle = TRUE; - saAttr.lpSecurityDescriptor = NULL; + + saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); + saAttr.bInheritHandle = TRUE; + saAttr.lpSecurityDescriptor = NULL; /* Assume failure to start process */ memset(&process_handle, 0, sizeof(process_handle)); @@ -3087,8 +3087,8 @@ tor_spawn_background(const char *const filename, const char **argv) } 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())); + "Failed to configure pipe for stdout communication with child " + "process: %s", format_win32_error(GetLastError())); return process_handle; } @@ -3101,14 +3101,15 @@ tor_spawn_background(const char *const filename, const char **argv) } 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())); + "Failed to configure pipe for stderr communication with child " + "process: %s", format_win32_error(GetLastError())); return process_handle; } /* Create the child process */ - /* Windows expects argv to be a whitespace delimited string, so join argv up */ + /* Windows expects argv to be a whitespace delimited string, so join argv up + */ argv_list = smartlist_create(); for (i=0; argv[i] != NULL; i++) { smartlist_add(argv_list, (void *)argv[i]); @@ -3118,39 +3119,41 @@ tor_spawn_background(const char *const filename, const char **argv) ZeroMemory(&process_handle.pid, sizeof(PROCESS_INFORMATION)); ZeroMemory(&siStartInfo, sizeof(STARTUPINFO)); - siStartInfo.cb = sizeof(STARTUPINFO); + siStartInfo.cb = sizeof(STARTUPINFO); siStartInfo.hStdError = stderr_pipe_write; siStartInfo.hStdOutput = stdout_pipe_write; siStartInfo.hStdInput = NULL; siStartInfo.dwFlags |= STARTF_USESTDHANDLES; - + /* 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 - 0, // creation flags (TODO: set CREATE_NEW CONSOLE/PROCESS_GROUP to make GetExitCodeProcess() work?) - NULL, // use parent's environment - NULL, // use parent's current directory - &siStartInfo, // STARTUPINFO pointer + joined_argv, // command line + 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 - tor_free(joined_argv); - + tor_free(joined_argv); + if (!retval) { log_warn(LD_GENERAL, "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? + /* 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 + /* TODO: Close pipes on exit */ return process_handle; #else // MS_WINDOWS @@ -3353,7 +3356,7 @@ tor_get_exit_code(const process_handle_t process_handle, retval = WaitForSingleObject(process_handle.pid.hProcess, INFINITE); if (retval != WAIT_OBJECT_0) { log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", - (int)retval, format_win32_error(GetLastError())); + (int)retval, format_win32_error(GetLastError())); return -1; } } else { @@ -3367,7 +3370,7 @@ tor_get_exit_code(const process_handle_t process_handle, return -1; } } - + if (exit_code != NULL) { success = GetExitCodeProcess(process_handle.pid.hProcess, (PDWORD)exit_code); @@ -3392,7 +3395,8 @@ tor_get_exit_code(const process_handle_t process_handle, } if (!WIFEXITED(stat_loc)) { - log_warn(LD_GENERAL, "Process %d did not exit normally", process_handle.pid); + log_warn(LD_GENERAL, "Process %d did not exit normally", + process_handle.pid); return -1; } @@ -3478,7 +3482,7 @@ tor_read_all_from_process_stdout(const process_handle_t process_handle, return read_all(process_handle.stdout_pipe, buf, count, 0); #endif } - + /* Read from stdout of a process until the process exits. */ ssize_t tor_read_all_from_process_stderr(const process_handle_t process_handle, @@ -3529,14 +3533,14 @@ log_from_handle(HANDLE *pipe, int severity) /* On Windows \r means end of line */ if ('\r' == buf[cur]) { buf[cur] = '\0'; - next = cur + 1; - /* If \n follows, remove it too */ + next = cur + 1; + /* If \n follows, remove it too */ if ((cur + 1) < pos && '\n' == buf[cur+1]) { - buf[cur + 1] = '\0'; + buf[cur + 1] = '\0'; next = cur + 2; - } - /* Line starts at start and ends with a null (was \r\n) */ - break; + } + /* Line starts at start and ends with a null (was \r\n) */ + break; } /* Line starts at start and ends at the end of a string but we already added a null in earlier */ @@ -3544,7 +3548,7 @@ log_from_handle(HANDLE *pipe, int severity) log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start); } return 0; -} +} #else /** Read from stream, and send lines to log at the specified log level. @@ -3630,7 +3634,8 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, { /* When fw-helper succeeds, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_SUCCESS 300 -/* When fw-helper failed to start, how long do we wait until running it again */ +/* When fw-helper failed to start, how long do we wait until running it again + */ #define TIME_TO_EXEC_FWHELPER_FAIL 60 /* Static variables are initialized to zero, so child_handle.status=0 @@ -3702,9 +3707,9 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, retval = 0; #else stdout_status = log_from_pipe(child_handle.stdout_handle, - LOG_INFO, filename, &retval); + LOG_INFO, filename, &retval); stderr_status = log_from_pipe(child_handle.stderr_handle, - LOG_WARN, filename, &retval); + LOG_WARN, filename, &retval); #endif if (retval) { /* There was a problem in the child process */ diff --git a/src/common/util.h b/src/common/util.h index 2602628..442001f 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -375,7 +375,8 @@ process_handle_t tor_spawn_background(const char *const filename, int tor_get_exit_code(const process_handle_t process_handle, int block, int *exit_code); #ifdef MS_WINDOWS -ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess); +ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, + HANDLE hProcess); #endif ssize_t tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count); diff --git a/src/test/test_util.c b/src/test/test_util.c index 5d3f95c..cb98030 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1395,7 +1395,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, #endif tt_int_op(process_handle.status, ==, expected_status); - + /* If the process failed to start, don't bother continuing */ if (process_handle.status == -1) return; @@ -1405,7 +1405,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, /* Check stdout */ pos = tor_read_all_from_process_stdout(process_handle, stdout_buf, - sizeof(stdout_buf) - 1); + sizeof(stdout_buf) - 1); tt_assert(pos >= 0); stdout_buf[pos] = '\0'; tt_str_op(stdout_buf, ==, expected_out); @@ -1419,7 +1419,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, /* Check stderr */ pos = tor_read_all_from_process_stderr(process_handle, stderr_buf, - sizeof(stderr_buf) - 1); + sizeof(stderr_buf) - 1); tt_assert(pos >= 0); stderr_buf[pos] = '\0'; tt_str_op(stderr_buf, ==, expected_err); @@ -1469,7 +1469,8 @@ test_util_spawn_background_fail(void *ptr) (void)ptr; - run_util_spawn_background(argv, expected_out, expected_err, 255, expected_status); + run_util_spawn_background(argv, expected_out, expected_err, 255, + expected_status); } /** Helper function for testing tor_spawn_background */

1 0

[tor/master] Improve comments and fix one bug
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 50b48c3ea7b9601c1ab29f786bb0d88eb4149474 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Wed Aug 24 21:33:53 2011 +0100 Improve comments and fix one bug --- src/common/util.c | 70 +++++++++++++++++++++++++++++++---------------------- src/common/util.h | 2 +- 2 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 8b9979c..c184583 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3034,20 +3034,24 @@ format_helper_exit_status(unsigned char child_state, int saved_errno, #define SPAWN_ERROR_MESSAGE "ERR: Failed to spawn background process - code " -/** Start a program in the background. If filename contains a '/', - * then it will be treated as an absolute or relative path. Otherwise the - * system path will be searched for filename. The strings in - * argv will be passed as the command line arguments of the child - * program (following convention, argv[0] should normally be the filename of - * the executable). The last element of argv must be NULL. If the child - * program is launched, the PID will be returned and stdout_read and - * stdout_err will be set to file descriptors from which the stdout - * and stderr, respectively, output of the child program can be read, and the - * stdin of the child process shall be set to /dev/null. Otherwise returns - * -1. Some parts of this code are based on the POSIX subprocess module from +/** Start a program in the background. If filename contains a '/', then + * it will be treated as an absolute or relative path. Otherwise, on + * non-Windows systems, the system path will be searched for filename. + * On Windows, only the current directory will be searched. Here, to search the + * system path (as well as the application directory, current working + * directory, and system directories), set filename to NULL. + * + * The strings in argv 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 filename is + * NULL). The last element of argv must be NULL. If the child program is + * launched, a handle to it will be returned. + * + * 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) { @@ -3122,8 +3126,8 @@ tor_spawn_background(const char *const filename, const char **argv) /* Create the child process */ - retval = CreateProcess(filename, // module name - joined_argv, // command line + retval = CreateProcess(filename, // module name + joined_argv, // command line NULL, // process security attributes NULL, // primary thread security attributes TRUE, // handles are inherited @@ -3270,7 +3274,7 @@ tor_spawn_background(const char *const filename, const char **argv) /* Write the error message. GCC requires that we check the return value, but there is nothing we can do if it fails */ - // TODO: Don't use STDOUT, use a pipe set up just for this purpose + /* TODO: Don't use STDOUT, use a pipe set up just for this purpose */ nbytes = write(STDOUT_FILENO, error_message, error_message_length); nbytes = write(STDOUT_FILENO, hex_errno, sizeof(hex_errno)); @@ -3291,7 +3295,9 @@ tor_spawn_background(const char *const filename, const char **argv) return process_handle; } - // TODO: If the child process forked but failed to exec, waitpid it + 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]; @@ -3301,8 +3307,6 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to close write end of stdout pipe in parent process: %s", strerror(errno)); - /* Do not return -1, because the child is running, so the parent - needs to know about the pid in order to reap it later */ } process_handle.stderr_pipe = stderr_pipe[0]; @@ -3312,12 +3316,9 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to close write end of stderr pipe in parent process: %s", strerror(errno)); - /* Do not return -1, because the child is running, so the parent - needs to know about the pid in order to reap it later */ } process_handle.status = 1; - process_handle.pid = pid; /* 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); @@ -3403,7 +3404,12 @@ tor_get_exit_code(const process_handle_t process_handle, } #ifdef MS_WINDOWS -/* Windows equivalent of read_all */ +/** Read from a handle h into buf, up to count bytes. If + * hProcess is NULL, the function will return immediately if there is + * nothing more to read. Otherwise hProcess should be set to the handle + * to the process owning the h. In this case, the function will exit + * only once the process has exited, or count bytes are read. Returns + * the number of bytes read, or -1 on error. */ ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) { @@ -3416,6 +3422,7 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) return -1; while (numread != count) { + /* Check if there is anything to read */ retval = PeekNamedPipe(h, NULL, 0, NULL, &byte_count, NULL); if (!retval) { log_warn(LD_GENERAL, @@ -3459,6 +3466,7 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) } #endif +/* Read from stdout of a process until the process exits. */ ssize_t tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count) @@ -3471,6 +3479,7 @@ tor_read_all_from_process_stdout(const process_handle_t process_handle, #endif } +/* Read from stdout of a process until the process exits. */ ssize_t tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count) @@ -3496,38 +3505,41 @@ log_from_handle(HANDLE *pipe, int severity) pos = tor_read_all_handle(pipe, buf, sizeof(buf) - 1, NULL); if (pos < 0) { - // Error + /* Error */ log_warn(LD_GENERAL, "Failed to read data from subprocess"); return -1; } if (0 == pos) { - // There's nothing to read (process is busy or has exited) + /* There's nothing to read (process is busy or has exited) */ log_debug(LD_GENERAL, "Subprocess had nothing to say"); return 0; } - // End with a null even if there isn't a \r\n at the end - // TODO: What if this is a partial line? + /* End with a null even if there isn't a \r\n at the end */ + /* TODO: What if this is a partial line? */ buf[pos] = '\0'; log_debug(LD_GENERAL, "Subprocess had %d bytes to say", pos); + /* Split buf into lines and log each one */ next = 0; // Start of the next line while (next < pos) { start = next; // Look for the end of this line for (cur=start; cur<pos; cur++) { + /* On Windows \r means end of line */ if ('\r' == buf[cur]) { buf[cur] = '\0'; next = cur + 1; - if ((cur + 1) < pos && buf[cur+1] < pos) { + /* If \n follows, remove it too */ + if ((cur + 1) < pos && '\n' == buf[cur+1]) { buf[cur + 1] = '\0'; next = cur + 2; } - // Line starts at start and ends with a null (was \r\n) + /* Line starts at start and ends with a null (was \r\n) */ break; } - // Line starts at start and ends at the end of a string - // but we already added a null in earlier + /* Line starts at start and ends at the end of a string + but we already added a null in earlier */ } log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start); } diff --git a/src/common/util.h b/src/common/util.h index b4ae3f8..2602628 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -366,7 +366,7 @@ typedef struct process_handle_s { int stderr_pipe; FILE *stdout_handle; FILE *stderr_handle; - int pid; + pid_t pid; #endif // MS_WINDOWS } process_handle_t;

1 0

[tor/master] Make signature of tor_spawn_background more conventional
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 3f0a197aad3cca6634e4eb63e8441e5507a6b77f Author: Steven Murdoch <Steven.Murdoch(a)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 argv 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 filename 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 */

1 0

[tor/master] Add a sanity check
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 93792b5aa6806646674795504419f3e97862685c Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Mon Aug 29 00:36:41 2011 +0100 Add a sanity check --- src/common/util.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 371e09b..dd2d111 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3462,10 +3462,11 @@ tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) continue; } + /* There is data to read; read it */ retval = ReadFile(h, buf+numread, count-numread, &byte_count, NULL); + tor_assert(byte_count + numread <= count); if (!retval) { - log_warn(LD_GENERAL, - "Failed to read from handle: %s", + log_warn(LD_GENERAL, "Failed to read from handle: %s", format_win32_error(GetLastError())); return -1; } else if (0 == byte_count) {

1 0

[tor/master] We don't need to find our own path, just tell Windows to search
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 476807211c30491a92344b0222274aefbc5b5e8a Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Wed Aug 24 20:50:58 2011 +0100 We don't need to find our own path, just tell Windows to search --- src/common/util.c | 7 ++++- src/test/test_util.c | 68 +++++-------------------------------------------- 2 files changed, 13 insertions(+), 62 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index f25dd43..8b9979c 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3137,7 +3137,7 @@ tor_spawn_background(const char *const filename, const char **argv) if (!retval) { log_warn(LD_GENERAL, - "Failed to create child process %s: %s", filename, + "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? @@ -3657,7 +3657,12 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, /* Assume tor-fw-helper will succeed, start it later*/ time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS; +#ifdef MS_WINDOWS + /* Passing NULL as lpApplicationName makes Windows search for the .exe */ + child_handle = tor_spawn_background(NULL, argv); +#else child_handle = tor_spawn_background(filename, argv); +#endif if (child_handle.status < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename); diff --git a/src/test/test_util.c b/src/test/test_util.c index d30cc11..5d3f95c 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1388,7 +1388,11 @@ run_util_spawn_background(const char *argv[], const char *expected_out, char stdout_buf[100], stderr_buf[100]; /* Start the program */ +#ifdef MS_WINDOWS + process_handle = tor_spawn_background(NULL, argv); +#else process_handle = tor_spawn_background(argv[0], argv); +#endif tt_int_op(process_handle.status, ==, expected_status); @@ -1425,55 +1429,14 @@ run_util_spawn_background(const char *argv[], const char *expected_out, ; } -static char* -get_windows_path(const char *process_name, const char **dest) -{ -#ifdef MS_WINDOWS - char fn[MAX_PATH]; - DWORD retval; - char *new_fn = NULL; - int i; - - /* Get the file name of the current module */ - retval = GetModuleFileName(NULL, fn, sizeof(fn)); - if (retval >= sizeof(fn)) { - log_warn(LD_GENERAL, "Executable path name was longer than maximum (%d)", sizeof(fn)); - return NULL; - } - - /* Remove the filename component */ - for (i = retval - 1; i >= 0; i--) { - /* \0 terminate at first path separator from end */ - if ('\\' == fn[i] || '/' == fn[i]) { - fn[i] = '\0'; - break; - } - } - - tor_asprintf(&new_fn, "%s\\%s", fn, process_name); - *dest = new_fn; - return new_fn; -#else - (void)process_name; - (void)dest; - return NULL; -#endif -} - /** Check that we can launch a process and read the output */ static void test_util_spawn_background_ok(void *ptr) { #ifdef MS_WINDOWS - const char *argv[] = {NULL, "--test", NULL}; + const char *argv[] = {"test-child.exe", "--test", NULL}; const char *expected_out = "OUT\r\n--test\r\nSLEEPING\r\nDONE\r\n"; const char *expected_err = "ERR\r\n"; - char *filename; - - /* Find path to test-child.exe (same directory as this executable */ - filename = get_windows_path("test-child.exe", argv); - tt_assert(filename != NULL); - log_warn(LD_GENERAL, "Using %s as path", filename); #else const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; const char *expected_out = "OUT\n--test\nSLEEPING\nDONE\n"; @@ -1483,12 +1446,6 @@ test_util_spawn_background_ok(void *ptr) (void)ptr; run_util_spawn_background(argv, expected_out, expected_err, 0, 1); - done: -#ifdef MS_WINDOWS - tor_free(filename); -#else - ; -#endif } /** Check that failing to find the executable works as expected */ @@ -1527,18 +1484,12 @@ test_util_spawn_background_partial_read(void *ptr) process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; #ifdef MS_WINDOWS - const char *argv[] = {NULL, "--test", NULL}; + const char *argv[] = {"test-child.exe", "--test", NULL}; const char *expected_out[] = { "OUT\r\n--test\r\nSLEEPING\r\n", "DONE\r\n", NULL }; const char *expected_err = "ERR\r\n"; int expected_out_ctr; - char *filename; - - /* Find path to test-child.exe (same directory as this executable */ - filename = get_windows_path("test-child.exe", argv); - tt_assert(filename != NULL); - log_warn(LD_GENERAL, "Using %s as path", filename); #else const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; const char *expected_out = "OUT\n--test\nSLEEPING\nDONE\n"; @@ -1547,9 +1498,8 @@ test_util_spawn_background_partial_read(void *ptr) (void)ptr; /* Start the program */ - process_handle = tor_spawn_background(argv[0], argv); + process_handle = tor_spawn_background(NULL, argv); tt_int_op(process_handle.status, ==, expected_status); - tor_free(filename); /* Check stdout */ #ifdef MS_WINDOWS @@ -1597,11 +1547,7 @@ test_util_spawn_background_partial_read(void *ptr) tt_int_op(pos, ==, strlen(expected_err)); done: -#ifdef MS_WINDOWS - tor_free(filename); -#else ; -#endif } static void

1 0

[tor/master] Refactor out command line formatting
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit bc97f410802d5b9c66bfba6aebeae1ecd70f8857 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Mon Aug 29 12:26:55 2011 +0100 Refactor out command line formatting Now correctly handles whitespace, quotes and backslashes. Passes all unit tests. --- src/common/util.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++---- src/common/util.h | 1 + src/test/test_util.c | 49 ++++++++++++++++++++++ 3 files changed, 150 insertions(+), 8 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index dd2d111..87c6fb5 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2953,6 +2953,105 @@ load_windows_system_library(const TCHAR *library_name) } #endif +/* Format a single argument for being put on a Windows command line. + * Returns a newly allocated string */ +static char * +format_cmdline_argument(const char *arg) +{ + char *formatted_arg; + char need_quotes; + const char *c; + int i; + int bs_counter = 0; + /* Backslash we can point to when one is inserted into the string */ + const char backslash = '\\'; + + /* Smartlist of *char */ + smartlist_t *arg_chars; + arg_chars = smartlist_create(); + + /* Quote string if it contains whitespace or is empty */ + need_quotes = (strchr(arg, ' ') || strchr(arg, '\t') || '\0' == arg[0]); + + /* Build up smartlist of *chars */ + for (c=arg; *c != '\0'; c++) { + if ('"' == *c) { + /* Double up backslashes preceding a quote */ + for (i=0; i<(bs_counter*2); i++) + smartlist_add(arg_chars, (void*)&backslash); + bs_counter = 0; + /* Escape the quote */ + smartlist_add(arg_chars, (void*)&backslash); + smartlist_add(arg_chars, (void*)c); + } else if ('\\' == *c) { + /* Count backslashes until we know whether to double up */ + bs_counter++; + } else { + /* Don't double up slashes preceding a non-quote */ + for (i=0; i<bs_counter; i++) + smartlist_add(arg_chars, (void*)&backslash); + bs_counter = 0; + smartlist_add(arg_chars, (void*)c); + } + } + /* Don't double up trailing backslashes */ + for (i=0; i<bs_counter; i++) + smartlist_add(arg_chars, (void*)&backslash); + + /* Allocate space for argument, quotes (if needed), and terminator */ + formatted_arg = tor_malloc(sizeof(char) * + (smartlist_len(arg_chars) + (need_quotes?2:0) + 1)); + + /* Add leading quote */ + i=0; + if (need_quotes) + formatted_arg[i++] = '"'; + + /* Add characters */ + SMARTLIST_FOREACH(arg_chars, char*, c, + { + formatted_arg[i++] = *c; + }); + + /* Add trailing quote */ + if (need_quotes) + formatted_arg[i++] = '"'; + formatted_arg[i] = '\0'; + + smartlist_free(arg_chars); + return formatted_arg; +} + +/* Format a command line for use on Windows, which takes the command as a + * string rather than string array. Follows the rules from "Parsing C++ + * Command-Line Arguments" in MSDN. Algorithm based on list2cmdline in the + * Python subprocess module. Returns a newly allocated string */ +char * +tor_join_cmdline(const char *argv[]) +{ + smartlist_t *argv_list; + char *joined_argv; + int i; + + /* Format each argument and put the result in a smartlist */ + argv_list = smartlist_create(); + for (i=0; argv[i] != NULL; i++) { + smartlist_add(argv_list, (void *)format_cmdline_argument(argv[i])); + } + + /* Join the arguments with whitespace */ + joined_argv = smartlist_join_strings(argv_list, " ", 0, NULL); + + /* Free the newly allocated arguments, and the smartlist */ + SMARTLIST_FOREACH(argv_list, char *, arg, + { + tor_free(arg); + }); + smartlist_free(argv_list); + + return joined_argv; +} + /** Format child_state and saved_errno as a hex string placed in * hex_errno. Called between fork and _exit, so must be signal-handler * safe. @@ -3068,9 +3167,7 @@ tor_spawn_background(const char *const filename, const char **argv, BOOL retval = FALSE; SECURITY_ATTRIBUTES saAttr; - smartlist_t *argv_list; char *joined_argv; - int i; /* process_handle must not be NULL */ tor_assert(process_handle != NULL); @@ -3116,12 +3213,7 @@ tor_spawn_background(const char *const filename, const char **argv, /* Windows expects argv to be a whitespace delimited string, so join argv up */ - argv_list = smartlist_create(); - for (i=0; argv[i] != NULL; i++) { - smartlist_add(argv_list, (void *)argv[i]); - } - - joined_argv = smartlist_join_strings(argv_list, " ", 0, NULL); + joined_argv = tor_join_cmdline(argv); ZeroMemory(&(process_handle->pid), sizeof(PROCESS_INFORMATION)); ZeroMemory(&siStartInfo, sizeof(STARTUPINFO)); diff --git a/src/common/util.h b/src/common/util.h index d8c7370..9cdd8cb 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -393,6 +393,7 @@ ssize_t tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count); ssize_t tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count); +char *tor_join_cmdline(const char *argv[]); void format_helper_exit_status(unsigned char child_state, int saved_errno, char *hex_errno); diff --git a/src/test/test_util.c b/src/test/test_util.c index 4568fde..9df7bc6 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1555,6 +1555,54 @@ test_util_spawn_background_partial_read(void *ptr) } static void +test_util_join_cmdline(void *ptr) +{ + /* Based on some test cases from "Parsing C++ Command-Line Arguments" in MSDN + * but we don't exercise all quoting rules because tor_join_cmdline will try + * to only generate simple cases for the child process to parse; i.e. we + * never embed quoted strings in arguments. */ + + const char *argvs[][4] = { + {"a", "bb", "CCC", NULL}, // Normal + {NULL, NULL, NULL, NULL}, // Empty argument list + {"", NULL, NULL, NULL}, // Empty argument + {"\"a", "b\"b", "CCC\"", NULL}, // Quotes + {"a\tbc", "dd dd", "E", NULL}, // Whitespace + {"a\\\\\\b", "de fg", "H", NULL}, // Backslashes + {"a\\\"b", "\\c", "D\\", NULL}, // Backslashes before quote + {"a\\\\b c", "d", "E", NULL}, // Backslashes not before quote + {} // Terminator + }; + + const char *cmdlines[] = { + "a bb CCC", + "", + "\"\"", + "\\\"a b\\\"b CCC\\\"", + "\"a\tbc\" \"dd dd\" E", + "a\\\\\\b \"de fg\" H", + "a\\\\\\\"b \\c D\\", + "\"a\\\\b c\" d E", + NULL // Terminator + }; + + int i; + char *joined_argv; + + (void)ptr; + + for (i=0; cmdlines[i]!=NULL; i++) { + log_info(LD_GENERAL, "Joining argvs[%d], expecting <%s>", i, cmdlines[i]); + joined_argv = tor_join_cmdline(argvs[i]); + tt_str_op(joined_argv, ==, cmdlines[i]); + tor_free(joined_argv); + } + + done: + ; +} + +static void test_util_di_ops(void) { #define LT -1 @@ -1642,6 +1690,7 @@ struct testcase_t util_tests[] = { UTIL_TEST(spawn_background_ok, 0), UTIL_TEST(spawn_background_fail, 0), UTIL_TEST(spawn_background_partial_read, 0), + UTIL_TEST(join_cmdline, 0), END_OF_TESTCASES };

1 0

[tor/master] Replace two magic tristates with #define'd names
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit f1ff65dfad800ed89e5b01c1c5b4b77c10a438b8 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Mon Aug 29 00:30:18 2011 +0100 Replace two magic tristates with #define'd names - process_handle_t.status - return value of tor_get_exit_code() --- src/common/util.c | 64 ++++++++++++++++++++++++------------------------- src/common/util.h | 13 +++++++++- src/test/test_util.c | 18 ++++++++------ 3 files changed, 53 insertions(+), 42 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 91b83db..371e09b 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3082,7 +3082,7 @@ tor_spawn_background(const char *const filename, const char **argv, /* Assume failure to start process */ memset(process_handle, 0, sizeof(process_handle_t)); - process_handle->status = -1; + process_handle->status = PROCESS_STATUS_ERROR; /* Set up pipe for stdout */ if (!CreatePipe(&stdout_pipe_read, &stdout_pipe_write, &saAttr, 0)) { @@ -3157,7 +3157,7 @@ tor_spawn_background(const char *const filename, const char **argv, /* 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; + process_handle->status = PROCESS_STATUS_RUNNING; } /* TODO: Close pipes on exit */ @@ -3183,7 +3183,7 @@ tor_spawn_background(const char *const filename, const char **argv, /* Assume failure to start */ memset(process_handle, 0, sizeof(process_handle_t)); - process_handle->status = -1; + process_handle->status = PROCESS_STATUS_ERROR; /* 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 */ @@ -3329,7 +3329,7 @@ tor_spawn_background(const char *const filename, const char **argv, strerror(errno)); } - process_handle->status = 1; + process_handle->status = PROCESS_STATUS_RUNNING; /* 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); @@ -3341,16 +3341,16 @@ tor_spawn_background(const char *const filename, const char **argv, #endif // MS_WINDOWS } -/* Get the exit code of a process specified by process_handle and - * store it in exit_code, if set to a non-NULL value. If - * block is set to true, the call will block until the process has - * exited. Otherwise if the process is still running, the function will - * return -2, and exit_code will be left unchanged. Returns 0 if the - * process did exit. If there is a failure, -1 will be returned and the - * contents of exit_code (if non-NULL) will be undefined. N.B. Under *nix - * operating systems, this will probably not work in Tor, because - * waitpid() is called in main.c to reap any terminated child - * processes.*/ +/* Get the exit code of a process specified by process_handle and store + * it in exit_code, if set to a non-NULL value. If block is set + * to true, the call will block until the process has exited. Otherwise if + * the process is still running, the function will return + * PROCESS_EXIT_RUNNING, and exit_code will be left unchanged. Returns + * PROCESS_EXIT_EXITED if the process did exit. If there is a failure, + * PROCESS_EXIT_ERROR will be returned and the contents of exit_code (if + * non-NULL) will be undefined. N.B. Under *nix operating systems, this will + * probably not work in Tor, because waitpid() is called in main.c to reap any + * terminated child processes.*/ int tor_get_exit_code(const process_handle_t process_handle, int block, int *exit_code) @@ -3365,17 +3365,17 @@ tor_get_exit_code(const process_handle_t process_handle, if (retval != WAIT_OBJECT_0) { log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", (int)retval, format_win32_error(GetLastError())); - return -1; + return PROCESS_EXIT_ERROR; } } else { retval = WaitForSingleObject(process_handle.pid.hProcess, 0); if (WAIT_TIMEOUT == retval) { /* Process has not exited */ - return -2; + return PROCESS_EXIT_RUNNING; } else if (retval != WAIT_OBJECT_0) { log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", (int)retval, format_win32_error(GetLastError())); - return -1; + return PROCESS_EXIT_ERROR; } } @@ -3385,7 +3385,7 @@ tor_get_exit_code(const process_handle_t process_handle, if (!success) { log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", format_win32_error(GetLastError())); - return -1; + return PROCESS_EXIT_ERROR; } } #else @@ -3395,24 +3395,24 @@ tor_get_exit_code(const process_handle_t process_handle, retval = waitpid(process_handle.pid, &stat_loc, block?0:WNOHANG); if (!block && 0 == retval) { /* Process has not exited */ - return -2; + return PROCESS_EXIT_RUNNING; } else if (retval != process_handle.pid) { log_warn(LD_GENERAL, "waitpid() failed for PID %d: %s", process_handle.pid, strerror(errno)); - return -1; + return PROCESS_EXIT_ERROR; } if (!WIFEXITED(stat_loc)) { log_warn(LD_GENERAL, "Process %d did not exit normally", process_handle.pid); - return -1; + return PROCESS_EXIT_ERROR; } if (exit_code != NULL) *exit_code = WEXITSTATUS(stat_loc); #endif // MS_WINDOWS - return 0; + return PROCESS_EXIT_EXITED; } #ifdef MS_WINDOWS @@ -3648,11 +3648,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, /* Static variables are initialized to zero, so child_handle.status=0 * which corresponds to it not running on startup */ -#ifdef MS_WINDOWS static process_handle_t child_handle; -#else - static process_handle_t child_handle; -#endif static time_t time_to_run_helper = 0; int stdout_status, stderr_status, retval; @@ -3678,7 +3674,8 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, argv[9] = NULL; /* Start the child, if it is not already running */ - if (child_handle.status <= 0 && time_to_run_helper < now) { + if (child_handle.status != PROCESS_STATUS_RUNNING && + time_to_run_helper < now) { /* Assume tor-fw-helper will succeed, start it later*/ time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS; @@ -3688,7 +3685,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, #else tor_spawn_background(filename, argv, &child_handle); #endif - if (child_handle.status < 0) { + if (PROCESS_STATUS_ERROR == child_handle.status) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename); time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL; @@ -3705,7 +3702,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, } /* If child is running, read from its stdout and stderr) */ - if (child_handle.status > 0) { + if (PROCESS_STATUS_RUNNING == child_handle.status) { /* Read from stdout/stderr and log result */ retval = 0; #ifdef MS_WINDOWS @@ -3729,8 +3726,9 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, /* There was a failure */ retval = -1; #ifdef MS_WINDOWS - else if (tor_get_exit_code(child_handle, 0, NULL) >= 0) { - /* process has exited */ + else if (tor_get_exit_code(child_handle, 0, NULL) != + PROCESS_EXIT_RUNNING) { + /* process has exited or there was an error */ /* TODO: Do something with the process return value */ /* TODO: What if the process output something since * between log_from_handle and tor_get_exit_code? */ @@ -3751,10 +3749,10 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, if (0 != retval) { if (1 == retval) { log_info(LD_GENERAL, "Port forwarding helper terminated"); - child_handle.status = 0; + child_handle.status = PROCESS_STATUS_NOTRUNNING; } else { log_warn(LD_GENERAL, "Failed to read from port forwarding helper"); - child_handle.status = -1; + child_handle.status = PROCESS_STATUS_ERROR; } /* TODO: The child might not actually be finished (maybe it failed or diff --git a/src/common/util.h b/src/common/util.h index e025265..d8c7370 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -355,8 +355,14 @@ HANDLE load_windows_system_library(const TCHAR *library_name); #ifdef UTIL_PRIVATE /* Prototypes for private functions only used by util.c (and unit tests) */ +/* Values of process_handle_t.status. PROCESS_STATUS_NOTRUNNING must be + * 0 because tor_check_port_forwarding depends on this being the initial + * statue of the static instance of process_handle_t */ +#define PROCESS_STATUS_NOTRUNNING 0 +#define PROCESS_STATUS_RUNNING 1 +#define PROCESS_STATUS_ERROR -1 typedef struct process_handle_s { - int status; // 0: not running; 1: running; -1: error + int status; #ifdef MS_WINDOWS HANDLE stdout_pipe; HANDLE stderr_pipe; @@ -372,6 +378,11 @@ typedef struct process_handle_s { int tor_spawn_background(const char *const filename, const char **argv, process_handle_t *process_handle); + +/* Return values of tor_get_exit_code() */ +#define PROCESS_EXIT_RUNNING 1 +#define PROCESS_EXIT_EXITED 0 +#define PROCESS_EXIT_ERROR -1 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 40ab098..4568fde 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1397,7 +1397,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, tt_int_op(process_handle.status, ==, expected_status); /* If the process failed to start, don't bother continuing */ - if (process_handle.status == -1) + if (process_handle.status == PROCESS_STATUS_ERROR) return; tt_int_op(process_handle.stdout_pipe, >, 0); @@ -1413,7 +1413,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, /* Check it terminated correctly */ retval = tor_get_exit_code(process_handle, 1, &exit_code); - tt_int_op(retval, ==, 0); + tt_int_op(retval, ==, PROCESS_EXIT_EXITED); tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0 @@ -1445,7 +1445,8 @@ test_util_spawn_background_ok(void *ptr) (void)ptr; - run_util_spawn_background(argv, expected_out, expected_err, 0, 1); + run_util_spawn_background(argv, expected_out, expected_err, 0, + PROCESS_STATUS_RUNNING); } /** Check that failing to find the executable works as expected */ @@ -1457,14 +1458,15 @@ test_util_spawn_background_fail(void *ptr) const char *expected_out = "ERR: Failed to spawn background process " "- code 9/2\n"; const char *expected_err = ""; - const int expected_status = -1; + const int expected_status = PROCESS_STATUS_ERROR; #else const char *argv[] = {BUILDDIR "/src/test/no-such-file", "--test", NULL}; const char *expected_out = "ERR: Failed to spawn background process " "- code 9/2\n"; const char *expected_err = ""; - // TODO: Once we can signal failure to exec, set this to be -1; - const int expected_status = 1; + /* TODO: Once we can signal failure to exec, set this to be + * PROCESS_STATUS_ERROR */ + const int expected_status = PROCESS_STATUS_RUNNING; #endif (void)ptr; @@ -1479,7 +1481,7 @@ static void test_util_spawn_background_partial_read(void *ptr) { const int expected_exit = 0; - const int expected_status = 1; + const int expected_status = PROCESS_STATUS_RUNNING; int retval, exit_code; ssize_t pos; @@ -1536,7 +1538,7 @@ test_util_spawn_background_partial_read(void *ptr) /* Check it terminated correctly */ retval = tor_get_exit_code(process_handle, 1, &exit_code); - tt_int_op(retval, ==, 0); + tt_int_op(retval, ==, PROCESS_EXIT_EXITED); tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0

1 0

[tor/master] Merge remote-tracking branch 'sjmurdoch/bug2046'
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 4f585b9ee21aa45ec696012c9ebb63b25aba50a4 Merge: b51e21c d1dd999 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Aug 30 15:51:45 2011 -0400 Merge remote-tracking branch 'sjmurdoch/bug2046' configure.in | 4 +- src/common/util.c | 619 +++++++++++++++++++++++--- src/common/util.h | 43 ++- src/test/test-child.c | 22 + src/test/test_util.c | 268 ++++++++++- src/tools/tor-fw-helper/Makefile.am | 2 +- src/tools/tor-fw-helper/tor-fw-helper-upnp.c | 3 + src/tools/tor-fw-helper/tor-fw-helper.c | 35 ++- 8 files changed, 907 insertions(+), 89 deletions(-)

1 0

[tor/master] Document limitation of log_from_handle with partial reads
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit d1dd9991cd636bafe7543aea4dbb18de69f26202 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Tue Aug 30 15:02:28 2011 +0100 Document limitation of log_from_handle with partial reads --- src/common/util.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index aa344bc..76124ef 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3654,6 +3654,9 @@ tor_split_lines(smartlist_t *sl, char *buf, int len) #ifdef MS_WINDOWS /** Read from stream, and send lines to log at the specified log level. * Returns -1 if there is a error reading, and 0 otherwise. + * If the generated stream is flushed more often than on new lines, or + * a read exceeds 256 bytes, lines will be truncated. This should be fixed, + * along with the corresponding problem on *nix (see bug #2045). */ static int log_from_handle(HANDLE *pipe, int severity)

1 0

[tor/master] Factor out and re-write code for splitting lines from a handle
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit da34360952c0fbbd8effc2789ed72b86c8045531 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Tue Aug 30 14:55:51 2011 +0100 Factor out and re-write code for splitting lines from a handle Now handles non-printable characters and will not output a spurious new-line if given a partial line. --- src/common/util.c | 89 +++++++++++++++++++++++++++++++++++++------------- src/common/util.h | 1 + src/test/test_util.c | 65 ++++++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 23 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 87c6fb5..aa344bc 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3597,6 +3597,60 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, #endif } +/* Split buf into lines, and add to smartlist. The buffer buf will be + * modified. The resulting smartlist will consist of pointers to buf, so there + * is no need to free the contents of sl. buf must be a NULL terminated + * string. len should be set to the length of the buffer excluding the + * NULL. Non-printable characters (including NULL) will be replaced with "." */ + +int +tor_split_lines(smartlist_t *sl, char *buf, int len) +{ + /* Index in buf of the start of the current line */ + int start = 0; + /* Index in buf of the current character being processed */ + int cur = 0; + /* Are we currently in a line */ + char in_line = 0; + + /* Loop over string */ + while (cur < len) { + /* Loop until end of line or end of string */ + for (; cur < len; cur++) { + if (in_line) { + if ('\r' == buf[cur] || '\n' == buf[cur]) { + /* End of line */ + buf[cur] = '\0'; + /* Point cur to the next line */ + cur++; + /* Line starts at start and ends with a null */ + break; + } else { + if (!TOR_ISPRINT(buf[cur])) + buf[cur] = '.'; + } + } else { + if ('\r' == buf[cur] || '\n' == buf[cur]) { + /* Skip leading vertical space */ + ; + } else { + in_line = 1; + start = cur; + if (!TOR_ISPRINT(buf[cur])) + buf[cur] = '.'; + } + } + } + /* We are at the end of the line or end of string. If in_line is true there + * is a line which starts at buf+start and ends at a NULL. cur points to + * the character after the NULL. */ + if (in_line) + smartlist_add(sl, (void *)(buf+start)); + in_line = 0; + } + return smartlist_len(sl); +} + #ifdef MS_WINDOWS /** Read from stream, and send lines to log at the specified log level. * Returns -1 if there is a error reading, and 0 otherwise. @@ -3606,7 +3660,7 @@ log_from_handle(HANDLE *pipe, int severity) { char buf[256]; int pos; - int start, cur, next; + smartlist_t *lines; pos = tor_read_all_handle(pipe, buf, sizeof(buf) - 1, NULL); if (pos < 0) { @@ -3626,28 +3680,17 @@ log_from_handle(HANDLE *pipe, int severity) buf[pos] = '\0'; log_debug(LD_GENERAL, "Subprocess had %d bytes to say", pos); - /* Split buf into lines and log each one */ - next = 0; // Start of the next line - while (next < pos) { - start = next; // Look for the end of this line - for (cur=start; cur<pos; cur++) { - /* On Windows \r means end of line */ - if ('\r' == buf[cur]) { - buf[cur] = '\0'; - next = cur + 1; - /* If \n follows, remove it too */ - if ((cur + 1) < pos && '\n' == buf[cur+1]) { - buf[cur + 1] = '\0'; - next = cur + 2; - } - /* Line starts at start and ends with a null (was \r\n) */ - break; - } - /* Line starts at start and ends at the end of a string - but we already added a null in earlier */ - } - log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start); - } + /* Split up the buffer */ + lines = smartlist_create(); + tor_split_lines(lines, buf, pos); + + /* Log each line */ + SMARTLIST_FOREACH(lines, char *, line, + { + log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", line); + }); + smartlist_free(lines); + return 0; } diff --git a/src/common/util.h b/src/common/util.h index 9cdd8cb..853ac8d 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -385,6 +385,7 @@ int tor_spawn_background(const char *const filename, const char **argv, #define PROCESS_EXIT_ERROR -1 int tor_get_exit_code(const process_handle_t process_handle, int block, int *exit_code); +int tor_split_lines(struct smartlist_t *sl, char *buf, int len); #ifdef MS_WINDOWS ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess); diff --git a/src/test/test_util.c b/src/test/test_util.c index 9df7bc6..a3bc003 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1554,6 +1554,9 @@ test_util_spawn_background_partial_read(void *ptr) ; } +/** + * Test that we can properly format q Windows command line + */ static void test_util_join_cmdline(void *ptr) { @@ -1602,6 +1605,67 @@ test_util_join_cmdline(void *ptr) ; } +#define MAX_SPLIT_LINE_COUNT 3 +struct split_lines_test_t { + const char *orig_line; // Line to be split (may contain \0's) + int orig_length; // Length of orig_line + const char *split_line[MAX_SPLIT_LINE_COUNT]; // Split lines +}; + +/** + * Test that we properly split a buffer into lines + */ +static void +test_util_split_lines(void *ptr) +{ + /* Test cases. orig_line of last test case must be NULL. + * The last element of split_line[i] must be NULL. */ + struct split_lines_test_t tests[] = { + {"", 0, {NULL}}, + {"foo", 3, {"foo", NULL}}, + {"\n\rfoo\n\rbar\r\n", 12, {"foo", "bar", NULL}}, + {"fo o\r\nb\tar", 10, {"fo o", "b.ar", NULL}}, + {"\x0f""f\0o\0\n\x01""b\0r\0\r", 12, {".f.o.", ".b.r.", NULL}}, + {NULL, 0, {}} + }; + + int i, j; + char *orig_line; + smartlist_t *sl; + + (void)ptr; + + for (i=0; tests[i].orig_line; i++) { + sl = smartlist_create(); + orig_line = tor_malloc(tests[i].orig_length); + memcpy(orig_line, tests[i].orig_line, tests[i].orig_length + 1); + tor_split_lines(sl, orig_line, tests[i].orig_length); + + j = 0; + log_info(LD_GENERAL, "Splitting test %d of length %d", + i, tests[i].orig_length); + SMARTLIST_FOREACH(sl, const char *, line, + { + /* Check we have not got too many lines */ + tt_int_op(j, <, MAX_SPLIT_LINE_COUNT); + /* Check that there actually should be a line here */ + tt_assert(tests[i].split_line[j] != NULL); + log_info(LD_GENERAL, "Line %d of test %d, should be <%s>", + j, i, tests[i].split_line[j]); + /* Check that the line is as expected */ + tt_str_op(tests[i].split_line[j], ==, line); + j++; + }); + /* Check that we didn't miss some lines */ + tt_assert(tests[i].split_line[j] == NULL); + tor_free(orig_line); + smartlist_free(sl); + } + + done: + ; +} + static void test_util_di_ops(void) { @@ -1691,6 +1755,7 @@ struct testcase_t util_tests[] = { UTIL_TEST(spawn_background_fail, 0), UTIL_TEST(spawn_background_partial_read, 0), UTIL_TEST(join_cmdline, 0), + UTIL_TEST(split_lines, 0), END_OF_TESTCASES };

1 0