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

4 files changed, 50 insertions, 18 deletions

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 <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 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;