[tor-commits] [tor/master] Change the Process exit_callback to return bool.
nickm at torproject.org
nickm at torproject.org
Tue Dec 18 18:36:43 UTC 2018
commit 5585cbd08f54f732c32feea276c1a47ec8446c5e
Author: Alexander Færøy <ahf at 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
More information about the tor-commits
mailing list