commit 5585cbd08f54f732c32feea276c1a47ec8446c5e Author: Alexander Færøy ahf@torproject.org Date: Wed Nov 28 21:55:04 2018 +0100

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 0e326f90e..e3cc67941 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 <b>exit_code</b> and our process can be - * found in <b>process</b>. */ -STATIC void + * found in <b>process</b>. 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 fbb720aac..ba8cbf710 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 7275d0a21..75bffe35b 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 <b>exit_code</b>. We mark the * process as no longer running and calls the <b>exit_callback</b> with - * information about the process termination. */ + * information about the process termination. The given <b>process</b> 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 cb5bccbf7..4b0fae425 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 e75328f3e..911bad393 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 3c809dfd2..00de8c949 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 3640b8668..17481097b 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 ad84127bb..b23492e97 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