December 2019 - tor-commits - lists.torproject.org

[tor/maint-0.4.2] Add test to check if the exit callback is called in process_t upon process_exec() failures.
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit ee8db8a2eb7a5d345d16d02bade593fa6db2877e Author: Alexander Færøy <ahf(a)torproject.org> Date: Thu Oct 17 16:56:21 2019 +0200 Add test to check if the exit callback is called in process_t upon process_exec() failures. This patch adds a test to check for whether the exit callback is always called when process_exec() fails, both on Windows and Unix. See: https://bugs.torproject.org/31810 --- src/test/test_process_slow.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/test/test_process_slow.c b/src/test/test_process_slow.c index 1322d7b83..d269a068e 100644 --- a/src/test/test_process_slow.c +++ b/src/test/test_process_slow.c @@ -328,8 +328,38 @@ test_callbacks_terminate(void *arg) process_free(process); } +static void +test_nonexistent_executable(void *arg) +{ + (void)arg; + + /* Process callback data. */ + process_data_t *process_data = process_data_new(); + + /* Setup our process. */ + process_t *process = process_new("binary-does-not-exist"); + process_set_data(process, process_data); + process_set_exit_callback(process, process_exit_callback); + + /* Run our process. */ + process_exec(process); + + /* Start our main loop. */ + run_main_loop(process_data); + + /* Ensure that the exit callback was actually called even though the binary + * did not exist. + */ + tt_assert(process_data->did_exit); + + done: + process_data_free(process_data); + process_free(process); +} + struct testcase_t slow_process_tests[] = { { "callbacks", test_callbacks, 0, NULL, NULL }, { "callbacks_terminate", test_callbacks_terminate, 0, NULL, NULL }, + { "nonexistent_executable", test_nonexistent_executable, 0, NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/maint-0.4.1] backtrace: Log the Tor backtrace version whenever we log a backtrace
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit 743bc0028be46e92a46b3b5e38f1e50ee69ff284 Author: teor <teor(a)torproject.org> Date: Fri Aug 30 21:15:54 2019 +1000 backtrace: Log the Tor backtrace version whenever we log a backtrace Previously, we just logged it in the crash handler. Part of 31571. --- src/lib/err/backtrace.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index 8cca3c37e..c34eb6dfa 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -127,7 +127,7 @@ log_backtrace_impl(int severity, int domain, const char *msg, depth = backtrace(cb_buf, MAX_DEPTH); symbols = backtrace_symbols(cb_buf, (int)depth); - logger(severity, domain, "%s. Stack trace:", msg); + logger(severity, domain, "%s: %s. Stack trace:", bt_version, msg); if (!symbols) { /* LCOV_EXCL_START -- we can't provoke this. */ logger(severity, domain, " Unable to generate backtrace."); @@ -240,7 +240,8 @@ void log_backtrace_impl(int severity, int domain, const char *msg, tor_log_fn logger) { - logger(severity, domain, "%s. (Stack trace not available)", msg); + logger(severity, domain, "%s: %s. (Stack trace not available)", + bt_version, msg); } static int

1 0

[tor/maint-0.4.1] backtrace: Check the return values of snprintf() and strncpy()
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit f311d0676caad1c61a4dbf8fffbeb79003866903 Author: teor <teor(a)torproject.org> Date: Tue Sep 3 15:50:37 2019 +1000 backtrace: Check the return values of snprintf() and strncpy() We can't use strlcat() or strlcpy() in torerr, because they are defined in string/compat_string.h on some platforms, and string uses torerr. Part of 31571. --- src/lib/err/backtrace.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index a4d8269c6..8606f4217 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -276,11 +276,23 @@ configure_backtrace_handler(const char *tor_version) char version[128] = "Tor\0"; if (tor_version) { - snprintf(version, sizeof(version), "Tor %s", tor_version); + int snp_rv = 0; + /* We can't use strlcat() here, because it is defined in + * string/compat_string.h on some platforms, and string uses torerr. */ + snp_rv = snprintf(version, sizeof(version), "Tor %s", tor_version); + /* It's safe to call raw_assert() here, because raw_assert() does not + * call configure_backtrace_handler(). */ + raw_assert(snp_rv < (int)sizeof(version)); + raw_assert(snp_rv >= 0); } - strncpy(bt_version, version, sizeof(bt_version) - 1); + char *str_rv = NULL; + /* We can't use strlcpy() here, see the note about strlcat() above. */ + str_rv = strncpy(bt_version, version, sizeof(bt_version) - 1); + /* We must terminate bt_version, then raw_assert(), because raw_assert() + * uses bt_version. */ bt_version[sizeof(bt_version) - 1] = 0; + raw_assert(str_rv == bt_version); return install_bt_handler(); }

