From 5585cbd08f54f732c32feea276c1a47ec8446c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20F=C3=A6r=C3=B8y?= Date: Wed, 28 Nov 2018 21:55:04 +0100 Subject: [PATCH] Change the Process exit_callback to return bool. This patch changes our process_t's exit_callback to return a boolean value. If the returned value is true, the process subsystem will call process_free() on the given process_t. See: https://bugs.torproject.org/28179 --- src/feature/client/transports.c | 11 +++++--- src/feature/client/transports.h | 2 +- src/lib/process/process.c | 14 +++++++--- src/lib/process/process.h | 4 +-- src/lib/process/process_win32.c | 48 +++++++++++++++++++++++++-------- src/lib/process/process_win32.h | 2 +- src/test/test_process.c | 5 ++-- src/test/test_process_slow.c | 5 ++-- 8 files changed, 65 insertions(+), 26 deletions(-) diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c index 0e326f90e9..e3cc679411 100644 --- a/src/feature/client/transports.c +++ b/src/feature/client/transports.c @@ -1759,8 +1759,9 @@ managed_proxy_stderr_callback(process_t *process, char *line, size_t size) /** Callback function that is called when our PT process terminates. The * process exit code can be found in exit_code and our process can be - * found in process. */ -STATIC void + * found in process. Returns true iff we want the process subsystem to + * free our process_t handle for us. */ +STATIC bool managed_proxy_exit_callback(process_t *process, process_exit_code_t exit_code) { tor_assert(process); @@ -1772,10 +1773,14 @@ managed_proxy_exit_callback(process_t *process, process_exit_code_t exit_code) /* We detach ourself from the MP (if we are attached) and free ourself. */ managed_proxy_t *mp = process_get_data(process); + /* If we are still attached to the process, it is probably because our PT + * process crashed before we got to call process_set_data(p, NULL); */ if (BUG(mp != NULL)) { + /* FIXME(ahf): Our process stopped without us having told it to stop + * (crashed). Should we restart it here? */ mp->process = NULL; process_set_data(process, NULL); } - process_free(process); + return true; } diff --git a/src/feature/client/transports.h b/src/feature/client/transports.h index fbb720aac6..ba8cbf7105 100644 --- a/src/feature/client/transports.h +++ b/src/feature/client/transports.h @@ -145,7 +145,7 @@ STATIC void free_execve_args(char **arg); STATIC void managed_proxy_stdout_callback(process_t *, char *, size_t); STATIC void managed_proxy_stderr_callback(process_t *, char *, size_t); -STATIC void managed_proxy_exit_callback(process_t *, process_exit_code_t); +STATIC bool managed_proxy_exit_callback(process_t *, process_exit_code_t); #endif /* defined(PT_PRIVATE) */ diff --git a/src/lib/process/process.c b/src/lib/process/process.c index 7275d0a21b..75bffe35b9 100644 --- a/src/lib/process/process.c +++ b/src/lib/process/process.c @@ -612,7 +612,8 @@ process_notify_event_stdin(process_t *process) /** This function is called by the Process backend when a given process have * terminated. The exit status code is passed in exit_code. We mark the * process as no longer running and calls the exit_callback with - * information about the process termination. */ + * information about the process termination. The given process is + * free'd iff the exit_callback returns true. */ void process_notify_event_exit(process_t *process, process_exit_code_t exit_code) { @@ -626,9 +627,14 @@ process_notify_event_exit(process_t *process, process_exit_code_t exit_code) process->exit_code = exit_code; /* Call our exit callback, if it exists. */ - if (process->exit_callback) { - process->exit_callback(process, exit_code); - } + bool free_process_handle = false; + + /* The exit callback will tell us if we should process_free() our handle. */ + if (process->exit_callback) + free_process_handle = process->exit_callback(process, exit_code); + + if (free_process_handle) + process_free(process); } /** This function is called whenever the Process backend have notified us that diff --git a/src/lib/process/process.h b/src/lib/process/process.h index cb5bccbf7b..4b0fae4250 100644 --- a/src/lib/process/process.h +++ b/src/lib/process/process.h @@ -56,8 +56,8 @@ typedef uint64_t process_pid_t; typedef void (*process_read_callback_t)(process_t *, char *, size_t); -typedef void (*process_exit_callback_t)(process_t *, - process_exit_code_t); +typedef bool +(*process_exit_callback_t)(process_t *, process_exit_code_t); void process_init(void); void process_free_all(void); diff --git a/src/lib/process/process_win32.c b/src/lib/process/process_win32.c index e75328f3e8..911bad3933 100644 --- a/src/lib/process/process_win32.c +++ b/src/lib/process/process_win32.c @@ -474,28 +474,47 @@ process_win32_timer_callback(periodic_timer_t *timer, void *data) tor_assert(timer == periodic_timer); tor_assert(data == NULL); - log_debug(LD_PROCESS, "Windows Process I/O timer ticked"); - /* Move the process into an alertable state. */ process_win32_trigger_completion_callbacks(); /* Check if our processes are still alive. */ - const smartlist_t *processes = process_get_all_processes(); - SMARTLIST_FOREACH(processes, process_t *, p, - process_win32_timer_test_process(p)); + /* Since the call to process_win32_timer_test_process() might call + * process_notify_event_exit() which again might call process_free() which + * updates the list of processes returned by process_get_all_processes() it + * is important here that we make sure to not touch the list of processes if + * the call to process_win32_timer_test_process() returns true. */ + bool done; + + do { + const smartlist_t *processes = process_get_all_processes(); + done = true; + + SMARTLIST_FOREACH_BEGIN(processes, process_t *, process) { + /* If process_win32_timer_test_process() returns true, it means that + * smartlist_remove() might have been called on the list returned by + * process_get_all_processes(). We start the loop over again until we + * have a succesful run over the entire list where the list was not + * modified. */ + if (process_win32_timer_test_process(process)) { + done = false; + break; + } + } SMARTLIST_FOREACH_END(process); + } while (! done); } /** Test whether a given process is still alive. Notify the Process subsystem - * if our process have died. */ -STATIC void + * if our process have died. Returns true iff the given process have + * terminated. */ +STATIC bool process_win32_timer_test_process(process_t *process) { tor_assert(process); /* No need to look at processes that don't claim they are running. */ if (process_get_status(process) != PROCESS_STATUS_RUNNING) - return; + return false; process_win32_t *win32_process = process_get_win32_process(process); BOOL ret = FALSE; @@ -508,12 +527,19 @@ process_win32_timer_test_process(process_t *process) if (! ret) { log_warn(LD_PROCESS, "GetExitCodeProcess() failed: %s", format_win32_error(GetLastError())); - return; + return false; } - /* Notify our process_t that our process have terminated. */ - if (exit_code != STILL_ACTIVE) + /* Notify our process_t that our process have terminated. Since our + * exit_callback might decide to process_free() our process handle it is very + * important that we do not touch the process_t after the call to + * process_notify_event_exit(). */ + if (exit_code != STILL_ACTIVE) { process_notify_event_exit(process, exit_code); + return true; + } + + return false; } /** Create a new overlapped named pipe. This function creates a new connected, diff --git a/src/lib/process/process_win32.h b/src/lib/process/process_win32.h index 3c809dfd23..00de8c949b 100644 --- a/src/lib/process/process_win32.h +++ b/src/lib/process/process_win32.h @@ -49,7 +49,7 @@ STATIC void process_win32_timer_start(void); STATIC void process_win32_timer_stop(void); STATIC bool process_win32_timer_running(void); STATIC void process_win32_timer_callback(periodic_timer_t *, void *); -STATIC void process_win32_timer_test_process(process_t *); +STATIC bool process_win32_timer_test_process(process_t *); /* I/O pipe handling. */ struct process_win32_handle_t; diff --git a/src/test/test_process.c b/src/test/test_process.c index 3640b86688..17481097be 100644 --- a/src/test/test_process.c +++ b/src/test/test_process.c @@ -125,7 +125,7 @@ process_stderr_callback(process_t *process, char *data, size_t size) return; } -static void +static bool process_exit_callback(process_t *process, process_exit_code_t exit_code) { tt_ptr_op(process, OP_NE, NULL); @@ -134,7 +134,8 @@ process_exit_callback(process_t *process, process_exit_code_t exit_code) process_data->exit_code = exit_code; done: - return; + /* Do not free up our process_t. */ + return false; } static void diff --git a/src/test/test_process_slow.c b/src/test/test_process_slow.c index ad84127bba..b23492e972 100644 --- a/src/test/test_process_slow.c +++ b/src/test/test_process_slow.c @@ -94,7 +94,7 @@ process_stderr_callback(process_t *process, char *data, size_t size) return; } -static void +static bool process_exit_callback(process_t *process, process_exit_code_t exit_code) { tt_ptr_op(process, OP_NE, NULL); @@ -106,7 +106,8 @@ process_exit_callback(process_t *process, process_exit_code_t exit_code) tor_shutdown_event_loop_and_exit(0); done: - return; + /* Do not free up our process_t. */ + return false; } #ifdef _WIN32