- tor-commits - lists.torproject.org

[tor/master] Add Windows version of tor_spawn_background and ancillary functions
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit fec902dd6024dd170d151409387df5319b6e2ad0 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Thu Jul 21 19:26:19 2011 +0100 Add Windows version of tor_spawn_background and ancillary functions --- src/common/util.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++---- src/common/util.h | 4 +- src/test/test_util.c | 14 ++++- 3 files changed, 143 insertions(+), 16 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 5bc7a80..3d39f59 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2976,18 +2976,107 @@ format_helper_exit_status(unsigned char child_state, int saved_errno, * 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 - * Python. + * 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) { process_handle_t process_handle; #ifdef MS_WINDOWS - (void) filename; (void) argv; - log_warn(LD_BUG, "not yet implemented on Windows."); + HANDLE stdout_pipe_read = NULL; + HANDLE stdout_pipe_write = NULL; + HANDLE stderr_pipe_read = NULL; + HANDLE stderr_pipe_write = NULL; + + SECURITY_ATTRIBUTES saAttr; + smartlist_t *argv_list; + char *joined_argv; + int i; + + saAttr.nLength = sizeof(SECURITY_ATTRIBUTES); + saAttr.bInheritHandle = TRUE; + saAttr.lpSecurityDescriptor = NULL; + 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; + } + 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())); + } + + /* Set up pipe for stderr */ + if (!CreatePipe(&stderr_pipe_read, &stderr_pipe_write, &saAttr, 0)) { + log_warn(LD_GENERAL, + "Failed to create pipe for stderr communication with child process: %s", + format_win32_error(GetLastError())); + return process_handle; + } + 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())); + } + + /* Create the child process */ + + /* 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); + + STARTUPINFO siStartInfo; + BOOL retval = FALSE; + + ZeroMemory(&process_handle.pid, sizeof(PROCESS_INFORMATION)); + ZeroMemory(&siStartInfo, 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 + &process_handle.pid); // receives PROCESS_INFORMATION + + tor_free(joined_argv); + + if (!retval) { + log_warn(LD_GENERAL, + "Failed to create child process %s: %s", filename, + 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 = 0; + } + + // TODO: Close pipes on exit + return process_handle; -#else +#else // MS_WINDOWS pid_t pid; int stdout_pipe[2]; int stderr_pipe[2]; @@ -3154,15 +3243,25 @@ tor_spawn_background(const char *const filename, const char **argv) process_handle.status = 0; process_handle.pid = pid; return process_handle; -#endif +#endif // MS_WINDOWS } int tor_get_exit_code(const process_handle_t process_handle) { #ifdef MS_WINDOWS - log_warn(LD_BUG, "not yet implemented on Windows."); - return -1; + DWORD exit_code; + BOOL retval; + WaitForSingleObject(process_handle.pid.hProcess, INFINITE); + retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); + + if (!retval) { + log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", + format_win32_error(GetLastError())); + return -1; + } else { + return exit_code; + } #else int stat_loc; @@ -3183,13 +3282,23 @@ tor_get_exit_code(const process_handle_t process_handle) } ssize_t -tor_read_all_from_process_stdin(const process_handle_t process_handle, +tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return -1; + BOOL retval; + DWORD bytes_read; + retval = ReadFile(process_handle.stdout_pipe, buf, count, &bytes_read, NULL); + if (!retval) { + log_warn(LD_GENERAL, + "Failed to read from stdin pipe: %s", + format_win32_error(GetLastError())); + return -1; + } else { + return bytes_read; + } #else - return read_all(process_handle.stdin_pipe, buf, count, 0); + return read_all(process_handle.stdout_pipe, buf, count, 0); #endif } @@ -3198,7 +3307,17 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return -1; + BOOL retval; + DWORD bytes_read; + retval = ReadFile(process_handle.stderr_pipe, buf, count, &bytes_read, NULL); + if (!retval) { + log_warn(LD_GENERAL, + "Failed to read from stderr pipe: %s", + format_win32_error(GetLastError())); + return -1; + } else { + return bytes_read; + } #else return read_all(process_handle.stderr_pipe, buf, count, 0); #endif diff --git a/src/common/util.h b/src/common/util.h index 2cf57a1..c111ba7 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -353,7 +353,7 @@ typedef struct process_handle_s { #ifdef MS_WINDOWS HANDLE stdout_pipe; HANDLE stderr_pipe; - HANDLE pid; + PROCESS_INFORMATION pid; #else int stdout_pipe; int stderr_pipe; @@ -364,7 +364,7 @@ typedef struct process_handle_s { process_handle_t tor_spawn_background(const char *const filename, const char **argv); int tor_get_exit_code(const process_handle_t pid); -ssize_t tor_read_all_from_process_stdin(const process_handle_t process_handle, +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); diff --git a/src/test/test_util.c b/src/test/test_util.c index fce53b8..28030b7 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1394,24 +1394,25 @@ run_util_spawn_background(const char *argv[], const char *expected_out, tt_int_op(process_handle.stderr_pipe, >, 0); /* Check stdout */ - pos = tor_read_all_from_process_stdin(process_handle, stdout_buf, + pos = tor_read_all_from_process_stdout(process_handle, stdout_buf, sizeof(stdout_buf) - 1); tt_assert(pos >= 0); stdout_buf[pos] = '\0'; - tt_int_op(pos, ==, strlen(expected_out)); tt_str_op(stdout_buf, ==, expected_out); + tt_int_op(pos, ==, strlen(expected_out)); /* Check it terminated correctly */ retval = tor_get_exit_code(process_handle); tt_int_op(retval, ==, expected_exit); + // TODO: Make test-child exit with something other than 0 /* Check stderr */ pos = tor_read_all_from_process_stderr(process_handle, stderr_buf, sizeof(stderr_buf) - 1); tt_assert(pos >= 0); stderr_buf[pos] = '\0'; - tt_int_op(pos, ==, strlen(expected_err)); tt_str_op(stderr_buf, ==, expected_err); + tt_int_op(pos, ==, strlen(expected_err)); done: ; @@ -1421,9 +1422,16 @@ run_util_spawn_background(const char *argv[], const char *expected_out, static void test_util_spawn_background_ok(void *ptr) { +#ifdef MS_WINDOWS + // TODO: Under MSYS, BUILDDIR in orconfig.h needs to be tweaked + const char *argv[] = {BUILDDIR "/src/test/test-child.exe", "--test", NULL}; + const char *expected_out = "OUT\r\n--test\r\nDONE\r\n"; + const char *expected_err = "ERR\r\n"; +#else const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; const char *expected_out = "OUT\n--test\nDONE\n"; const char *expected_err = "ERR\n"; +#endif (void)ptr;

1 0

[tor/master] Use PeekNamedPipe to avoid blocking ReadFile when there is nothing to read
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 2d5059e08e16ef704c3021e3ccdc2923be02c7cb Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Fri Jul 22 21:12:00 2011 +0100 Use PeekNamedPipe to avoid blocking ReadFile when there is nothing to read --- src/common/util.c | 48 +++++++++++++++++++++++++++++++++++++++--------- src/test/test_util.c | 4 ++-- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index e9b7909..31d5586 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3287,27 +3287,55 @@ tor_get_exit_code(const process_handle_t process_handle) #ifdef MS_WINDOWS /* Windows equivalent of read_all */ static ssize_t -read_all_handle(HANDLE h, char *buf, size_t count) +read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) { size_t numread = 0; BOOL retval; - DWORD bytes_read; + DWORD byte_count; + BOOL process_exited = FALSE; if (count > SIZE_T_CEILING || count > SSIZE_T_MAX) return -1; while (numread != count) { - retval = ReadFile(h, buf+numread, count-numread, &bytes_read, NULL); + retval = PeekNamedPipe(h, NULL, 0, NULL, &byte_count, NULL); if (!retval) { log_warn(LD_GENERAL, - "Failed to read from stdin pipe: %s", + "Failed to peek from handle: %s", format_win32_error(GetLastError())); return -1; - } else if (0 == bytes_read) { + } else if (0 == byte_count) { + /* Nothing available: process exited or it is busy */ + + /* Keep on reading if we don't know whether the process is running */ + if (NULL == hProcess) + continue; + + /* The process exited and there's nothing left to read from it */ + if (process_exited) + break; + + /* If process is not running, check for output one more time in case + it wrote something after the peek was performed. Otherwise keep on + waiting for output */ + byte_count = WaitForSingleObject(hProcess, 0); + if (WAIT_TIMEOUT != byte_count) + process_exited = TRUE; + + continue; + } + + retval = ReadFile(h, buf+numread, count-numread, &byte_count, NULL); + if (!retval) { + log_warn(LD_GENERAL, + "Failed to read from handle: %s", + format_win32_error(GetLastError())); + return -1; + } else if (0 == byte_count) { /* End of file */ - return bytes_read; + break; } - numread += bytes_read; + numread += byte_count; } return (ssize_t)numread; } @@ -3318,7 +3346,8 @@ tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return read_all_handle(process_handle.stdout_pipe, buf, count); + return read_all_handle(process_handle.stdout_pipe, buf, count, + process_handle.pid.hProcess); #else return read_all(process_handle.stdout_pipe, buf, count, 0); #endif @@ -3329,7 +3358,8 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return read_all_handle(process_handle.stderr_pipe, buf, count); + return read_all_handle(process_handle.stderr_pipe, buf, count, + process_handle.pid.hProcess); #else return read_all(process_handle.stderr_pipe, buf, count, 0); #endif diff --git a/src/test/test_util.c b/src/test/test_util.c index 63c8ad8..c6dc98d 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1431,11 +1431,11 @@ test_util_spawn_background_ok(void *ptr) #ifdef MS_WINDOWS // TODO: Under MSYS, BUILDDIR in orconfig.h needs to be tweaked const char *argv[] = {BUILDDIR "/src/test/test-child.exe", "--test", NULL}; - const char *expected_out = "OUT\r\n--test\r\nDONE\r\n"; + const char *expected_out = "OUT\r\n--test\r\nSLEEPING\r\nDONE\r\n"; const char *expected_err = "ERR\r\n"; #else const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; - const char *expected_out = "OUT\n--test\nDONE\n"; + const char *expected_out = "OUT\n--test\nSLEEPING\nDONE\n"; const char *expected_err = "ERR\n"; #endif

1 0

[tor/master] Fix compilation on non-Windows platforms
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 99baa7e45cc65f0683912c59934679df554fa306 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Sat Jul 23 14:32:39 2011 +0100 Fix compilation on non-Windows platforms --- src/common/util.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 2691b1f..e63047b 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3094,6 +3094,11 @@ tor_spawn_background(const char *const filename, const char **argv) static int max_fd = -1; + // XXX + process_handle.pid = 0; + process_handle.stderr_pipe = 0; + process_handle.stdout_pipe = 0; + /* 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 */ error_message_length = strlen(error_message); @@ -3267,11 +3272,12 @@ tor_get_exit_code(const process_handle_t process_handle) } #else int stat_loc; + int retval; retval = waitpid(process_handle.pid, &stat_loc, 0); if (retval != process_handle.pid) { log_warn(LD_GENERAL, "waitpid() failed for PID %d: %s", process_handle.pid, - sterror(errno)); + strerror(errno)); return -1; } @@ -3457,7 +3463,9 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, /* When fw-helper fails, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_FAIL 60 + // XXX: remove static int child_pid = -1; + static process_handle_t child_handle = {0, 0, 0, 0}; static FILE *stdout_read = NULL; static FILE *stderr_read = NULL; static time_t time_to_run_helper = 0; @@ -3491,7 +3499,7 @@ 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; - child_pid = tor_spawn_background(filename, &fd_out, &fd_err, argv); + child_handle = tor_spawn_background(filename, argv); if (child_pid < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename);

1 0

[tor/master] If hProcess is NULL, read_all_handle returns if it would block
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit c5796a8fb2fc70085c771d6296d4f29a7ddb68ae Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Sat Jul 23 21:35:50 2011 +0100 If hProcess is NULL, read_all_handle returns if it would block --- src/common/util.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 31d5586..2691b1f 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3304,12 +3304,12 @@ read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) "Failed to peek from handle: %s", format_win32_error(GetLastError())); return -1; - } else if (0 == byte_count) { + } else if (0 == byte_count) /* Nothing available: process exited or it is busy */ - /* Keep on reading if we don't know whether the process is running */ + /* Exit if we don't know whether the process is running */ if (NULL == hProcess) - continue; + break; /* The process exited and there's nothing left to read from it */ if (process_exited)

