Fix crash bug in PT subsystem.

This patch fixes a crash bug (assertion failure) in the PT subsystem
that could get triggered if the user cancels bootstrap via the UI in
TorBrowser. This would cause Tor to call `managed_proxy_destroy()` which
called `process_free()` after it had called `process_terminate()`. This
leads to a crash when the various process callbacks returns with data
after the `process_t` have been freed using `process_free()`.

We solve this issue by ensuring that everywhere we call
`process_terminate()` we make sure to detach the `managed_proxy_t` from
the `process_t` (by calling `process_set_data(process, NULL)`) and avoid
calling `process_free()` at all in the transports code. Instead we just
call `process_terminate()` and let the process exit callback in
`managed_proxy_exit_callback()` handle the `process_free()` call by
returning true to the process subsystem.

See: https://bugs.torproject.org/29562

1 files changed, 14 insertions, 15 deletions

diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c
index e247055164..e7ff3bf34a 100644
--- a/src/feature/client/transports.c
+++ b/src/feature/client/transports.c
@@ -713,10 +713,13 @@ managed_proxy_destroy(managed_proxy_t *mp,
   tor_free(mp->proxy_uri);
 
   /* do we want to terminate our process if it's still running? */
-  if (also_terminate_process && mp->process)
+  if (also_terminate_process && mp->process) {
+    /* Note that we do not call process_free(mp->process) here because we let
+     * the exit handler in managed_proxy_exit_callback() return `true` which
+     * makes the process subsystem deallocate the process_t. */
+    process_set_data(mp->process, NULL);
     process_terminate(mp->process);
-
-  process_free(mp->process);
+  }
 
   tor_free(mp);
 }
@@ -1823,6 +1826,9 @@ managed_proxy_stdout_callback(process_t *process,
 
   managed_proxy_t *mp = process_get_data(process);
 
+  if (BUG(mp == NULL))
+    return;
+
   handle_proxy_line(line, mp);
 
   if (proxy_configuration_finished(mp)) {
@@ -1846,6 +1852,9 @@ managed_proxy_stderr_callback(process_t *process,
 
   managed_proxy_t *mp = process_get_data(process);
 
+  if (BUG(mp == NULL))
+    return;
+
   log_warn(LD_PT, "Managed proxy at '%s' reported: %s", mp->argv[0], line);
 }
 
@@ -1862,18 +1871,8 @@ managed_proxy_exit_callback(process_t *process, process_exit_code_t exit_code)
           "Pluggable Transport process terminated with status code %" PRIu64,
           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);
-  }
-
+  /* Returning true here means that the process subsystem will take care of
+   * calling process_free() on our process_t. */
   return true;
 }