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

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

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) */
 

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

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

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,

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;

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

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