1 0

[tor/maint-0.4.1] torerr: Log the Tor backtrace version when a raw assertion fails
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit 65a1d86491d56c77048d01e0b4fc6b933a13795c Author: teor <teor(a)torproject.org> Date: Fri Aug 30 21:17:07 2019 +1000 torerr: Log the Tor backtrace version when a raw assertion fails Part of 31571. --- src/lib/err/backtrace.c | 8 ++++++++ src/lib/err/backtrace.h | 1 + src/lib/err/torerr.c | 3 ++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index c34eb6dfa..a4d8269c6 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -261,6 +261,14 @@ dump_stack_symbols_to_error_fds(void) } #endif /* defined(NO_BACKTRACE_IMPL) */ +/** Return the tor version used for error messages on crashes. + * Signal-safe: returns a pointer to a static array. */ +const char * +get_tor_backtrace_version(void) +{ + return bt_version; +} + /** Set up code to handle generating error messages on crashes. */ int configure_backtrace_handler(const char *tor_version) diff --git a/src/lib/err/backtrace.h b/src/lib/err/backtrace.h index 9b313261e..48b41fca0 100644 --- a/src/lib/err/backtrace.h +++ b/src/lib/err/backtrace.h @@ -21,6 +21,7 @@ void log_backtrace_impl(int severity, int domain, const char *msg, int configure_backtrace_handler(const char *tor_version); void clean_up_backtrace_handler(void); void dump_stack_symbols_to_error_fds(void); +const char *get_tor_backtrace_version(void); #define log_backtrace(sev, dom, msg) \ log_backtrace_impl((sev), (dom), (msg), tor_log) diff --git a/src/lib/err/torerr.c b/src/lib/err/torerr.c index 54acf722a..88b19b732 100644 --- a/src/lib/err/torerr.c +++ b/src/lib/err/torerr.c @@ -144,7 +144,8 @@ tor_raw_assertion_failed_msg_(const char *file, int line, const char *expr, { char linebuf[16]; format_dec_number_sigsafe(line, linebuf, sizeof(linebuf)); - tor_log_err_sigsafe("INTERNAL ERROR: Raw assertion failed at ", + tor_log_err_sigsafe("INTERNAL ERROR: Raw assertion failed in ", + get_tor_backtrace_version(), " at ", file, ":", linebuf, ": ", expr, NULL); if (msg) { tor_log_err_sigsafe_write(msg);

1 0

[tor/maint-0.4.1] torerr: Try harder to flush raw assert messages before process termination
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit c55591825fedb0fe1db92fab7b654ccc15ad50d3 Author: teor <teor(a)torproject.org> Date: Fri Aug 30 21:17:56 2019 +1000 torerr: Try harder to flush raw assert messages before process termination Some platforms (macOS, maybe others?) can swallow the last write before an abort. This issue is probably caused by a race condition between write buffer cache flushing, and process termination. So we write an extra newline, to make sure that the message always gets through. Fixes bug 31571; bugfix on 0.3.5.1-alpha. --- changes/bug31571 | 7 +++++++ src/lib/err/torerr.c | 8 +++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/changes/bug31571 b/changes/bug31571 new file mode 100644 index 000000000..86de3537b --- /dev/null +++ b/changes/bug31571 @@ -0,0 +1,7 @@ + o Minor bugfixes (error handling): + - Report the tor version whenever an assertion fails. Previously, we only + reported the Tor version on some crashes, and some non-fatal assertions. + Fixes bug 31571; bugfix on 0.3.5.1-alpha. + - On abort, try harder to flush the output buffers of log messages. On + some platforms (macOS), log messages can be discarded when the process + terminates. Fixes bug 31571; bugfix on 0.3.5.1-alpha. diff --git a/src/lib/err/torerr.c b/src/lib/err/torerr.c index 88b19b732..6b5224273 100644 --- a/src/lib/err/torerr.c +++ b/src/lib/err/torerr.c @@ -146,13 +146,19 @@ tor_raw_assertion_failed_msg_(const char *file, int line, const char *expr, format_dec_number_sigsafe(line, linebuf, sizeof(linebuf)); tor_log_err_sigsafe("INTERNAL ERROR: Raw assertion failed in ", get_tor_backtrace_version(), " at ", - file, ":", linebuf, ": ", expr, NULL); + file, ":", linebuf, ": ", expr, "\n", NULL); if (msg) { tor_log_err_sigsafe_write(msg); tor_log_err_sigsafe_write("\n"); } dump_stack_symbols_to_error_fds(); + + /* Some platforms (macOS, maybe others?) can swallow the last write before an + * abort. This issue is probably caused by a race condition between write + * buffer cache flushing, and process termination. So we write an extra + * newline, to make sure that the message always gets through. */ + tor_log_err_sigsafe_write("\n"); } /* As format_{hex,dex}_number_sigsafe, but takes a <b>radix</b> argument

1 0

[tor/maint-0.4.1] backtrace: Always set a backtrace Tor version
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit e2a7d08aa7c51fa903d5294628612a13b4c0d6e0 Author: teor <teor(a)torproject.org> Date: Fri Aug 30 21:12:52 2019 +1000 backtrace: Always set a backtrace Tor version We want to report the tor version, even on platforms that don't have backtrace support (like Android). This commit stores the backtrace Tor version, regardless of USE_BACKTRACE. Preparation for 31571. --- src/lib/err/backtrace.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/lib/err/backtrace.c b/src/lib/err/backtrace.c index 1d1b3bcfa..8cca3c37e 100644 --- a/src/lib/err/backtrace.c +++ b/src/lib/err/backtrace.c @@ -68,10 +68,10 @@ // Redundant with util.h, but doing it here so we can avoid that dependency. #define raw_free free -#ifdef USE_BACKTRACE /** Version of Tor to report in backtrace messages. */ static char bt_version[128] = ""; +#ifdef USE_BACKTRACE /** Largest stack depth to try to dump. */ #define MAX_DEPTH 256 /** Static allocation of stack to dump. This is static so we avoid stack @@ -193,15 +193,12 @@ dump_stack_symbols_to_error_fds(void) /** Install signal handlers as needed so that when we crash, we produce a * useful stack trace. Return 0 on success, -errno on failure. */ static int -install_bt_handler(const char *software) +install_bt_handler(void) { int trap_signals[] = { SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS, SIGIO, -1 }; int i, rv=0; - strncpy(bt_version, software, sizeof(bt_version) - 1); - bt_version[sizeof(bt_version) - 1] = 0; - struct sigaction sa; memset(&sa, 0, sizeof(sa)); @@ -247,9 +244,8 @@ log_backtrace_impl(int severity, int domain, const char *msg, } static int -install_bt_handler(const char *software) +install_bt_handler(void) { - (void) software; return 0; } @@ -274,7 +270,10 @@ configure_backtrace_handler(const char *tor_version) snprintf(version, sizeof(version), "Tor %s", tor_version); } - return install_bt_handler(version); + strncpy(bt_version, version, sizeof(bt_version) - 1); + bt_version[sizeof(bt_version) - 1] = 0; + + return install_bt_handler(); } /** Perform end-of-process cleanup for code that generates error messages on

1 0

[tor/maint-0.4.1] Ensure that the exit callback is called if CreateProcessA() fails on Windows.
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit cbed6ff8430ee7f7d1002965ebf784628735c234 Author: Alexander Færøy <ahf(a)torproject.org> Date: Thu Oct 17 16:47:04 2019 +0200 Ensure that the exit callback is called if CreateProcessA() fails on Windows. This patch fixes an issue where the exit handler is not called for the given process_t in case CreateProcessA() fails. This could, for example, happen if the user tries to execute a binary that does not exist. See: https://bugs.torproject.org/31810 --- src/lib/process/process_win32.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/lib/process/process_win32.c b/src/lib/process/process_win32.c index ddbe76bfd..1b1de6ad5 100644 --- a/src/lib/process/process_win32.c +++ b/src/lib/process/process_win32.c @@ -234,6 +234,24 @@ process_win32_exec(process_t *process) CloseHandle(stdin_pipe_read); CloseHandle(stdin_pipe_write); + /* In the Unix backend, we do not get an error in the Tor process when a + * child process fails to spawn its target executable since we need to + * first do the fork() call in the Tor process and then the child process + * is responsible for doing the call to execve(). + * + * This means that the user of the process_exec() API must check for + * whether it returns PROCESS_STATUS_ERROR, which will rarely happen on + * Unix, but will happen for error cases on Windows where it does not + * happen on Unix. For example: when the target executable does not exist + * on the file system. + * + * To have somewhat feature compatibility between the Unix and the Windows + * backend, we here notify the process_t owner that the process have exited + * (even though it never managed to run) to ensure that the exit callback + * is executed. + */ + process_notify_event_exit(process, 0); + return PROCESS_STATUS_ERROR; }

1 0

[tor/maint-0.4.1] Merge branch 'maint-0.3.5' into maint-0.4.0
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit 92fb0990675424cd04eb0b39b4ae157ac7f5385b Merge: 46057ec5a aee966cb0 Author: teor <teor(a)torproject.org> Date: Thu Dec 5 10:13:53 2019 +1000 Merge branch 'maint-0.3.5' into maint-0.4.0 changes/bug31571 | 7 +++++++ src/lib/err/backtrace.c | 42 +++++++++++++++++++++++++++++++----------- src/lib/err/backtrace.h | 1 + src/lib/err/torerr.c | 11 +++++++++-- 4 files changed, 48 insertions(+), 13 deletions(-)

1 0

[tor/maint-0.4.1] Handle errors from execve() in the Unix process backend more gracefully.
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit 9915b8f0bc3e6a1b6c8413fbb87c41137ea89104 Author: Alexander Færøy <ahf(a)torproject.org> Date: Thu Oct 17 16:39:05 2019 +0200 Handle errors from execve() in the Unix process backend more gracefully. This patch removes a call to tor_assert_unreached() after execve() failed. This assertion leads to the child process emitting a stack trace on its standard output, which makes the error harder for the user to demystify, since they think it is an internal error in Tor instead of "just" being a "no such file or directory" error. The process will now instead output "Error from child process: X" where X is the stringified version of the errno value. See: https://bugs.torproject.org/31810 --- changes/bug31810 | 4 ++++ src/lib/process/process_unix.c | 9 +-------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/changes/bug31810 b/changes/bug31810 new file mode 100644 index 000000000..628d12f09 --- /dev/null +++ b/changes/bug31810 @@ -0,0 +1,4 @@ + o Minor bugfixes (process management): + - Remove assertion in the Unix process backend. This assertion would trigger + when a new process is spawned where the executable is not found leading to + a stack trace from the child process. Fixes bug 31810; bugfix on 0.4.0.1-alpha. diff --git a/src/lib/process/process_unix.c b/src/lib/process/process_unix.c index 790ab897e..b102afff4 100644 --- a/src/lib/process/process_unix.c +++ b/src/lib/process/process_unix.c @@ -253,22 +253,15 @@ process_unix_exec(process_t *process) process_environment_t *env = process_get_environment(process); /* Call the requested program. */ - retval = execve(argv[0], argv, env->unixoid_environment_block); + execve(argv[0], argv, env->unixoid_environment_block); /* If we made it here it is because execve failed :-( */ - if (-1 == retval) - fprintf(stderr, "Call to execve() failed: %s", strerror(errno)); - tor_free(argv); process_environment_free(env); - tor_assert_unreached(); - error: - /* LCOV_EXCL_START */ fprintf(stderr, "Error from child process: %s", strerror(errno)); _exit(1); - /* LCOV_EXCL_STOP */ } /* We are in the parent process. */

1 0

[tor/maint-0.4.1] Add test to check if the exit callback is called in process_t upon process_exec() failures.
by teor＠torproject.org 05 Dec '19

05 Dec '19

commit ee8db8a2eb7a5d345d16d02bade593fa6db2877e Author: Alexander Færøy <ahf(a)torproject.org> Date: Thu Oct 17 16:56:21 2019 +0200 Add test to check if the exit callback is called in process_t upon process_exec() failures. This patch adds a test to check for whether the exit callback is always called when process_exec() fails, both on Windows and Unix. See: https://bugs.torproject.org/31810 --- src/test/test_process_slow.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/test/test_process_slow.c b/src/test/test_process_slow.c index 1322d7b83..d269a068e 100644 --- a/src/test/test_process_slow.c +++ b/src/test/test_process_slow.c @@ -328,8 +328,38 @@ test_callbacks_terminate(void *arg) process_free(process); } +static void +test_nonexistent_executable(void *arg) +{ + (void)arg; + + /* Process callback data. */ + process_data_t *process_data = process_data_new(); + + /* Setup our process. */ + process_t *process = process_new("binary-does-not-exist"); + process_set_data(process, process_data); + process_set_exit_callback(process, process_exit_callback); + + /* Run our process. */ + process_exec(process); + + /* Start our main loop. */ + run_main_loop(process_data); + + /* Ensure that the exit callback was actually called even though the binary + * did not exist. + */ + tt_assert(process_data->did_exit); + + done: + process_data_free(process_data); + process_free(process); +} + struct testcase_t slow_process_tests[] = { { "callbacks", test_callbacks, 0, NULL, NULL }, { "callbacks_terminate", test_callbacks_terminate, 0, NULL, NULL }, + { "nonexistent_executable", test_nonexistent_executable, 0, NULL, NULL }, END_OF_TESTCASES };

1 0