Merge remote-tracking branch 'tor-github/pr/1723/head' into maint-0.4.3

8 files changed, 46 insertions, 60 deletions

diff --git a/changes/bug33087 b/changes/bug33087
new file mode 100644
index 0000000000..ab6df58cc6
--- /dev/null
+++ b/changes/bug33087
@@ -0,0 +1,7 @@
+  o Minor bugfixes (logging):
+    - Stop closing stderr and stdout during shutdown. Closing these file
+      descriptors can hide sanitiser logs.
+      Fixes bug 33087; bugfix on 0.4.1.6.
+    - Flush stderr, stdout, and file logs during shutdown, if supported by the
+      OS. This change helps make sure that any final logs are recorded.
+      Fixes bug 33087; bugfix on 0.4.1.6.
diff --git a/configure.ac b/configure.ac
index 27cfda1be3..5af9d9862e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -649,6 +649,7 @@ AC_CHECK_FUNCS(
 	explicit_bzero \
 	timingsafe_memcmp \
 	flock \
+	fsync \
 	ftime \
 	get_current_dir_name \
 	getaddrinfo \
diff --git a/src/lib/err/torerr.c b/src/lib/err/torerr.c
index 92ef80e56a..2de75c0be4 100644
--- a/src/lib/err/torerr.c
+++ b/src/lib/err/torerr.c
@@ -151,29 +151,27 @@ tor_log_reset_sigsafe_err_fds(void)
 }
 
 /**
- * Close the list of fds that get errors from inside a signal handler or
+ * Flush the list of fds that get errors from inside a signal handler or
  * other emergency condition. These fds are shared with the logging code:
- * closing them flushes the log buffers, and prevents any further logging.
+ * flushing them also flushes the log buffers.
  *
- * This function closes stderr, so it should only be called immediately before
- * process shutdown.
+ * This function is safe to call during signal handlers.
  */
 void
-tor_log_close_sigsafe_err_fds(void)
+tor_log_flush_sigsafe_err_fds(void)
 {
+  /* If we don't have fsync() in unistd.h, we can't flush the logs. */
+#ifdef HAVE_FSYNC
   int n_fds, i;
   const int *fds = NULL;
 
   n_fds = tor_log_get_sigsafe_err_fds(&fds);
   for (i = 0; i < n_fds; ++i) {
-    /* tor_log_close_sigsafe_err_fds_on_error() is called on error and on
-     * shutdown, so we can't log or take any useful action if close()
-     * fails. */
-    (void)close(fds[i]);
+    /* This function is called on error and on shutdown, so we don't log, or
+     * take any other action, if fsync() fails. */
+    (void)fsync(fds[i]);
   }
-
-  /* Don't even try logging, we've closed all the log fds. */
-  tor_log_set_sigsafe_err_fds(NULL, 0);
+#endif /* defined(HAVE_FSYNC) */
 }
 
 /**
@@ -217,13 +215,13 @@ tor_raw_assertion_failed_msg_(const char *file, int line, const char *expr,
 
 /**
  * Call the abort() function to kill the current process with a fatal
- * error. But first, close the raw error file descriptors, so error messages
+ * error. But first, flush the raw error file descriptors, so error messages
  * are written before process termination.
  **/
 void
 tor_raw_abort_(void)
 {
-  tor_log_close_sigsafe_err_fds();
+  tor_log_flush_sigsafe_err_fds();
   abort();
 }
 
diff --git a/src/lib/err/torerr.h b/src/lib/err/torerr.h
index f5b36eef9c..ce1b049c47 100644
--- a/src/lib/err/torerr.h
+++ b/src/lib/err/torerr.h
@@ -40,7 +40,7 @@ void tor_log_err_sigsafe(const char *m, ...);
 int tor_log_get_sigsafe_err_fds(const int **out);
 void tor_log_set_sigsafe_err_fds(const int *fds, int n);
 void tor_log_reset_sigsafe_err_fds(void);
-void tor_log_close_sigsafe_err_fds(void);
+void tor_log_flush_sigsafe_err_fds(void);
 void tor_log_sigsafe_err_set_granularity(int ms);
 
 void tor_raw_abort_(void) ATTR_NORETURN;