1 0

[tor/master] Complete logging of output from port forwarding helper
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 7d015c886a3fc985ab767de01811d1591f0ac86a Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Thu Aug 18 18:41:23 2011 +0100 Complete logging of output from port forwarding helper --- src/common/util.c | 139 +++++++++++++++++++++++++++++++++++++------------- src/common/util.h | 6 ++- src/test/test_util.c | 4 +- 3 files changed, 110 insertions(+), 39 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 5a5f3b5..3769d06 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3070,7 +3070,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 = 0; + process_handle.status = 1; } // TODO: Close pipes on exit @@ -3248,20 +3248,36 @@ tor_spawn_background(const char *const filename, const char **argv) needs to know about the pid in order to reap it later */ } - process_handle.status = 0; + 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); + /* 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"); + return process_handle; #endif // MS_WINDOWS } int -tor_get_exit_code(const process_handle_t process_handle) +tor_get_exit_code(const process_handle_t process_handle, int block) { #ifdef MS_WINDOWS DWORD exit_code; BOOL retval; - WaitForSingleObject(process_handle.pid.hProcess, INFINITE); - retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); + if (block) { + exit_code = WaitForSingleObject(process_handle.pid.hProcess, INFINITE); + retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); + } else { + exit_code = WaitForSingleObject(process_handle.pid.hProcess, 0); + if (WAIT_TIMEOUT == exit_code) { + // Process has not exited + return -2; + } + retval = GetExitCodeProcess(process_handle.pid.hProcess, &exit_code); + } if (!retval) { log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", @@ -3371,6 +3387,55 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, #endif } +#ifdef MS_WINDOWS +static int +log_from_handle(HANDLE *pipe, int severity) +{ + char buf[256]; + int pos; + int start, cur, next; + + pos = tor_read_all_handle(pipe, buf, sizeof(buf) - 1, NULL); + if (pos < 0) { + // 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) + 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? + buf[pos] = '\0'; + log_debug(LD_GENERAL, "Subprocess had %d bytes to say", pos); + + 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++) { + if ('\r' == buf[cur]) { + buf[cur] = '\0'; + next = cur + 1; + if ((cur + 1) < pos && buf[cur+1] < pos) { + 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); + } + return pos; +} + +#else /** Read from stream, and send lines to log at the specified log level. * Returns 1 if stream is closed normally, -1 if there is a error reading, and * 0 otherwise. Handles lines from tor-fw-helper and @@ -3446,28 +3511,23 @@ log_from_pipe(FILE *stream, int severity, const char *executable, /* We should never get here */ return -1; } +#endif void tor_check_port_forwarding(const char *filename, int dir_port, int or_port, time_t now) { -#ifdef MS_WINDOWS - (void) filename; (void) dir_port; (void) or_port; (void) now; - (void) tor_spawn_background; - (void) log_from_pipe; - log_warn(LD_GENERAL, "Sorry, port forwarding is not yet supported " - "on windows."); -#else /* When fw-helper succeeds, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_SUCCESS 300 /* When fw-helper fails, how long do we wait until running it again */ #define TIME_TO_EXEC_FWHELPER_FAIL 60 - // XXX: remove - static int child_pid = -1; - static process_handle_t child_handle = {0, 0, 0, 0}; - static FILE *stdout_read = NULL; - static FILE *stderr_read = NULL; +#ifdef MS_WINDOWS + static process_handle_t child_handle = {0, NULL, NULL, {NULL}}; +#else + static process_handle_t child_handle; +#endif + static time_t time_to_run_helper = 0; int stdout_status, stderr_status, retval; const char *argv[10]; @@ -3492,37 +3552,40 @@ 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 (-1 == child_pid && - time_to_run_helper < now) { - int fd_out=-1, fd_err=-1; - + if (child_handle.status <= 0 && time_to_run_helper < now) { /* Assume tor-fw-helper will succeed, start it later*/ time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_SUCCESS; child_handle = tor_spawn_background(filename, argv); - if (child_pid < 0) { + if (child_handle.status < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename); - child_pid = -1; return; } - /* Set stdout/stderr pipes to be non-blocking */ - fcntl(fd_out, F_SETFL, O_NONBLOCK); - fcntl(fd_err, F_SETFL, O_NONBLOCK); - /* Open the buffered IO streams */ - stdout_read = fdopen(fd_out, "r"); - stderr_read = fdopen(fd_err, "r"); - +#ifdef MS_WINDOWS + log_info(LD_GENERAL, + "Started port forwarding helper (%s)", filename); +#else log_info(LD_GENERAL, "Started port forwarding helper (%s) with pid %d", filename, child_pid); +#endif } /* If child is running, read from its stdout and stderr) */ - if (child_pid > 0) { + if (child_handle.status > 0) { /* Read from stdout/stderr and log result */ retval = 0; - stdout_status = log_from_pipe(stdout_read, LOG_INFO, filename, &retval); - stderr_status = log_from_pipe(stderr_read, LOG_WARN, filename, &retval); +#ifdef MS_WINDOWS + stdout_status = log_from_handle(child_handle.stdout_pipe, LOG_INFO); + stderr_status = log_from_handle(child_handle.stderr_pipe, LOG_WARN); + // If we got this far (on Windows), the process started + retval = 0; +#else + stdout_status = log_from_pipe(child_handle.stdout_handle, + LOG_INFO, filename, &retval); + stderr_status = log_from_pipe(child_handle.stderr_handle, + LOG_WARN, filename, &retval); +#endif if (retval) { /* There was a problem in the child process */ time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL; @@ -3532,9 +3595,16 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, if (-1 == stdout_status || -1 == stderr_status) /* There was a failure */ retval = -1; +#ifdef MS_WINDOWS + else if (tor_get_exit_code(child_handle, 0) >= 0) { + /* process has exited */ + retval = 1; + } +#else else if (1 == stdout_status || 1 == stderr_status) /* stdout or stderr was closed */ retval = 1; +#endif else /* Both are fine */ retval = 0; @@ -3549,9 +3619,8 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, /* TODO: The child might not actually be finished (maybe it failed or closed stdout/stderr), so maybe we shouldn't start another? */ - child_pid = -1; + child_handle.status = -1; } } -#endif } diff --git a/src/common/util.h b/src/common/util.h index 2efe557..40fe305 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -349,7 +349,7 @@ HANDLE load_windows_system_library(const TCHAR *library_name); /* Prototypes for private functions only used by util.c (and unit tests) */ typedef struct process_handle_s { - int status; + int status; // 0: not running; 1: running; -1: error #ifdef MS_WINDOWS HANDLE stdout_pipe; HANDLE stderr_pipe; @@ -357,13 +357,15 @@ typedef struct process_handle_s { #else int stdout_pipe; int stderr_pipe; + FILE *stdout_handle; + FILE *stderr_handle; int pid; #endif // MS_WINDOWS } process_handle_t; process_handle_t tor_spawn_background(const char *const filename, const char **argv); -int tor_get_exit_code(const process_handle_t pid); +int tor_get_exit_code(const process_handle_t pid, int block); ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess); 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 071fd04..6f87560 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1408,7 +1408,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, tt_int_op(pos, ==, strlen(expected_out)); /* Check it terminated correctly */ - retval = tor_get_exit_code(process_handle); + retval = tor_get_exit_code(process_handle, 1); tt_int_op(retval, ==, expected_exit); // TODO: Make test-child exit with something other than 0 @@ -1529,7 +1529,7 @@ test_util_spawn_background_partial_read(void *ptr) #endif /* Check it terminated correctly */ - retval = tor_get_exit_code(process_handle); + retval = tor_get_exit_code(process_handle, 1); tt_int_op(retval, ==, expected_exit); // TODO: Make test-child exit with something other than 0

1 0

[tor/master] Test case for reading the partial output of a background process
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 5bf9890b3b18ad98f5ac601992bc08533ce9e209 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Sun Jul 24 23:31:59 2011 +0100 Test case for reading the partial output of a background process --- src/common/util.c | 14 ++++---- src/common/util.h | 1 + src/test/test-child.c | 4 ++ src/test/test_util.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 7 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index e63047b..5a5f3b5 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3292,8 +3292,8 @@ tor_get_exit_code(const process_handle_t process_handle) #ifdef MS_WINDOWS /* Windows equivalent of read_all */ -static ssize_t -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) { size_t numread = 0; BOOL retval; @@ -3310,7 +3310,7 @@ read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess) "Failed to peek from handle: %s", format_win32_error(GetLastError())); return -1; - } else if (0 == byte_count) + } else if (0 == byte_count) { /* Nothing available: process exited or it is busy */ /* Exit if we don't know whether the process is running */ @@ -3352,8 +3352,8 @@ tor_read_all_from_process_stdout(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return read_all_handle(process_handle.stdout_pipe, buf, count, - process_handle.pid.hProcess); + return tor_read_all_handle(process_handle.stdout_pipe, buf, count, + process_handle.pid.hProcess); #else return read_all(process_handle.stdout_pipe, buf, count, 0); #endif @@ -3364,8 +3364,8 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - return read_all_handle(process_handle.stderr_pipe, buf, count, - process_handle.pid.hProcess); + return tor_read_all_handle(process_handle.stderr_pipe, buf, count, + process_handle.pid.hProcess); #else return read_all(process_handle.stderr_pipe, buf, count, 0); #endif diff --git a/src/common/util.h b/src/common/util.h index c111ba7..2efe557 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -364,6 +364,7 @@ typedef struct process_handle_s { process_handle_t tor_spawn_background(const char *const filename, const char **argv); int tor_get_exit_code(const process_handle_t pid); +ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count, HANDLE hProcess); 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, diff --git a/src/test/test-child.c b/src/test/test-child.c index cf49c5d..1b9c5e3 100644 --- a/src/test/test-child.c +++ b/src/test/test-child.c @@ -19,6 +19,10 @@ main(int argc, char **argv) for (i = 1; i < argc; i++) fprintf(stdout, "%s\n", argv[i]); fprintf(stdout, "SLEEPING\n"); + /* We need to flush stdout so that test_util_spawn_background_partial_read() + succeed. Otherwise ReadFile() will get the entire output in one */ + // XXX: Can we make stdio flush on newline? + fflush(stdout); #ifdef MS_WINDOWS Sleep(1000); #else diff --git a/src/test/test_util.c b/src/test/test_util.c index c6dc98d..071fd04 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1468,6 +1468,83 @@ test_util_spawn_background_fail(void *ptr) run_util_spawn_background(argv, expected_out, expected_err, 255, expected_status); } +/** Helper function for testing tor_spawn_background */ +static void +test_util_spawn_background_partial_read(void *ptr) +{ +#ifdef MS_WINDOWS + // TODO: Under MSYS, BUILDDIR in orconfig.h needs to be tweaked + const char *argv[] = {BUILDDIR "/src/test/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; +#else + const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; + const char *expected_out = "OUT\n--test\nSLEEPING\nDONE\n"; + const char *expected_err = "ERR\r\n"; +#endif + const int expected_exit = 0; + const int expected_status = 0; + + int retval; + ssize_t pos; + process_handle_t process_handle; + char stdout_buf[100], stderr_buf[100]; + + /* Start the program */ + process_handle = tor_spawn_background(argv[0], argv); + tt_int_op(process_handle.status, ==, expected_status); + + /* Check stdout */ +#ifdef MS_WINDOWS + for (expected_out_ctr =0; expected_out[expected_out_ctr] != NULL;) { + pos = tor_read_all_handle(process_handle.stdout_pipe, stdout_buf, + sizeof(stdout_buf) - 1, NULL); + log_info(LD_GENERAL, "tor_read_all_handle() returned %d", (int)pos); + + /* We would have blocked, keep on trying */ + if (0 == pos) + continue; + + tt_assert(pos >= 0); + stdout_buf[pos] = '\0'; + tt_str_op(stdout_buf, ==, expected_out[expected_out_ctr]); + tt_int_op(pos, ==, strlen(expected_out[expected_out_ctr])); + expected_out_ctr++; + } + /* The process should have exited without writing more */ + pos = tor_read_all_handle(process_handle.stdout_pipe, stdout_buf, + sizeof(stdout_buf) - 1, + process_handle.pid.hProcess); + tt_int_op(pos, ==, 0); +#else + pos = tor_read_all_from_process_stdout(process_handle, stdout_buf, + sizeof(stdout_buf) - 1); + tt_assert(pos >= 0); + stdout_buf[pos] = '\0'; + tt_str_op(stdout_buf, ==, expected_out); + tt_int_op(pos, ==, strlen(expected_out)); +#endif + + /* Check it terminated correctly */ + retval = tor_get_exit_code(process_handle); + tt_int_op(retval, ==, expected_exit); + // TODO: Make test-child exit with something other than 0 + + /* Check stderr */ + pos = tor_read_all_from_process_stderr(process_handle, stderr_buf, + sizeof(stderr_buf) - 1); + tt_assert(pos >= 0); + stderr_buf[pos] = '\0'; + tt_str_op(stderr_buf, ==, expected_err); + tt_int_op(pos, ==, strlen(expected_err)); + + done: + ; +} + static void test_util_di_ops(void) { @@ -1555,6 +1632,7 @@ struct testcase_t util_tests[] = { #endif UTIL_TEST(spawn_background_ok, 0), UTIL_TEST(spawn_background_fail, 0), + UTIL_TEST(spawn_background_partial_read, 0), END_OF_TESTCASES };

