Merge branch 'maint-0.4.0' into maint-0.4.1

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.

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.

src/lib/err/backtrace.c

@@ -69,10 +69,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
@@ -128,7 +128,7 @@ log_backtrace_impl(int severity, log_domain_mask_t 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.");
@@ -198,13 +198,10 @@ static int trap_signals[] = { SIGSEGV, SIGILL, SIGFPE, SIGBUS, SIGSYS,
 /** 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 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));
@@ -263,13 +260,13 @@ void
 log_backtrace_impl(int severity, log_domain_mask_t 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
-install_bt_handler(const char *software)
+install_bt_handler(void)
 {
-  (void) software;
   return 0;
 }
 
@@ -284,6 +281,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)
@@ -291,10 +296,25 @@ 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);
   }
 
-  return install_bt_handler(version);
+  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();
 }
 
 /** Perform end-of-process cleanup for code that generates error messages on

src/lib/err/backtrace.h

@@ -24,6 +24,7 @@ void log_backtrace_impl(int severity, log_domain_mask_t domain,
 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)

src/lib/err/torerr.c

@@ -198,14 +198,21 @@ 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 ",
-                      file, ":", linebuf, ": ", expr, NULL);
+  tor_log_err_sigsafe("INTERNAL ERROR: Raw assertion failed in ",
+                      get_tor_backtrace_version(), " at ",
+                      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");
 }
 
 /**

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

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

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