From 2eca0165945f4caddf0585f3f9a33e422f1deeb1 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Wed, 20 Jul 2011 16:07:13 +0100 Subject: [PATCH 01/31] 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 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.in b/configure.in index 0a018628db..f111355485 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 #include #include ], @@ -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 From 0ea28ccea0034ef846b0600f9b683d3b341299e6 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 21 Jul 2011 13:47:16 +0100 Subject: [PATCH 02/31] 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 file changed, 1 insertion(+) 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 18ca56394f..55bae11e1a 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 #include #include From 8db98c13b5cdc8eb11dfb593aea380334dc2ef7a Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 21 Jul 2011 13:47:58 +0100 Subject: [PATCH 03/31] XXX Link in libiphlpapi (should be disabled on non Win32 platforms) --- src/tools/tor-fw-helper/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/tor-fw-helper/Makefile.am b/src/tools/tor-fw-helper/Makefile.am index 75878f2594..05cea7716a 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 = From 2002d4acdfac823c03cca3ed92de7f60b3272d86 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 21 Jul 2011 14:14:57 +0100 Subject: [PATCH 04/31] Under Windows, call WSAStartup before using the network --- src/tools/tor-fw-helper/tor-fw-helper.c | 33 ++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/tools/tor-fw-helper/tor-fw-helper.c b/src/tools/tor-fw-helper/tor-fw-helper.c index 20d60d7ba6..926fa61800 100644 --- a/src/tools/tor-fw-helper/tor-fw-helper.c +++ b/src/tools/tor-fw-helper/tor-fw-helper.c @@ -13,6 +13,7 @@ * later date. */ +#include "orconfig.h" #include #include #include @@ -20,7 +21,10 @@ #include #include -#include "orconfig.h" +#ifdef MS_WINDOWS +#include +#endif + #include "tor-fw-helper.h" #ifdef NAT_PMP #include "tor-fw-helper-natpmp.h" @@ -219,6 +223,29 @@ tor_fw_add_dir_port(tor_fw_options_t *tor_fw_options, } } +/** Called before we make any calls to network-related functions. + * (Some operating systems require their network libraries to be + * initialized.) (from common/compat.c) */ +static int +network_init(void) +{ +#ifdef MS_WINDOWS + /* This silly exercise is necessary before windows will allow + * gethostbyname to work. */ + WSADATA WSAData; + int r; + r = WSAStartup(0x101, &WSAData); + if (r) { + fprintf(stderr, "E: Error initializing Windows network layer - code was %d", r); + return -1; + } + /* WSAData.iMaxSockets might show the max sockets we're allowed to use. + * We might use it to complain if we're trying to be a server but have + * too few sockets available. */ +#endif + return 0; +} + int main(int argc, char **argv) { @@ -329,6 +356,10 @@ main(int argc, char **argv) tor_fw_options.public_dir_port); } + // Initialize networking + if (network_init()) + exit(1); + // Initalize the various fw-helper backend helpers r = init_backends(&tor_fw_options, &backend_state); if (r) From 35c89be02b535e2951b695429bd9b255afb2a7b2 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 21 Jul 2011 16:34:48 +0100 Subject: [PATCH 05/31] Generalize process spawning so its test compiles (but fails) in Windows - pid, stdout/stderr_pipe now encapsulated in process_handle - read_all replaced by tor_read_all_from_process_stdin/stderr - waitpid replaced by tor_get_exit_code Untested on *nix --- src/common/util.c | 78 +++++++++++++++++++++++++++++++++++++------- src/common/util.h | 23 +++++++++++-- src/test/test_util.c | 33 ++++++++----------- 3 files changed, 101 insertions(+), 33 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index b95ee3a612..5bc7a8017b 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2978,14 +2978,15 @@ format_helper_exit_status(unsigned char child_state, int saved_errno, * -1. Some parts of this code are based on the POSIX subprocess module from * Python. */ -int -tor_spawn_background(const char *const filename, int *stdout_read, - int *stderr_read, const char **argv) +process_handle_t +tor_spawn_background(const char *const filename, const char **argv) { + process_handle_t process_handle; #ifdef MS_WINDOWS - (void) filename; (void) stdout_read; (void) stderr_read; (void) argv; + (void) filename; (void) argv; log_warn(LD_BUG, "not yet implemented on Windows."); - return -1; + process_handle.status = -1; + return process_handle; #else pid_t pid; int stdout_pipe[2]; @@ -3016,7 +3017,8 @@ tor_spawn_background(const char *const filename, int *stdout_read, log_warn(LD_GENERAL, "Failed to set up pipe for stdout communication with child process: %s", strerror(errno)); - return -1; + process_handle.status = -1; + return process_handle; } retval = pipe(stderr_pipe); @@ -3024,7 +3026,8 @@ tor_spawn_background(const char *const filename, int *stdout_read, log_warn(LD_GENERAL, "Failed to set up pipe for stderr communication with child process: %s", strerror(errno)); - return -1; + process_handle.status = -1; + return process_handle; } child_state = CHILD_STATE_MAXFD; @@ -3109,7 +3112,8 @@ tor_spawn_background(const char *const filename, int *stdout_read, (void) nbytes; _exit(255); - return -1; /* Never reached, but avoids compiler warning */ + process_handle.status = -1; + return process_handle; /* Never reached, but avoids compiler warning */ } /* In parent */ @@ -3120,11 +3124,12 @@ tor_spawn_background(const char *const filename, int *stdout_read, close(stdout_pipe[1]); close(stderr_pipe[0]); close(stderr_pipe[1]); - return -1; + process_handle.status = -1; + return process_handle; } /* Return read end of the pipes to caller, and close write end */ - *stdout_read = stdout_pipe[0]; + process_handle.stdout_pipe = stdout_pipe[0]; retval = close(stdout_pipe[1]); if (-1 == retval) { @@ -3135,7 +3140,7 @@ tor_spawn_background(const char *const filename, int *stdout_read, needs to know about the pid in order to reap it later */ } - *stderr_read = stderr_pipe[0]; + process_handle.stderr_pipe = stderr_pipe[0]; retval = close(stderr_pipe[1]); if (-1 == retval) { @@ -3146,7 +3151,56 @@ tor_spawn_background(const char *const filename, int *stdout_read, needs to know about the pid in order to reap it later */ } - return pid; + process_handle.status = 0; + process_handle.pid = pid; + return process_handle; +#endif +} + +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; +#else + int stat_loc; + + 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)); + return -1; + } + + if (!WIFEXITED(stat_loc)) { + log_warn(LD_GENERAL, "Process %d did not exit normally", process_handle.pid); + return -1; + } + + return WEXITSTATUS(stat_loc); +#endif // MS_WINDOWS +} + +ssize_t +tor_read_all_from_process_stdin(const process_handle_t process_handle, + char *buf, size_t count) +{ +#ifdef MS_WINDOWS + return -1; +#else + return read_all(process_handle.stdin_pipe, buf, count, 0); +#endif +} + +ssize_t +tor_read_all_from_process_stderr(const process_handle_t process_handle, + char *buf, size_t count) +{ +#ifdef MS_WINDOWS + return -1; +#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 6496c42db8..2cf57a125d 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -347,8 +347,27 @@ HANDLE load_windows_system_library(const TCHAR *library_name); #ifdef UTIL_PRIVATE /* Prototypes for private functions only used by util.c (and unit tests) */ -int tor_spawn_background(const char *const filename, int *stdout_read, - int *stderr_read, const char **argv); + +typedef struct process_handle_s { + int status; +#ifdef MS_WINDOWS + HANDLE stdout_pipe; + HANDLE stderr_pipe; + HANDLE pid; +#else + int stdout_pipe; + int stderr_pipe; + 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); +ssize_t tor_read_all_from_process_stdin(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); 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 c4769e6407..fce53b835e 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1376,42 +1376,38 @@ test_util_fgets_eagain(void *ptr) } #endif -#ifndef MS_WINDOWS /** 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) { - int stdout_pipe=-1, stderr_pipe=-1; - int retval, stat_loc; - pid_t pid; + int retval; ssize_t pos; + process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; /* Start the program */ - retval = tor_spawn_background(argv[0], &stdout_pipe, &stderr_pipe, argv); - tt_int_op(retval, >, 0); - tt_int_op(stdout_pipe, >, 0); - tt_int_op(stderr_pipe, >, 0); - pid = retval; + process_handle = tor_spawn_background(argv[0], argv); + + tt_int_op(process_handle.status, ==, 0); + tt_int_op(process_handle.stdout_pipe, >, 0); + tt_int_op(process_handle.stderr_pipe, >, 0); /* Check stdout */ - pos = read_all(stdout_pipe, stdout_buf, sizeof(stdout_buf) - 1, 0); + pos = tor_read_all_from_process_stdin(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); /* Check it terminated correctly */ - retval = waitpid(pid, &stat_loc, 0); - tt_int_op(retval, ==, pid); - tt_assert(WIFEXITED(stat_loc)); - tt_int_op(WEXITSTATUS(stat_loc), ==, expected_exit); - tt_assert(!WIFSIGNALED(stat_loc)); - tt_assert(!WIFSTOPPED(stat_loc)); + retval = tor_get_exit_code(process_handle); + tt_int_op(retval, ==, expected_exit); /* Check stderr */ - pos = read_all(stderr_pipe, stderr_buf, sizeof(stderr_buf) - 1, 0); + 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)); @@ -1447,7 +1443,6 @@ test_util_spawn_background_fail(void *ptr) run_util_spawn_background(argv, expected_out, expected_err, 255); } -#endif static void test_util_di_ops(void) @@ -1533,9 +1528,9 @@ struct testcase_t util_tests[] = { UTIL_TEST(exit_status, 0), #ifndef MS_WINDOWS UTIL_TEST(fgets_eagain, TT_SKIP), +#endif UTIL_TEST(spawn_background_ok, 0), UTIL_TEST(spawn_background_fail, 0), -#endif END_OF_TESTCASES }; From fec902dd6024dd170d151409387df5319b6e2ad0 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 21 Jul 2011 19:26:19 +0100 Subject: [PATCH 06/31] 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 5bc7a8017b..3d39f594f1 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 2cf57a125d..c111ba72f2 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 fce53b835e..28030b79b3 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; From 55a1cb53d6d1a8375afc12679b43901a977cf4b7 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Fri, 22 Jul 2011 15:57:56 +0100 Subject: [PATCH 07/31] Add code to read all from a handle, but this block forever See http://stackoverflow.com/questions/3722409/windows-child-process-with-redirected-input-and-output 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 3d39f594f1..e9b790990c 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 } +#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; + + if (count > SIZE_T_CEILING || count > SSIZE_T_MAX) + return -1; + + 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 - 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; - } + 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 ca52750c2f..cf49c5d31e 100644 --- a/src/test/test-child.c +++ b/src/test/test-child.c @@ -1,4 +1,11 @@ #include +#include "orconfig.h" +#ifdef MS_WINDOWS +#define WINDOWS_LEAN_AND_MEAN +#include +#else +#include +#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 28030b79b3..63c8ad8a93 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 From 2d5059e08e16ef704c3021e3ccdc2923be02c7cb Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Fri, 22 Jul 2011 21:12:00 +0100 Subject: [PATCH 08/31] Use PeekNamedPipe to avoid blocking ReadFile when there is nothing to read --- src/common/util.c | 50 +++++++++++++++++++++++++++++++++++--------- src/test/test_util.c | 4 ++-- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index e9b790990c..31d558644e 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) { - /* End of file */ - return 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; } - numread += bytes_read; + + 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 */ + break; + } + 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 63c8ad8a93..c6dc98dc6b 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 From c5796a8fb2fc70085c771d6296d4f29a7ddb68ae Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Sat, 23 Jul 2011 21:35:50 +0100 Subject: [PATCH 09/31] If hProcess is NULL, read_all_handle returns if it would block --- src/common/util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 31d558644e..2691b1f53b 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) From 99baa7e45cc65f0683912c59934679df554fa306 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Sat, 23 Jul 2011 14:32:39 +0100 Subject: [PATCH 10/31] Fix compilation on non-Windows platforms --- src/common/util.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 2691b1f53b..e63047b91e 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); From 5bf9890b3b18ad98f5ac601992bc08533ce9e209 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Sun, 24 Jul 2011 23:31:59 +0100 Subject: [PATCH 11/31] 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 e63047b91e..5a5f3b5fbd 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 c111ba72f2..2efe557857 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 cf49c5d31e..1b9c5e3d57 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 c6dc98dc6b..071fd04e87 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 }; From 7d015c886a3fc985ab767de01811d1591f0ac86a Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Thu, 18 Aug 2011 18:41:23 +0100 Subject: [PATCH 12/31] 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 5a5f3b5fbd..3769d06317 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 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 2efe557857..40fe305651 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 071fd04e87..6f875606ec 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 From 2ad336f999781db211d92332e657398829c8799c Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Fri, 19 Aug 2011 14:47:44 +0100 Subject: [PATCH 13/31] Link and build tor-fw-helper on Windows - Update configure script to test for libminiupnpc along with the libws2_32 and libiphlpapi libraries required by libminiupnpc - When building tor-fw-helper, link in libiphlpapi - Link in libminiupnpc statically becasue I could not get the DLL to link properly - Call WSAStartup before doing network operations - Fix up a compiler warning about uninitialized backend_state N.B. The changes to configure.in and Makefile.am will break on non- Windows platforms. --- configure.in | 2 +- 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 | 34 +++++++++++++++++++- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/configure.in b/configure.in index 4a89df6e04..1c45f186d5 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 #include #include ], diff --git a/src/tools/tor-fw-helper/Makefile.am b/src/tools/tor-fw-helper/Makefile.am index 77ff63fc36..39aabc7b20 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 = 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 18ca56394f..c4b14a84e2 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,9 @@ #include "orconfig.h" #ifdef MINIUPNPC +#ifdef MS_WINDOWS +#define STATICLIB +#endif #include #include #include diff --git a/src/tools/tor-fw-helper/tor-fw-helper.c b/src/tools/tor-fw-helper/tor-fw-helper.c index 20d60d7ba6..cb8e0cd9eb 100644 --- a/src/tools/tor-fw-helper/tor-fw-helper.c +++ b/src/tools/tor-fw-helper/tor-fw-helper.c @@ -13,6 +13,7 @@ * later date. */ +#include "orconfig.h" #include #include #include @@ -20,7 +21,10 @@ #include #include -#include "orconfig.h" +#ifdef MS_WINDOWS +#include +#endif + #include "tor-fw-helper.h" #ifdef NAT_PMP #include "tor-fw-helper-natpmp.h" @@ -219,6 +223,29 @@ tor_fw_add_dir_port(tor_fw_options_t *tor_fw_options, } } +/** Called before we make any calls to network-related functions. + * (Some operating systems require their network libraries to be + * initialized.) (from common/compat.c) */ +static int +network_init(void) +{ +#ifdef MS_WINDOWS + /* This silly exercise is necessary before windows will allow + * gethostbyname to work. */ + WSADATA WSAData; + int r; + r = WSAStartup(0x101, &WSAData); + if (r) { + fprintf(stderr, "E: Error initializing Windows network layer - code was %d", r); + return -1; + } + /* WSAData.iMaxSockets might show the max sockets we're allowed to use. + * We might use it to complain if we're trying to be a server but have + * too few sockets available. */ +#endif + return 0; +} + int main(int argc, char **argv) { @@ -229,6 +256,7 @@ main(int argc, char **argv) backends_t backend_state; memset(&tor_fw_options, 0, sizeof(tor_fw_options)); + memset(&backend_state, 0, sizeof(backend_state)); while (1) { int option_index = 0; @@ -329,6 +357,10 @@ main(int argc, char **argv) tor_fw_options.public_dir_port); } + // Initialize networking + if (network_init()) + exit(1); + // Initalize the various fw-helper backend helpers r = init_backends(&tor_fw_options, &backend_state); if (r) From 269c0b4633a5b41b2d37cf9c877fe37c4ea4a1f9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 19 Aug 2011 13:20:15 -0400 Subject: [PATCH 14/31] Only link ws2_32 and iphlapi on windows. This is a tweak for the tor-fw-helper port to windows. --- configure.in | 4 +++- src/tools/tor-fw-helper/Makefile.am | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/configure.in b/configure.in index 1c45f186d5..f9e98f0256 100644 --- a/configure.in +++ b/configure.in @@ -325,6 +325,7 @@ dnl Where do you live, libevent? And how do we call you? if test "$bwin32" = true; then TOR_LIB_WS32=-lws2_32 + TOR_LIB_IPHLAPI=-liphlapi # Some of the cargo-cults recommend -lwsock32 as well, but I don't # think it's actually necessary. TOR_LIB_GDI=-lgdi32 @@ -334,6 +335,7 @@ else fi AC_SUBST(TOR_LIB_WS32) AC_SUBST(TOR_LIB_GDI) +AC_SUBST(TOR_LIB_IPHLAPI) dnl We need to do this before we try our disgusting hack below. AC_CHECK_HEADERS([sys/types.h]) @@ -559,7 +561,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 -lws2_32 -liphlpapi], + TOR_SEARCH_LIBRARY(libminiupnpc, $trylibminiupnpcdir, [-lminiupnpc $TOR_LIB_WS32 $TOR_LIB_IPHLAPI], [#include #include #include ], diff --git a/src/tools/tor-fw-helper/Makefile.am b/src/tools/tor-fw-helper/Makefile.am index 39aabc7b20..127a000020 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 -liphlpapi +miniupnpc_ldadd = -lminiupnpc -lm @TOR_LIB_IPHLAPI@ miniupnpc_cppflags = @TOR_CPPFLAGS_libminiupnpc@ else miniupnpc_ldflags = From c5e74fc60db2b0c763d55f5bca8a32dc4bc05fe5 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 16:31:30 +0100 Subject: [PATCH 15/31] Appease "make check spaces" --- src/tools/tor-fw-helper/tor-fw-helper.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/tor-fw-helper/tor-fw-helper.c b/src/tools/tor-fw-helper/tor-fw-helper.c index cb8e0cd9eb..002239745a 100644 --- a/src/tools/tor-fw-helper/tor-fw-helper.c +++ b/src/tools/tor-fw-helper/tor-fw-helper.c @@ -236,7 +236,8 @@ network_init(void) int r; r = WSAStartup(0x101, &WSAData); if (r) { - fprintf(stderr, "E: Error initializing Windows network layer - code was %d", r); + fprintf(stderr, "E: Error initializing Windows network layer " + "- code was %d", r); return -1; } /* WSAData.iMaxSockets might show the max sockets we're allowed to use. From 850d8c9eb8a847d3d35d4e39acf2d84afe8dffa4 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 17:38:22 +0100 Subject: [PATCH 16/31] Correct reference to libiphlpapi from libiphlapi --- configure.in | 6 +++--- src/tools/tor-fw-helper/Makefile.am | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configure.in b/configure.in index f9e98f0256..26792a6677 100644 --- a/configure.in +++ b/configure.in @@ -325,7 +325,7 @@ dnl Where do you live, libevent? And how do we call you? if test "$bwin32" = true; then TOR_LIB_WS32=-lws2_32 - TOR_LIB_IPHLAPI=-liphlapi + TOR_LIB_IPHLPAPI=-liphlpapi # Some of the cargo-cults recommend -lwsock32 as well, but I don't # think it's actually necessary. TOR_LIB_GDI=-lgdi32 @@ -335,7 +335,7 @@ else fi AC_SUBST(TOR_LIB_WS32) AC_SUBST(TOR_LIB_GDI) -AC_SUBST(TOR_LIB_IPHLAPI) +AC_SUBST(TOR_LIB_IPHLPAPI) dnl We need to do this before we try our disgusting hack below. AC_CHECK_HEADERS([sys/types.h]) @@ -561,7 +561,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_LIB_WS32 $TOR_LIB_IPHLAPI], + TOR_SEARCH_LIBRARY(libminiupnpc, $trylibminiupnpcdir, [-lminiupnpc $TOR_LIB_WS32 $TOR_LIB_IPHLPAPI], [#include #include #include ], diff --git a/src/tools/tor-fw-helper/Makefile.am b/src/tools/tor-fw-helper/Makefile.am index 127a000020..8f64ad2ba3 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 @TOR_LIB_IPHLAPI@ +miniupnpc_ldadd = -lminiupnpc -lm @TOR_LIB_IPHLPAPI@ miniupnpc_cppflags = @TOR_CPPFLAGS_libminiupnpc@ else miniupnpc_ldflags = From a78ffd9e45a270b9979dcdc0d376f33bd95d7f09 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 17:55:40 +0100 Subject: [PATCH 17/31] Revert change to libminiupnpc search path This was supposed to fix an oddity with library searching in autoconf but it didn't actually fix anything. --- configure.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.in b/configure.in index e59f31b7a1..26792a6677 100644 --- a/configure.in +++ b/configure.in @@ -569,7 +569,7 @@ if test "$upnp" = "true"; then const char * minissdpdsock, int sameport);], [upnpDiscover(1, 0, 0, 0); exit(0);], [--with-libminiupnpc-dir], - [/local/lib/]) + [/usr/lib/]) fi AC_SYS_LARGEFILE From f46f6aabb4da8d7c185bf46cb05a9469ca1f11df Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 18:13:58 +0100 Subject: [PATCH 18/31] Fix some compiler warnings --- src/common/util.c | 8 ++++---- src/test/test_util.c | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 517cc3e499..48e48ce813 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3058,6 +3058,9 @@ tor_spawn_background(const char *const filename, const char **argv) HANDLE stderr_pipe_read = NULL; HANDLE stderr_pipe_write = NULL; + STARTUPINFO siStartInfo; + BOOL retval = FALSE; + SECURITY_ATTRIBUTES saAttr; smartlist_t *argv_list; char *joined_argv; @@ -3105,9 +3108,6 @@ tor_spawn_background(const char *const filename, const char **argv) 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); @@ -3592,7 +3592,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, #define TIME_TO_EXEC_FWHELPER_FAIL 60 #ifdef MS_WINDOWS - static process_handle_t child_handle = {0, NULL, NULL, {NULL}}; + static process_handle_t child_handle = {0, NULL, NULL, {NULL, NULL, 0, 0}}; #else static process_handle_t child_handle; #endif diff --git a/src/test/test_util.c b/src/test/test_util.c index 6f875606ec..40de76ac3b 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1493,6 +1493,8 @@ test_util_spawn_background_partial_read(void *ptr) process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; + (void)ptr; + /* Start the program */ process_handle = tor_spawn_background(argv[0], argv); tt_int_op(process_handle.status, ==, expected_status); From 1ad986335a5d2fc0c9952412d25037fb70226472 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 19:43:38 +0100 Subject: [PATCH 19/31] Tidy up subprocess code - Better error handling - Write description of functions - Don't assume non-negative process return values --- src/common/util.c | 109 +++++++++++++++++++++++++++++-------------- src/common/util.h | 3 +- src/test/test_util.c | 14 +++--- 3 files changed, 83 insertions(+), 43 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 48e48ce813..36b59662bc 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3070,6 +3070,7 @@ tor_spawn_background(const char *const filename, const char **argv) saAttr.bInheritHandle = TRUE; saAttr.lpSecurityDescriptor = NULL; + /* Assume failure to start process */ process_handle.status = -1; /* Set up pipe for stdout */ @@ -3083,6 +3084,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to configure pipe for stdout communication with child process: %s", format_win32_error(GetLastError())); + return process_handle; } /* Set up pipe for stderr */ @@ -3096,6 +3098,7 @@ tor_spawn_background(const char *const filename, const char **argv) log_warn(LD_GENERAL, "Failed to configure pipe for stderr communication with child process: %s", format_win32_error(GetLastError())); + return process_handle; } /* Create the child process */ @@ -3163,10 +3166,8 @@ 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; + /* Assume failure to start */ + 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 */ @@ -3180,7 +3181,6 @@ 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)); - process_handle.status = -1; return process_handle; } @@ -3189,7 +3189,6 @@ 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)); - process_handle.status = -1; return process_handle; } @@ -3276,7 +3275,6 @@ tor_spawn_background(const char *const filename, const char **argv) (void) nbytes; _exit(255); - process_handle.status = -1; return process_handle; /* Never reached, but avoids compiler warning */ } @@ -3288,7 +3286,6 @@ tor_spawn_background(const char *const filename, const char **argv) close(stdout_pipe[1]); close(stderr_pipe[0]); close(stderr_pipe[1]); - process_handle.status = -1; return process_handle; } @@ -3330,37 +3327,64 @@ 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.*/ int -tor_get_exit_code(const process_handle_t process_handle, int block) +tor_get_exit_code(const process_handle_t process_handle, + int block, int *exit_code) { #ifdef MS_WINDOWS - DWORD exit_code; - BOOL retval; + DWORD retval; + BOOL success; + 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; + /* Wait for the process to exit */ + 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())); + return -1; + } + } else { + retval = WaitForSingleObject(process_handle.pid.hProcess, 0); + if (WAIT_TIMEOUT == retval) { + /* Process has not exited */ + return -2; + } else if (retval != WAIT_OBJECT_0) { + log_warn(LD_GENERAL, "WaitForSingleObject() failed (%d): %s", + (int)retval, format_win32_error(GetLastError())); + return -1; + } + } + + if (exit_code != NULL) { + success = GetExitCodeProcess(process_handle.pid.hProcess, + (PDWORD)exit_code); + if (!success) { + log_warn(LD_GENERAL, "GetExitCodeProcess() failed: %s", + format_win32_error(GetLastError())); + return -1; } - 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; - } + return 0; #else int stat_loc; int retval; - retval = waitpid(process_handle.pid, &stat_loc, 0); - if (retval != process_handle.pid) { + retval = waitpid(process_handle.pid, &stat_loc, block?0:WNOHANG); + if (!block && 0 == retval) { + /* Process has not exited */ + return -2; + } else if (retval != process_handle.pid) { log_warn(LD_GENERAL, "waitpid() failed for PID %d: %s", process_handle.pid, strerror(errno)); return -1; @@ -3371,7 +3395,8 @@ tor_get_exit_code(const process_handle_t process_handle, int block) return -1; } - return WEXITSTATUS(stat_loc); + if (exit_code != NULL) + *exit_code = WEXITSTATUS(stat_loc); #endif // MS_WINDOWS } @@ -3457,6 +3482,9 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle, } #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. + */ static int log_from_handle(HANDLE *pipe, int severity) { @@ -3501,7 +3529,7 @@ log_from_handle(HANDLE *pipe, int severity) } log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start); } - return pos; + return 0; } #else @@ -3588,11 +3616,13 @@ 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 fails, 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 + * which corresponds to it not running on startup */ #ifdef MS_WINDOWS - static process_handle_t child_handle = {0, NULL, NULL, {NULL, NULL, 0, 0}}; + static process_handle_t child_handle; #else static process_handle_t child_handle; #endif @@ -3629,6 +3659,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, if (child_handle.status < 0) { log_warn(LD_GENERAL, "Failed to start port forwarding helper %s", filename); + time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL; return; } #ifdef MS_WINDOWS @@ -3647,7 +3678,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, #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 + /* If we got this far (on Windows), the process started */ retval = 0; #else stdout_status = log_from_pipe(child_handle.stdout_handle, @@ -3665,13 +3696,18 @@ 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) >= 0) { + else if (tor_get_exit_code(child_handle, 0, NULL) >= 0) { /* process has exited */ + /* 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? */ retval = 1; } #else else if (1 == stdout_status || 1 == stderr_status) - /* stdout or stderr was closed */ + /* stdout or stderr was closed, the process probably + * exited. It will be reaped by waitpid() in main.c */ + /* TODO: Do something with the process return value */ retval = 1; #endif else @@ -3682,13 +3718,14 @@ 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; } else { log_warn(LD_GENERAL, "Failed to read from port forwarding helper"); + child_handle.status = -1; } /* TODO: The child might not actually be finished (maybe it failed or closed stdout/stderr), so maybe we shouldn't start another? */ - child_handle.status = -1; } } } diff --git a/src/common/util.h b/src/common/util.h index d0ad8eb637..902233499f 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -372,7 +372,8 @@ 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, int block); +int tor_get_exit_code(const process_handle_t process_handle, + int block, int *exit_code); 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 40de76ac3b..3916b4c89a 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1382,7 +1382,7 @@ run_util_spawn_background(const char *argv[], const char *expected_out, const char *expected_err, int expected_exit, int expected_status) { - int retval; + int retval, exit_code; ssize_t pos; process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; @@ -1408,8 +1408,9 @@ 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, 1); - tt_int_op(retval, ==, expected_exit); + retval = tor_get_exit_code(process_handle, 1, &exit_code); + tt_int_op(retval, ==, 0); + tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0 /* Check stderr */ @@ -1488,7 +1489,7 @@ test_util_spawn_background_partial_read(void *ptr) const int expected_exit = 0; const int expected_status = 0; - int retval; + int retval, exit_code; ssize_t pos; process_handle_t process_handle; char stdout_buf[100], stderr_buf[100]; @@ -1531,8 +1532,9 @@ test_util_spawn_background_partial_read(void *ptr) #endif /* Check it terminated correctly */ - retval = tor_get_exit_code(process_handle, 1); - tt_int_op(retval, ==, expected_exit); + retval = tor_get_exit_code(process_handle, 1, &exit_code); + tt_int_op(retval, ==, 0); + tt_int_op(exit_code, ==, expected_exit); // TODO: Make test-child exit with something other than 0 /* Check stderr */ From 50504fc4cb1a626b180b786b261aa59c928e269f Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 22 Aug 2011 20:05:11 +0100 Subject: [PATCH 20/31] Fix test cases to handle MSYS style paths (/c/foo rather than c:/foo) Also fix test case to expect 1 on successfully spawning a subprocess --- src/test/test_util.c | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index 3916b4c89a..eaf5e0dd89 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1429,11 +1429,20 @@ run_util_spawn_background(const char *argv[], const char *expected_out, static void test_util_spawn_background_ok(void *ptr) { + char *filename=NULL; #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\nDONE\r\n"; const char *expected_err = "ERR\r\n"; + + if (argv[0][0] == '/') { + /* We have a fake path, e.g. /c/foo, make it c:/foo */ + filename = tor_strdup(argv[0]); + filename[0] = filename[1]; + filename[1] = ':'; + argv[0] = filename; + log_warn(LD_GENERAL, "%s", argv[0]); + } #else const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; const char *expected_out = "OUT\n--test\nSLEEPING\nDONE\n"; @@ -1442,7 +1451,8 @@ test_util_spawn_background_ok(void *ptr) (void)ptr; - run_util_spawn_background(argv, expected_out, expected_err, 0, 0); + run_util_spawn_background(argv, expected_out, expected_err, 0, 1); + tor_free(filename); } /** Check that failing to find the executable works as expected */ @@ -1461,7 +1471,7 @@ test_util_spawn_background_fail(void *ptr) "- 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; + const int expected_status = 1; #endif (void)ptr; @@ -1473,32 +1483,41 @@ test_util_spawn_background_fail(void *ptr) static void test_util_spawn_background_partial_read(void *ptr) { + char *filename=NULL; + const int expected_exit = 0; + const int expected_status = 1; + + int retval, exit_code; + ssize_t pos; + process_handle_t process_handle; + char stdout_buf[100], stderr_buf[100]; #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; + + if (argv[0][0] == '/') { + /* We have a fake path, e.g. /c/foo, make it c:/foo */ + filename = tor_strdup(argv[0]); + filename[0] = filename[1]; + filename[1] = ':'; + argv[0] = filename; + log_warn(LD_GENERAL, "%s", argv[0]); + } #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, exit_code; - ssize_t pos; - process_handle_t process_handle; - char stdout_buf[100], stderr_buf[100]; - (void)ptr; /* Start the program */ process_handle = tor_spawn_background(argv[0], argv); tt_int_op(process_handle.status, ==, expected_status); + tor_free(filename); /* Check stdout */ #ifdef MS_WINDOWS From 2efafdfe14411b800e9350e527955ca74f3c704b Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Tue, 23 Aug 2011 01:09:24 +0100 Subject: [PATCH 21/31] Fix compilation errors under *nix --- src/common/util.c | 9 ++++++--- src/common/util.h | 2 ++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 36b59662bc..f25dd43b60 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3071,6 +3071,7 @@ tor_spawn_background(const char *const filename, const char **argv) saAttr.lpSecurityDescriptor = NULL; /* Assume failure to start process */ + memset(&process_handle, 0, sizeof(process_handle)); process_handle.status = -1; /* Set up pipe for stdout */ @@ -3167,6 +3168,7 @@ 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; /* We do the strlen here because strlen() is not signal handler safe, @@ -3374,8 +3376,6 @@ tor_get_exit_code(const process_handle_t process_handle, return -1; } } - - return 0; #else int stat_loc; int retval; @@ -3398,6 +3398,8 @@ tor_get_exit_code(const process_handle_t process_handle, if (exit_code != NULL) *exit_code = WEXITSTATUS(stat_loc); #endif // MS_WINDOWS + + return 0; } #ifdef MS_WINDOWS @@ -3667,7 +3669,8 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port, "Started port forwarding helper (%s)", filename); #else log_info(LD_GENERAL, - "Started port forwarding helper (%s) with pid %d", filename, child_pid); + "Started port forwarding helper (%s) with pid %d", filename, + child_handle.pid); #endif } diff --git a/src/common/util.h b/src/common/util.h index 902233499f..b4ae3f8585 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -374,7 +374,9 @@ process_handle_t tor_spawn_background(const char *const filename, const char **argv); 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); +#endif 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, From 6304e088d039fe6b1475cff8b811d62cb3db0a9a Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Wed, 24 Aug 2011 19:56:38 +0100 Subject: [PATCH 22/31] Find test-child.exe by looking in same directory as test.exe --- src/test/test_util.c | 76 ++++++++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index eaf5e0dd89..d30cc11161 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1425,24 +1425,55 @@ 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) { - char *filename=NULL; #ifdef MS_WINDOWS - const char *argv[] = {BUILDDIR "/src/test/test-child.exe", "--test", NULL}; + const char *argv[] = {NULL, "--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; - if (argv[0][0] == '/') { - /* We have a fake path, e.g. /c/foo, make it c:/foo */ - filename = tor_strdup(argv[0]); - filename[0] = filename[1]; - filename[1] = ':'; - argv[0] = filename; - log_warn(LD_GENERAL, "%s", argv[0]); - } + /* 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"; @@ -1452,7 +1483,12 @@ 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 */ @@ -1483,7 +1519,6 @@ test_util_spawn_background_fail(void *ptr) static void test_util_spawn_background_partial_read(void *ptr) { - char *filename=NULL; const int expected_exit = 0; const int expected_status = 1; @@ -1492,21 +1527,18 @@ 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[] = {BUILDDIR "/src/test/test-child.exe", "--test", NULL}; + const char *argv[] = {NULL, "--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; - if (argv[0][0] == '/') { - /* We have a fake path, e.g. /c/foo, make it c:/foo */ - filename = tor_strdup(argv[0]); - filename[0] = filename[1]; - filename[1] = ':'; - argv[0] = filename; - log_warn(LD_GENERAL, "%s", argv[0]); - } + /* 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"; @@ -1565,7 +1597,11 @@ 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 From 476807211c30491a92344b0222274aefbc5b5e8a Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Wed, 24 Aug 2011 20:50:58 +0100 Subject: [PATCH 23/31] 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 f25dd43b60..8b9979cc46 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 d30cc11161..5d3f95c8ca 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 From 50b48c3ea7b9601c1ab29f786bb0d88eb4149474 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Wed, 24 Aug 2011 21:33:53 +0100 Subject: [PATCH 24/31] 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 8b9979cc46..c18458355d 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 Date: Wed, 24 Aug 2011 21:34:13 +0100 Subject: [PATCH 25/31] 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 c18458355d..d69d8f108c 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 2602628ab8..442001f489 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 5d3f95c8ca..cb98030ccf 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 */ From 3f0a197aad3cca6634e4eb63e8441e5507a6b77f Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Sun, 28 Aug 2011 23:35:02 +0100 Subject: [PATCH 26/31] 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 d69d8f108c..91b83db01b 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 442001f489..e0252658ad 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 cb98030ccf..40ab09813b 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 */ From f1ff65dfad800ed89e5b01c1c5b4b77c10a438b8 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 29 Aug 2011 00:30:18 +0100 Subject: [PATCH 27/31] 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 91b83db01b..371e09b8db 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 e0252658ad..d8c73703e2 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 40ab09813b..4568fdec1c 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 From 93792b5aa6806646674795504419f3e97862685c Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 29 Aug 2011 00:36:41 +0100 Subject: [PATCH 28/31] Add a sanity check --- src/common/util.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 371e09b8db..dd2d1110e9 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) { From bc97f410802d5b9c66bfba6aebeae1ecd70f8857 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Mon, 29 Aug 2011 12:26:55 +0100 Subject: [PATCH 29/31] 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 dd2d1110e9..87c6fb5c17 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; ichild_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 d8c73703e2..9cdd8cb399 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 4568fdec1c..9df7bc675d 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1554,6 +1554,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) { @@ -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 }; From da34360952c0fbbd8effc2789ed72b86c8045531 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Tue, 30 Aug 2011 14:55:51 +0100 Subject: [PATCH 30/31] 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 87c6fb5c17..aa344bc9c6 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", + 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 }; From d1dd9991cd636bafe7543aea4dbb18de69f26202 Mon Sep 17 00:00:00 2001 From: Steven Murdoch Date: Tue, 30 Aug 2011 15:02:28 +0100 Subject: [PATCH 31/31] Document limitation of log_from_handle with partial reads --- src/common/util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/common/util.c b/src/common/util.c index aa344bc9c6..76124ef06c 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)