1 0

[tor/master] Add code to read all from a handle, but this block forever
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 55a1cb53d6d1a8375afc12679b43901a977cf4b7 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Fri Jul 22 15:57:56 2011 +0100 Add code to read all from a handle, but this block forever See http://stackoverflow.com/questions/3722409/windows-child-process-with-redir… for a potential solution --- src/common/util.c | 56 +++++++++++++++++++++++++++++------------------- src/test/test-child.c | 18 +++++++++++++++ src/test/test_util.c | 24 +++++++++++++++++--- 3 files changed, 72 insertions(+), 26 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 3d39f59..e9b7909 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3195,6 +3195,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 nbytes = write(STDOUT_FILENO, error_message, error_message_length); nbytes = write(STDOUT_FILENO, hex_errno, sizeof(hex_errno)); @@ -3217,6 +3218,8 @@ 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 + /* Return read end of the pipes to caller, and close write end */ process_handle.stdout_pipe = stdout_pipe[0]; retval = close(stdout_pipe[1]); @@ -3281,22 +3284,41 @@ tor_get_exit_code(const process_handle_t process_handle) #endif // MS_WINDOWS } -ssize_t -tor_read_all_from_process_stdout(const process_handle_t process_handle, - char *buf, size_t count) -{ #ifdef MS_WINDOWS +/* Windows equivalent of read_all */ +static ssize_t +read_all_handle(HANDLE h, char *buf, size_t count) +{ + size_t numread = 0; BOOL retval; DWORD bytes_read; - retval = ReadFile(process_handle.stdout_pipe, buf, count, &bytes_read, NULL); - if (!retval) { - log_warn(LD_GENERAL, - "Failed to read from stdin pipe: %s", - format_win32_error(GetLastError())); + + if (count > SIZE_T_CEILING || count > SSIZE_T_MAX) return -1; - } else { - return bytes_read; + + while (numread != count) { + retval = ReadFile(h, buf+numread, count-numread, &bytes_read, NULL); + if (!retval) { + log_warn(LD_GENERAL, + "Failed to read from stdin pipe: %s", + format_win32_error(GetLastError())); + return -1; + } else if (0 == bytes_read) { + /* End of file */ + return bytes_read; + } + numread += bytes_read; } + return (ssize_t)numread; +} +#endif + +ssize_t +tor_read_all_from_process_stdout(const process_handle_t process_handle, + char *buf, size_t count) +{ +#ifdef MS_WINDOWS + return read_all_handle(process_handle.stdout_pipe, buf, count); #else return read_all(process_handle.stdout_pipe, buf, count, 0); #endif @@ -3307,17 +3329,7 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, char *buf, size_t count) { #ifdef MS_WINDOWS - BOOL retval; - DWORD bytes_read; - retval = ReadFile(process_handle.stderr_pipe, buf, count, &bytes_read, NULL); - if (!retval) { - log_warn(LD_GENERAL, - "Failed to read from stderr pipe: %s", - format_win32_error(GetLastError())); - return -1; - } else { - return bytes_read; - } + return read_all_handle(process_handle.stderr_pipe, buf, count); #else return read_all(process_handle.stderr_pipe, buf, count, 0); #endif diff --git a/src/test/test-child.c b/src/test/test-child.c index ca52750..cf49c5d 100644 --- a/src/test/test-child.c +++ b/src/test/test-child.c @@ -1,4 +1,11 @@ #include <stdio.h> +#include "orconfig.h" +#ifdef MS_WINDOWS +#define WINDOWS_LEAN_AND_MEAN +#include <windows.h> +#else +#include <unistd.h> +#endif /** Trivial test program which prints out its command line arguments so we can * check if tor_spawn_background() works */ @@ -11,7 +18,18 @@ main(int argc, char **argv) fprintf(stderr, "ERR\n"); for (i = 1; i < argc; i++) fprintf(stdout, "%s\n", argv[i]); + fprintf(stdout, "SLEEPING\n"); +#ifdef MS_WINDOWS + Sleep(1000); +#else + sleep(1); +#endif fprintf(stdout, "DONE\n"); +#ifdef MS_WINDOWS + Sleep(1000); +#else + sleep(1); +#endif return 0; } diff --git a/src/test/test_util.c b/src/test/test_util.c index 28030b7..63c8ad8 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1379,7 +1379,8 @@ test_util_fgets_eagain(void *ptr) /** Helper function for testing tor_spawn_background */ static void run_util_spawn_background(const char *argv[], const char *expected_out, - const char *expected_err, int expected_exit) + const char *expected_err, int expected_exit, + int expected_status) { int retval; ssize_t pos; @@ -1389,7 +1390,12 @@ run_util_spawn_background(const char *argv[], const char *expected_out, /* Start the program */ process_handle = tor_spawn_background(argv[0], argv); - tt_int_op(process_handle.status, ==, 0); + tt_int_op(process_handle.status, ==, expected_status); + + /* If the process failed to start, don't bother continuing */ + if (process_handle.status == -1) + return; + tt_int_op(process_handle.stdout_pipe, >, 0); tt_int_op(process_handle.stderr_pipe, >, 0); @@ -1435,21 +1441,31 @@ test_util_spawn_background_ok(void *ptr) (void)ptr; - run_util_spawn_background(argv, expected_out, expected_err, 0); + run_util_spawn_background(argv, expected_out, expected_err, 0, 0); } /** Check that failing to find the executable works as expected */ static void test_util_spawn_background_fail(void *ptr) { +#ifdef MS_WINDOWS 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 = ""; + const int expected_status = -1; +#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 = 0; +#endif (void)ptr; - run_util_spawn_background(argv, expected_out, expected_err, 255); + run_util_spawn_background(argv, expected_out, expected_err, 255, expected_status); } static void

