[tor-commits] [tor/maint-0.4.0] Fix crash bug in PT subsystem.

[nickm@xxxxxxxxxxxxxx]

commit aa360b255bc1c262486500655bac70c4f0f00118
Author: Alexander Færøy <ahf@xxxxxxxxxxxxxx>
Date:   Tue Feb 26 15:26:33 2019 +0100

    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
---
 changes/bug29562                |  4 ++++
 src/feature/client/transports.c | 29 ++++++++++++++---------------
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/changes/bug29562 b/changes/bug29562
new file mode 100644
index 000000000..0621cd09a
--- /dev/null
+++ b/changes/bug29562
@@ -0,0 +1,4 @@
+  o Minor bugfixes (pluggable transports):
+    - Fix an assertion failure crash bug when a pluggable transport process is
+      terminated during the bootstrap phase. Fixes bug 29562; bugfix on
+      0.4.0.1-alpha.
diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c
index e24705516..e7ff3bf34 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;
 }
 



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits