From 4ed03965a5a412bf58540e678f48f6c331ad30d9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 9 Apr 2014 13:45:27 -0400 Subject: New waitpid-handler functions to run callbacks when a child exits. Also, move 'procmon' into libor_event library, since it uses libevent. --- src/common/include.am | 9 ++- src/common/procmon.c | 1 + src/common/util_process.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++ src/common/util_process.h | 25 ++++++++ src/or/main.c | 4 +- src/or/or.h | 3 - 6 files changed, 191 insertions(+), 8 deletions(-) create mode 100644 src/common/util_process.c create mode 100644 src/common/util_process.h diff --git a/src/common/include.am b/src/common/include.am index 7b2465cd8..c9dcedd52 100644 --- a/src/common/include.am +++ b/src/common/include.am @@ -57,9 +57,9 @@ LIBOR_A_SOURCES = \ src/common/log.c \ src/common/memarea.c \ src/common/mempool.c \ - src/common/procmon.c \ src/common/util.c \ src/common/util_codedigest.c \ + src/common/util_process.c \ src/common/sandbox.c \ src/ext/csiphash.c \ $(libor_extra_source) @@ -72,7 +72,9 @@ LIBOR_CRYPTO_A_SOURCES = \ src/common/tortls.c \ $(libcrypto_extra_source) -LIBOR_EVENT_A_SOURCES = src/common/compat_libevent.c +LIBOR_EVENT_A_SOURCES = \ + src/common/compat_libevent.c \ + src/common/procmon.c src_common_libor_a_SOURCES = $(LIBOR_A_SOURCES) src_common_libor_crypto_a_SOURCES = $(LIBOR_CRYPTO_A_SOURCES) @@ -111,7 +113,8 @@ COMMONHEADERS = \ src/common/torint.h \ src/common/torlog.h \ src/common/tortls.h \ - src/common/util.h + src/common/util.h \ + src/common/util_process.h noinst_HEADERS+= $(COMMONHEADERS) diff --git a/src/common/procmon.c b/src/common/procmon.c index 0a49689e3..7c9b7c3c8 100644 --- a/src/common/procmon.c +++ b/src/common/procmon.c @@ -162,6 +162,7 @@ tor_validate_process_specifier(const char *process_spec, return parse_process_specifier(process_spec, &ppspec, msg); } +/* XXXX we should use periodic_timer_new() for this stuff */ #ifdef HAVE_EVENT2_EVENT_H #define PERIODIC_TIMER_FLAGS EV_PERSIST #else diff --git a/src/common/util_process.c b/src/common/util_process.c new file mode 100644 index 000000000..8ccf7f38f --- /dev/null +++ b/src/common/util_process.c @@ -0,0 +1,157 @@ +/* Copyright (c) 2003-2004, Roger Dingledine + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file util_process.c + * \brief utility functions for launching processes and checking their + * status. These functions are kept separately from procmon so that they + * won't require linking against libevent. + **/ + +#include "orconfig.h" + +#ifdef HAVE_SYS_TYPES_H +#include +#endif +#ifdef HAVE_SYS_WAIT_H +#include +#endif + +#include "compat.h" +#include "util.h" +#include "torlog.h" +#include "util_process.h" +#include "ht.h" + +/* ================================================== */ +/* Convenience structures for handlers for waitpid(). + * + * The tor_process_monitor*() code above doesn't use them, since it is for + * monitoring a non-child process. + */ + +#ifndef _WIN32 + +/** Mapping from a PID to a userfn/userdata pair. */ +struct waitpid_callback_t { + HT_ENTRY(waitpid_callback_t) node; + pid_t pid; + + void (*userfn)(int, void *userdata); + void *userdata; + + unsigned running; +}; + +static INLINE unsigned int +process_map_entry_hash_(const waitpid_callback_t *ent) +{ + return (unsigned) ent->pid; +} + +static INLINE unsigned int +process_map_entries_eq_(const waitpid_callback_t *a, const waitpid_callback_t *b) +{ + return a->pid == b->pid; +} + +static HT_HEAD(process_map, waitpid_callback_t) process_map = HT_INITIALIZER(); + +HT_PROTOTYPE(process_map, waitpid_callback_t, node, process_map_entry_hash_, + process_map_entries_eq_); +HT_GENERATE(process_map, waitpid_callback_t, node, process_map_entry_hash_, + process_map_entries_eq_, 0.6, malloc, realloc, free); + +/** + * Begin monitoring the child pid pid to see if we get a SIGCHLD for + * it. If we eventually do, call fn, passing it the exit status (as + * yielded by waitpid) and the pointer arg. + * + * To cancel this, or clean up after it has triggered, call + * clear_waitpid_callback(). + */ +waitpid_callback_t * +set_waitpid_callback(pid_t pid, void (*fn)(int, void *), void *arg) +{ + waitpid_callback_t *old_ent; + waitpid_callback_t *ent = tor_malloc_zero(sizeof(waitpid_callback_t)); + ent->pid = pid; + ent->userfn = fn; + ent->userdata = arg; + ent->running = 1; + + old_ent = HT_REPLACE(process_map, &process_map, ent); + if (old_ent) { + log_warn(LD_BUG, "Replaced a waitpid monitor on pid %u. That should be " + "impossible.", (unsigned) pid); + old_ent->running = 0; + } + + return ent; +} + +/** + * Cancel a waitpid_callback_t, or clean up after one has triggered. Releases + * all storage held by ent. + */ +void +clear_waitpid_callback(waitpid_callback_t *ent) +{ + waitpid_callback_t *old_ent; + if (ent == NULL) + return; + + if (ent->running) { + old_ent = HT_REMOVE(process_map, &process_map, ent); + if (old_ent != ent) { + log_warn(LD_BUG, "Couldn't remove waitpid monitor for pid %u.", + (unsigned) ent->pid); + return; + } + } + + tor_free(ent); +} + +/** Helper: find the callack for pid; if there is one, run it, + * reporting the exit status as status. */ +static void +notify_waitpid_callback_by_pid(pid_t pid, int status) +{ + waitpid_callback_t search, *ent; + + search.pid = pid; + ent = HT_REMOVE(process_map, &process_map, &search); + if (!ent || !ent->running) { + log_info(LD_GENERAL, "Child process %u has exited; no callback was " + "registered", (unsigned)pid); + return; + } + + log_info(LD_GENERAL, "Child process %u has exited; running callback.", + (unsigned)pid); + + ent->running = 0; + ent->userfn(status, ent->userdata); +} + +/** Use waitpid() to wait for all children that have exited, and invoke any + * callbacks registered for them. */ +void +notify_pending_waitpid_callbacks(void) +{ + /* I was going to call this function reap_zombie_children(), but + * that makes it sound way more exciting than it really is. */ + pid_t child; + int status = 0; + + while ((child = waitpid(-1, &status, WNOHANG)) > 0) { + notify_waitpid_callback_by_pid(child, status); + status = 0; /* should be needless */ + } +} + +#endif + diff --git a/src/common/util_process.h b/src/common/util_process.h new file mode 100644 index 000000000..877702c68 --- /dev/null +++ b/src/common/util_process.h @@ -0,0 +1,25 @@ +/* Copyright (c) 2011-2013, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +/** + * \file util_process.h + * \brief Headers for util_process.c + **/ + +#ifndef TOR_UTIL_PROCESS_H +#define TOR_UTIL_PROCESS_H + +#ifndef _WIN32 +/** A callback structure waiting for us to get a SIGCHLD informing us that a + * PID has been closed. Created by set_waitpid_callback. Cancelled or cleaned- + * up from clear_waitpid_callback(). Do not access outside of the main thread; + * do not access from inside a signal handler. */ +typedef struct waitpid_callback_t waitpid_callback_t; + +waitpid_callback_t *set_waitpid_callback(pid_t pid, + void (*fn)(int, void *), void *arg); +void clear_waitpid_callback(waitpid_callback_t *ent); +void notify_pending_waitpid_callbacks(void); +#endif + +#endif diff --git a/src/or/main.c b/src/or/main.c index 6713d8036..2ca717459 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -54,6 +54,7 @@ #include "routerparse.h" #include "statefile.h" #include "status.h" +#include "util_process.h" #include "ext_orport.h" #ifdef USE_DMALLOC #include @@ -2097,8 +2098,7 @@ process_signal(uintptr_t sig) break; #ifdef SIGCHLD case SIGCHLD: - while (waitpid(-1,NULL,WNOHANG) > 0) ; /* keep reaping until no more - zombies */ + notify_pending_waitpid_callbacks(); break; #endif case SIGNEWNYM: { diff --git a/src/or/or.h b/src/or/or.h index aeaeb8e6a..9586034d1 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -42,9 +42,6 @@ #include /* FreeBSD needs this to know what version it is */ #endif #include "torint.h" -#ifdef HAVE_SYS_WAIT_H -#include -#endif #ifdef HAVE_SYS_FCNTL_H #include #endif -- cgit v1.2.3 From f8344c2d28be2489c8abadd694b5b96fe18efc02 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 10 Apr 2014 11:06:10 -0400 Subject: Use waitpid code to learn when a controlled process dies This lets us avoid sending SIGTERM to something that has already died, since we realize it has already died, and is a fix for the unix version of #8746. --- src/common/util.c | 46 +++++++++++++++++++++++++++++++++++++++++++--- src/common/util.h | 9 ++++++++- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 56235aa66..0a1410139 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -26,6 +26,7 @@ #include "address.h" #include "sandbox.h" #include "backtrace.h" +#include "util_process.h" #ifdef _WIN32 #include @@ -3642,7 +3643,10 @@ tor_terminate_process(process_handle_t *process_handle) return 0; } #else /* Unix */ - return kill(process_handle->pid, SIGTERM); + if (process_handle->waitpid_cb) { + /* We haven't got a waitpid yet, so we can just kill off the process. */ + return kill(process_handle->pid, SIGTERM); + } #endif return -1; @@ -3691,6 +3695,23 @@ process_handle_new(void) return out; } +#ifndef _WIN32 +/** Invoked when a process that we've launched via tor_spawn_background() has + * been found to have terminated. + */ +static void +process_handle_waitpid_cb(int status, void *arg) +{ + process_handle_t *process_handle = arg; + + process_handle->waitpid_exit_status = status; + clear_waitpid_callback(process_handle->waitpid_cb); + if (process_handle->status == PROCESS_STATUS_RUNNING) + process_handle->status = PROCESS_STATUS_NOTRUNNING; + process_handle->waitpid_cb = 0; +} +#endif + /** * @name child-process states * @@ -4007,6 +4028,10 @@ tor_spawn_background(const char *const filename, const char **argv, strerror(errno)); } + process_handle->waitpid_cb = set_waitpid_callback(pid, + process_handle_waitpid_cb, + process_handle); + process_handle->stderr_pipe = stderr_pipe[0]; retval = close(stderr_pipe[1]); @@ -4071,6 +4096,8 @@ tor_process_handle_destroy,(process_handle_t *process_handle, if (process_handle->stderr_handle) fclose(process_handle->stderr_handle); + + clear_waitpid_callback(process_handle->waitpid_cb); #endif memset(process_handle, 0x0f, sizeof(process_handle_t)); @@ -4088,7 +4115,7 @@ tor_process_handle_destroy,(process_handle_t *process_handle, * 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, +tor_get_exit_code(process_handle_t *process_handle, int block, int *exit_code) { #ifdef _WIN32 @@ -4128,7 +4155,20 @@ tor_get_exit_code(const process_handle_t *process_handle, int stat_loc; int retval; - retval = waitpid(process_handle->pid, &stat_loc, block?0:WNOHANG); + if (process_handle->waitpid_cb) { + /* We haven't processed a SIGCHLD yet. */ + retval = waitpid(process_handle->pid, &stat_loc, block?0:WNOHANG); + if (retval == process_handle->pid) { + clear_waitpid_callback(process_handle->waitpid_cb); + process_handle->waitpid_cb = NULL; + process_handle->waitpid_exit_status = stat_loc; + } + } else { + /* We already got a SIGCHLD for this process, and handled it. */ + retval = process_handle->pid; + stat_loc = process_handle->waitpid_exit_status; + } + if (!block && 0 == retval) { /* Process has not exited */ return PROCESS_EXIT_RUNNING; diff --git a/src/common/util.h b/src/common/util.h index 18dc20639..97367a9a7 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -446,6 +446,7 @@ void set_environment_variable_in_smartlist(struct smartlist_t *env_vars, #define PROCESS_STATUS_ERROR -1 #ifdef UTIL_PRIVATE +struct waitpid_callback_t; /** Structure to represent the state of a process with which Tor is * communicating. The contents of this structure are private to util.c */ struct process_handle_t { @@ -461,6 +462,12 @@ struct process_handle_t { FILE *stdout_handle; FILE *stderr_handle; pid_t pid; + /** If the process has not given us a SIGCHLD yet, this has the + * waitpid_callback_t that gets invoked once it has. Otherwise this + * contains NULL. */ + struct waitpid_callback_t *waitpid_cb; + /** The exit status reported by waitpid. */ + int waitpid_exit_status; #endif // _WIN32 }; #endif @@ -469,7 +476,7 @@ struct process_handle_t { #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 tor_get_exit_code(process_handle_t *process_handle, int block, int *exit_code); int tor_split_lines(struct smartlist_t *sl, char *buf, int len); #ifdef _WIN32 -- cgit v1.2.3 From 34f8723dc784142b30d92bbbdeb37089ae7a3bc5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 10 Apr 2014 11:16:42 -0400 Subject: On Windows, terminate processes by handle, not pid When we create a process yourself with CreateProcess, we get a handle to the process in the PROCESS_INFO output structure. But instead of using that handle, we were manually looking up a _new_ handle based on the process ID, which is a poor idea, since the process ID might refer to a new process later on, but the handle can't. --- src/common/util.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 0a1410139..f79e72bea 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3629,13 +3629,7 @@ tor_terminate_process(process_handle_t *process_handle) { #ifdef _WIN32 if (tor_get_exit_code(process_handle, 0, NULL) == PROCESS_EXIT_RUNNING) { - HANDLE handle; - /* If the signal is outside of what GenerateConsoleCtrlEvent can use, - attempt to open and terminate the process. */ - handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, - process_handle->pid.dwProcessId); - if (!handle) - return -1; + HANDLE handle = process_handle->pid.hProcess; if (!TerminateProcess(handle, 0)) return -1; -- cgit v1.2.3 From e2e588175eac4ebe8fb47c0540954d0f78525cce Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Apr 2014 12:48:46 -0400 Subject: New testing-only tor_sleep_msec function In the unit tests I want to loop with a delay, but I want less than a 1 second delay. This, sadly, requires compatibility code. --- configure.ac | 2 ++ src/common/compat.c | 26 ++++++++++++++++++++++++++ src/common/compat.h | 4 ++++ 3 files changed, 32 insertions(+) diff --git a/configure.ac b/configure.ac index c0c818757..c662a9fc1 100644 --- a/configure.ac +++ b/configure.ac @@ -355,6 +355,7 @@ AC_CHECK_FUNCS( sysconf \ sysctl \ uname \ + usleep \ vasprintf \ _vscprintf ) @@ -885,6 +886,7 @@ AC_CHECK_HEADERS( sys/param.h \ sys/prctl.h \ sys/resource.h \ + sys/select.h \ sys/socket.h \ sys/sysctl.h \ sys/syslimits.h \ diff --git a/src/common/compat.c b/src/common/compat.c index 1ba264a0c..1c460b6ae 100644 --- a/src/common/compat.c +++ b/src/common/compat.c @@ -114,6 +114,12 @@ /* Only use the linux prctl; the IRIX prctl is totally different */ #include #endif +#ifdef TOR_UNIT_TESTS +#if !defined(HAVE_USLEEP) && defined(HAVE_SYS_SELECT_H) +/* as fallback implementation for tor_sleep_msec */ +#include +#endif +#endif #include "torlog.h" #include "util.h" @@ -3450,3 +3456,23 @@ get_total_system_memory(size_t *mem_out) return -1; } +#ifdef TOR_UNIT_TESTS +/** Delay for msec milliseconds. Only used in tests. */ +void +tor_sleep_msec(int msec) +{ +#ifdef _WIN32 + Sleep(msec); +#elif defined(HAVE_USLEEP) + sleep(msec / 1000); + /* Some usleep()s hate sleeping more than 1 sec */ + usleep((msec % 1000) * 1000); +#elif defined(HAVE_SYS_SELECT_H) + struct timeval tv = { msec / 1000, (msec % 1000) * 1000}; + select(0, NULL, NULL, NULL, &tv); +#else + sleep(CEIL_DIV(msec, 1000)); +#endif +} +#endif + diff --git a/src/common/compat.h b/src/common/compat.h index 314b1aa00..d723448fd 100644 --- a/src/common/compat.h +++ b/src/common/compat.h @@ -744,6 +744,10 @@ char *format_win32_error(DWORD err); #endif +#ifdef TOR_UNIT_TESTS +void tor_sleep_msec(int msec); +#endif + #ifdef COMPAT_PRIVATE #if !defined(HAVE_SOCKETPAIR) || defined(_WIN32) || defined(TOR_UNIT_TESTS) #define NEED_ERSATZ_SOCKETPAIR -- cgit v1.2.3 From e3833193af11dc4252e90565c3919889cc4130b3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Apr 2014 12:50:00 -0400 Subject: More unit tests for process spawning Try killing a running process; try noticing that a process has exited without checking its output; verify that waitpid_cb (when present) is set to NULL when you would expect it to be. --- src/test/test-child.c | 40 +++++++++++----- src/test/test_util.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 146 insertions(+), 18 deletions(-) diff --git a/src/test/test-child.c b/src/test/test-child.c index ef10fbb92..756782e70 100644 --- a/src/test/test-child.c +++ b/src/test/test-child.c @@ -9,6 +9,13 @@ #else #include #endif +#include + +#ifdef _WIN32 +#define SLEEP(sec) Sleep((sec)*1000) +#else +#define SLEEP(sec) sleep(sec) +#endif /** Trivial test program which prints out its command line arguments so we can * check if tor_spawn_background() works */ @@ -16,27 +23,38 @@ int main(int argc, char **argv) { int i; + int delay = 1; + int fast = 0; + + if (argc > 1) { + if (!strcmp(argv[1], "--hang")) { + delay = 60; + } else if (!strcmp(argv[1], "--fast")) { + fast = 1; + delay = 0; + } + } fprintf(stdout, "OUT\n"); fprintf(stderr, "ERR\n"); for (i = 1; i < argc; i++) fprintf(stdout, "%s\n", argv[i]); - fprintf(stdout, "SLEEPING\n"); + if (!fast) + 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 _WIN32 - Sleep(1000); -#else - sleep(1); -#endif + if (!fast) + SLEEP(1); fprintf(stdout, "DONE\n"); -#ifdef _WIN32 - Sleep(1000); -#else - sleep(1); -#endif + fflush(stdout); + if (fast) + return 0; + + while (--delay) { + SLEEP(1); + } return 0; } diff --git a/src/test/test_util.c b/src/test/test_util.c index 6d6b6dbdf..029e88eb6 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -14,6 +14,7 @@ #include "test.h" #include "mempool.h" #include "memarea.h" +#include "util_process.h" #ifdef _WIN32 #include @@ -2529,6 +2530,10 @@ test_util_fgets_eagain(void *ptr) } #endif +#ifdef _WIN32 +#define notify_pending_waitpid_callbacks() STMT_NIL +#endif + /** Helper function for testing tor_spawn_background */ static void run_util_spawn_background(const char *argv[], const char *expected_out, @@ -2548,19 +2553,28 @@ run_util_spawn_background(const char *argv[], const char *expected_out, status = tor_spawn_background(argv[0], argv, NULL, &process_handle); #endif + notify_pending_waitpid_callbacks(); + test_eq(expected_status, status); - if (status == PROCESS_STATUS_ERROR) + if (status == PROCESS_STATUS_ERROR) { + tt_ptr_op(process_handle, ==, NULL); return; + } test_assert(process_handle != NULL); test_eq(expected_status, process_handle->status); +#ifndef _WIN32 + notify_pending_waitpid_callbacks(); + tt_ptr_op(process_handle->waitpid_cb, !=, NULL); +#endif + #ifdef _WIN32 test_assert(process_handle->stdout_pipe != INVALID_HANDLE_VALUE); test_assert(process_handle->stderr_pipe != INVALID_HANDLE_VALUE); #else - test_assert(process_handle->stdout_pipe > 0); - test_assert(process_handle->stderr_pipe > 0); + test_assert(process_handle->stdout_pipe >= 0); + test_assert(process_handle->stderr_pipe >= 0); #endif /* Check stdout */ @@ -2571,12 +2585,19 @@ run_util_spawn_background(const char *argv[], const char *expected_out, test_eq(strlen(expected_out), pos); test_streq(expected_out, stdout_buf); + notify_pending_waitpid_callbacks(); + /* Check it terminated correctly */ retval = tor_get_exit_code(process_handle, 1, &exit_code); test_eq(PROCESS_EXIT_EXITED, retval); test_eq(expected_exit, exit_code); // TODO: Make test-child exit with something other than 0 +#ifndef _WIN32 + notify_pending_waitpid_callbacks(); + tt_ptr_op(process_handle->waitpid_cb, ==, NULL); +#endif + /* Check stderr */ pos = tor_read_all_from_process_stderr(process_handle, stderr_buf, sizeof(stderr_buf) - 1); @@ -2585,6 +2606,8 @@ run_util_spawn_background(const char *argv[], const char *expected_out, test_streq(expected_err, stderr_buf); test_eq(strlen(expected_err), pos); + notify_pending_waitpid_callbacks(); + done: if (process_handle) tor_process_handle_destroy(process_handle, 1); @@ -2607,7 +2630,7 @@ test_util_spawn_background_ok(void *ptr) (void)ptr; run_util_spawn_background(argv, expected_out, expected_err, 0, - PROCESS_STATUS_RUNNING); + PROCESS_STATUS_RUNNING); } /** Check that failing to find the executable works as expected */ @@ -2637,13 +2660,13 @@ test_util_spawn_background_fail(void *ptr) "ERR: Failed to spawn background process - code %s\n", code); run_util_spawn_background(argv, expected_out, expected_err, 255, - expected_status); + expected_status); } /** Test that reading from a handle returns a partial read rather than * blocking */ static void -test_util_spawn_background_partial_read(void *ptr) +test_util_spawn_background_partial_read_impl(int exit_early) { const int expected_exit = 0; const int expected_status = PROCESS_STATUS_RUNNING; @@ -2668,7 +2691,16 @@ test_util_spawn_background_partial_read(void *ptr) int eof = 0; #endif int expected_out_ctr; - (void)ptr; + + + if (exit_early) { + argv[1] = "--hang"; +#ifdef _WIN32 + expected_out[0] = "OUT\r\n--hang\r\nSLEEPING\r\n"; +#else + expected_out[0] = "OUT\n--hang\nSLEEPING\n"; +#endif + } /* Start the program */ #ifdef _WIN32 @@ -2704,6 +2736,12 @@ test_util_spawn_background_partial_read(void *ptr) expected_out_ctr++; } + if (exit_early) { + tor_process_handle_destroy(process_handle, 1); + process_handle = NULL; + goto done; + } + /* The process should have exited without writing more */ #ifdef _WIN32 pos = tor_read_all_handle(process_handle->stdout_pipe, stdout_buf, @@ -2741,6 +2779,76 @@ test_util_spawn_background_partial_read(void *ptr) tor_process_handle_destroy(process_handle, 1); } +static void +test_util_spawn_background_partial_read(void *arg) +{ + (void)arg; + test_util_spawn_background_partial_read_impl(0); +} + +static void +test_util_spawn_background_exit_early(void *arg) +{ + (void)arg; + test_util_spawn_background_partial_read_impl(1); +} + +static void +test_util_spawn_background_waitpid_notify(void *arg) +{ + int retval, exit_code; + process_handle_t *process_handle=NULL; + int status; + int ms_timer; + +#ifdef _WIN32 + const char *argv[] = {"test-child.exe", "--fast", NULL}; +#else + const char *argv[] = {BUILDDIR "/src/test/test-child", "--fast", NULL}; +#endif + + (void) arg; + +#ifdef _WIN32 + status = tor_spawn_background(NULL, argv, NULL, &process_handle); +#else + status = tor_spawn_background(argv[0], argv, NULL, &process_handle); +#endif + + tt_int_op(status, ==, PROCESS_STATUS_RUNNING); + tt_ptr_op(process_handle, !=, NULL); + + /* We're not going to look at the stdout/stderr output this time. Instead, + * we're testing whether notify_pending_waitpid_calbacks() can report the + * process exit (on unix) and/or whether tor_get_exit_code() can notice it + * (on windows) */ + +#ifndef _WIN32 + ms_timer = 30*1000; + tt_ptr_op(process_handle->waitpid_cb, !=, NULL); + while (process_handle->waitpid_cb && ms_timer > 0) { + tor_sleep_msec(100); + ms_timer -= 100; + notify_pending_waitpid_callbacks(); + } + tt_int_op(ms_timer, >, 0); + tt_ptr_op(process_handle->waitpid_cb, ==, NULL); +#endif + + ms_timer = 30*1000; + while (((retval = tor_get_exit_code(process_handle, 0, &exit_code)) + == PROCESS_EXIT_RUNNING) && ms_timer > 0) { + tor_sleep_msec(100); + ms_timer -= 100; + } + tt_int_op(ms_timer, >, 0); + + tt_int_op(retval, ==, PROCESS_EXIT_EXITED); + + done: + tor_process_handle_destroy(process_handle, 1); +} + /** * Test for format_hex_number_sigsafe() */ @@ -3682,6 +3790,8 @@ 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(spawn_background_exit_early, 0), + UTIL_TEST(spawn_background_waitpid_notify, 0), UTIL_TEST(format_hex_number, 0), UTIL_TEST(format_dec_number, 0), UTIL_TEST(join_win_cmdline, 0), -- cgit v1.2.3 From a5c092b34b1505051071e05a54353225dd6cba5e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Apr 2014 13:00:54 -0400 Subject: refactor win/nix handling for test_spawn_background*() Instead of having a #if ... for every function, just define TEST_CHILD to the right patch and EOL to the expected line terminator. --- src/test/test_util.c | 58 ++++++++++++++++++++++------------------------------ 1 file changed, 24 insertions(+), 34 deletions(-) diff --git a/src/test/test_util.c b/src/test/test_util.c index 029e88eb6..656b77b1a 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2530,8 +2530,17 @@ test_util_fgets_eagain(void *ptr) } #endif +#ifndef BUILDDIR +#define BUILDDIR "." +#endif + #ifdef _WIN32 #define notify_pending_waitpid_callbacks() STMT_NIL +#define TEST_CHILD "test-child.exe" +#define EOL "\r\n" +#else +#define TEST_CHILD (BUILDDIR "/src/test/test-child") +#define EOL "\n" #endif /** Helper function for testing tor_spawn_background */ @@ -2617,15 +2626,9 @@ run_util_spawn_background(const char *argv[], const char *expected_out, static void test_util_spawn_background_ok(void *ptr) { -#ifdef _WIN32 - 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"; -#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\n"; -#endif + const char *argv[] = {TEST_CHILD, "--test", NULL}; + const char *expected_out = "OUT"EOL "--test"EOL "SLEEPING"EOL "DONE" EOL; + const char *expected_err = "ERR"EOL; (void)ptr; @@ -2637,9 +2640,6 @@ test_util_spawn_background_ok(void *ptr) static void test_util_spawn_background_fail(void *ptr) { -#ifndef BUILDDIR -#define BUILDDIR "." -#endif const char *argv[] = {BUILDDIR "/src/test/no-such-file", "--test", NULL}; const char *expected_err = ""; char expected_out[1024]; @@ -2676,30 +2676,21 @@ test_util_spawn_background_partial_read_impl(int exit_early) process_handle_t *process_handle=NULL; int status; char stdout_buf[100], stderr_buf[100]; -#ifdef _WIN32 - 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"; -#else - const char *argv[] = {BUILDDIR "/src/test/test-child", "--test", NULL}; - const char *expected_out[] = { "OUT\n--test\nSLEEPING\n", - "DONE\n", + + const char *argv[] = {TEST_CHILD, "--test", NULL}; + const char *expected_out[] = { "OUT" EOL "--test" EOL "SLEEPING" EOL, + "DONE" EOL, NULL }; - const char *expected_err = "ERR\n"; + const char *expected_err = "ERR" EOL; + +#ifndef _WIN32 int eof = 0; #endif int expected_out_ctr; - if (exit_early) { argv[1] = "--hang"; -#ifdef _WIN32 - expected_out[0] = "OUT\r\n--hang\r\nSLEEPING\r\n"; -#else - expected_out[0] = "OUT\n--hang\nSLEEPING\n"; -#endif + expected_out[0] = "OUT"EOL "--hang"EOL "SLEEPING" EOL; } /* Start the program */ @@ -2801,11 +2792,7 @@ test_util_spawn_background_waitpid_notify(void *arg) int status; int ms_timer; -#ifdef _WIN32 - const char *argv[] = {"test-child.exe", "--fast", NULL}; -#else - const char *argv[] = {BUILDDIR "/src/test/test-child", "--fast", NULL}; -#endif + const char *argv[] = {TEST_CHILD, "--fast", NULL}; (void) arg; @@ -2849,6 +2836,9 @@ test_util_spawn_background_waitpid_notify(void *arg) tor_process_handle_destroy(process_handle, 1); } +#undef TEST_CHILD +#undef EOL + /** * Test for format_hex_number_sigsafe() */ -- cgit v1.2.3 From e07d328457b26babe89abdcc1d5a550ee8273462 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Apr 2014 13:13:38 -0400 Subject: changes file for 8746 --- changes/bug8746 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug8746 diff --git a/changes/bug8746 b/changes/bug8746 new file mode 100644 index 000000000..b6e52ca43 --- /dev/null +++ b/changes/bug8746 @@ -0,0 +1,4 @@ + o Major bugfixes: + - When managing pluggable transports, use OS notification facilities to + learn if they have crashed, and do not attempt to kill any process + that has already exited. Fix for bug 8746; bugfix on 0.2.3.6-alpha. -- cgit v1.2.3