diff --git a/src/lib/err/torerr_sys.c b/src/lib/err/torerr_sys.c
index aa49ba36bd..46fc853550 100644
--- a/src/lib/err/torerr_sys.c
+++ b/src/lib/err/torerr_sys.c
@@ -27,11 +27,9 @@ subsys_torerr_initialize(void)
 static void
 subsys_torerr_shutdown(void)
 {
-  /* Stop handling signals with backtraces, then close the logs. */
+  /* Stop handling signals with backtraces, then flush the logs. */
   clean_up_backtrace_handler();
-  /* We can't log any log messages after this point: we've closed all the log
-   * fds, including stdio. */
-  tor_log_close_sigsafe_err_fds();
+  tor_log_flush_sigsafe_err_fds();
 }
 
 const subsys_fns_t sys_torerr = {
diff --git a/src/lib/log/log.c b/src/lib/log/log.c
index 75f8f79da2..eb81515746 100644
--- a/src/lib/log/log.c
+++ b/src/lib/log/log.c
@@ -665,24 +665,15 @@ tor_log_update_sigsafe_err_fds(void)
   const logfile_t *lf;
   int found_real_stderr = 0;
 
-  /* log_fds and err_fds contain matching entries: log_fds are the fds used by
-   * the log module, and err_fds are the fds used by the err module.
-   * For stdio logs, the log_fd and err_fd values are identical,
-   * and the err module closes the fd on shutdown.
-   * For file logs, the err_fd is a dup() of the log_fd,
-   * and the log and err modules both close their respective fds on shutdown.
-   * (Once all fds representing a file are closed, the underlying file is
-   * closed.)
-   */
-  int log_fds[TOR_SIGSAFE_LOG_MAX_FDS];
-  int err_fds[TOR_SIGSAFE_LOG_MAX_FDS];
+  /* The fds are the file descriptors of tor's stdout, stderr, and file
+   * logs. The log and err modules flush these fds during their shutdowns. */
+  int fds[TOR_SIGSAFE_LOG_MAX_FDS];
   int n_fds;
 
   LOCK_LOGS();
   /* Reserve the first one for stderr. This is safe because when we daemonize,
-   * we dup2 /dev/null to stderr.
-   * For stderr, log_fds and err_fds are the same. */
-  log_fds[0] = err_fds[0] = STDERR_FILENO;
+   * we dup2 /dev/null to stderr. */
+  fds[0] = STDERR_FILENO;
   n_fds = 1;
 
   for (lf = logfiles; lf; lf = lf->next) {
@@ -697,21 +688,11 @@ tor_log_update_sigsafe_err_fds(void)
         (LD_BUG|LD_GENERAL)) {
       if (lf->fd == STDERR_FILENO)
         found_real_stderr = 1;
-      /* Avoid duplicates by checking the log module fd against log_fds */
-      if (int_array_contains(log_fds, n_fds, lf->fd))
+      /* Avoid duplicates by checking the log module fd against fds */
+      if (int_array_contains(fds, n_fds, lf->fd))
         continue;
-      /* Update log_fds using the log module's fd */
-      log_fds[n_fds] = lf->fd;
-      if (lf->needs_close) {
-        /* File log fds are duplicated, because close_log() closes the log
-         * module's fd, and tor_log_close_sigsafe_err_fds() closes the err
-         * module's fd. Both refer to the same file. */
-        err_fds[n_fds] = dup(lf->fd);
-      } else {
-        /* stdio log fds are not closed by the log module.
-         * tor_log_close_sigsafe_err_fds() closes stdio logs.  */
-        err_fds[n_fds] = lf->fd;
-      }
+      /* Update fds using the log module's fd */
+      fds[n_fds] = lf->fd;
       n_fds++;
       if (n_fds == TOR_SIGSAFE_LOG_MAX_FDS)
         break;
@@ -719,20 +700,19 @@ tor_log_update_sigsafe_err_fds(void)
   }
 
   if (!found_real_stderr &&
-      int_array_contains(log_fds, n_fds, STDOUT_FILENO)) {
+      int_array_contains(fds, n_fds, STDOUT_FILENO)) {
     /* Don't use a virtual stderr when we're also logging to stdout.
      * If we reached max_fds logs, we'll now have (max_fds - 1) logs.
      * That's ok, max_fds is large enough that most tor instances don't exceed
      * it. */
     raw_assert(n_fds >= 2); /* Don't tor_assert inside log fns */
     --n_fds;
-    log_fds[0] = log_fds[n_fds];
-    err_fds[0] = err_fds[n_fds];
+    fds[0] = fds[n_fds];
   }
 
   UNLOCK_LOGS();
 
-  tor_log_set_sigsafe_err_fds(err_fds, n_fds);
+  tor_log_set_sigsafe_err_fds(fds, n_fds);
 }
 
 /** Add to <b>out</b> a copy of every currently configured log file name. Used
@@ -841,16 +821,16 @@ logs_free_all(void)
    * log mutex. */
 }
 
-/** Close signal-safe log files.
- * Closing the log files makes the process and OS flush log buffers.
+/** Flush the signal-safe log files.
  *
- * This function is safe to call from a signal handler. It should only be
- * called when shutting down the log or err modules. It is currenly called
- * by the err module, when terminating the process on an abnormal condition.
+ * This function is safe to call from a signal handler. It is currenly called
+ * by the BUG() macros, when terminating the process on an abnormal condition.
  */
 void
-logs_close_sigsafe(void)
+logs_flush_sigsafe(void)
 {
+  /* If we don't have fsync() in unistd.h, we can't flush the logs. */
+#ifdef HAVE_FSYNC
   logfile_t *victim, *next;
   /* We can't LOCK_LOGS() in a signal handler, because it may call
    * signal-unsafe functions. And we can't deallocate memory, either. */
@@ -860,9 +840,11 @@ logs_close_sigsafe(void)
     victim = next;
     next = next->next;
     if (victim->needs_close) {
-      close_log_sigsafe(victim);
+      /* We can't do anything useful if the flush fails. */
+      (void)fsync(victim->fd);
     }
   }
+#endif /* defined(HAVE_FSYNC) */
 }
 
 /** Remove and free the log entry <b>victim</b> from the linked-list
diff --git a/src/lib/log/log.h b/src/lib/log/log.h
index cb588635d7..aafbf9be2f 100644
--- a/src/lib/log/log.h
+++ b/src/lib/log/log.h
@@ -186,7 +186,7 @@ void logs_set_domain_logging(int enabled);
 int get_min_log_level(void);
 void switch_logs_debug(void);
 void logs_free_all(void);
-void logs_close_sigsafe(void);
+void logs_flush_sigsafe(void);
 void add_default_log(int min_severity);
 void close_temp_logs(void);
 void rollback_log_changes(void);
diff --git a/src/lib/log/util_bug.c b/src/lib/log/util_bug.c
index de44d30e4e..581ae85f47 100644
--- a/src/lib/log/util_bug.c
+++ b/src/lib/log/util_bug.c
@@ -172,7 +172,7 @@ tor_bug_occurred_(const char *fname, unsigned int line,
 void
 tor_abort_(void)
 {
-  logs_close_sigsafe();
+  logs_flush_sigsafe();
   tor_raw_abort_();
 }