commit aa360b255bc1c262486500655bac70c4f0f00118 Author: Alexander Færøy ahf@torproject.org 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; }