1 0

[tor/master] XXX Hack to allow tor-fw-helper to be built on Windows
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 2eca0165945f4caddf0585f3f9a33e422f1deeb1 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Wed Jul 20 16:07:13 2011 +0100 XXX Hack to allow tor-fw-helper to be built on Windows For some reason, --with-libminiupnpc-dir doesn't work on Windows, so this hardcodes /local/lib as the path in which libminiupnpc.a can be found. Also, libminiupnpc needs libws2_32 and libiphlpapi under Windows, so this hardcodes these libraries when building the ./configure test program. These changes almost certainly break *nix, so should be fixed before merge. --- configure.in | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.in b/configure.in index 0a01862..f111355 100644 --- a/configure.in +++ b/configure.in @@ -559,7 +559,7 @@ dnl There are no packages for Debian or Redhat as of this patch if test "$upnp" = "true"; then AC_DEFINE(MINIUPNPC, 1, [Define to 1 if we are building with UPnP.]) - TOR_SEARCH_LIBRARY(libminiupnpc, $trylibminiupnpcdir, [-lminiupnpc], + TOR_SEARCH_LIBRARY(libminiupnpc, $trylibminiupnpcdir, [-lminiupnpc -lws2_32 -liphlpapi], [#include <miniupnpc/miniwget.h> #include <miniupnpc/miniupnpc.h> #include <miniupnpc/upnpcommands.h>], @@ -567,7 +567,7 @@ if test "$upnp" = "true"; then const char * minissdpdsock, int sameport);], [upnpDiscover(1, 0, 0, 0); exit(0);], [--with-libminiupnpc-dir], - [/usr/lib/]) + [/local/lib/]) fi AC_SYS_LARGEFILE

1 0

[tor/master] XXX Use static linking on Windows because miniupnpc doesn't work as DLL
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 0ea28ccea0034ef846b0600f9b683d3b341299e6 Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Thu Jul 21 13:47:16 2011 +0100 XXX Use static linking on Windows because miniupnpc doesn't work as DLL --- src/tools/tor-fw-helper/tor-fw-helper-upnp.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/tools/tor-fw-helper/tor-fw-helper-upnp.c b/src/tools/tor-fw-helper/tor-fw-helper-upnp.c index 18ca563..55bae11 100644 --- a/src/tools/tor-fw-helper/tor-fw-helper-upnp.c +++ b/src/tools/tor-fw-helper/tor-fw-helper-upnp.c @@ -9,6 +9,7 @@ #include "orconfig.h" #ifdef MINIUPNPC +#define STATICLIB #include <stdint.h> #include <string.h> #include <stdio.h>

1 0

[tor/master] XXX Link in libiphlpapi (should be disabled on non Win32 platforms)
by nickm＠torproject.org 30 Aug '11

30 Aug '11

commit 8db98c13b5cdc8eb11dfb593aea380334dc2ef7a Author: Steven Murdoch <Steven.Murdoch(a)cl.cam.ac.uk> Date: Thu Jul 21 13:47:58 2011 +0100 XXX Link in libiphlpapi (should be disabled on non Win32 platforms) --- src/tools/tor-fw-helper/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/tools/tor-fw-helper/Makefile.am b/src/tools/tor-fw-helper/Makefile.am index 75878f2..05cea77 100644 --- a/src/tools/tor-fw-helper/Makefile.am +++ b/src/tools/tor-fw-helper/Makefile.am @@ -25,7 +25,7 @@ endif if MINIUPNPC miniupnpc_ldflags = @TOR_LDFLAGS_libminiupnpc@ -miniupnpc_ldadd = -lminiupnpc -lm +miniupnpc_ldadd = -lminiupnpc -lm -liphlpapi miniupnpc_cppflags = @TOR_CPPFLAGS_libminiupnpc@ else miniupnpc_ldflags =

1 0