1 files changed, 89 insertions, 17 deletions

diff --git a/src/lib/log/log.c b/src/lib/log/log.c
index d21d8d1d41..ec7c2fa24e 100644
--- a/src/lib/log/log.c
+++ b/src/lib/log/log.c
@@ -49,6 +49,7 @@
 #include "lib/wallclock/approx_time.h"
 #include "lib/wallclock/time_to_tm.h"
 #include "lib/fdio/fdio.h"
+#include "lib/cc/ctassert.h"
 
 #ifdef HAVE_ANDROID_LOG_H
 #include <android/log.h>
@@ -154,7 +155,7 @@ severity_to_android_log_priority(int severity)
       // LCOV_EXCL_STOP
   }
 }
-#endif // HAVE_ANDROID_LOG_H.
+#endif /* defined(HAVE_ANDROID_LOG_H) */
 
 /** A mutex to guard changes to logfiles and logging. */
 static tor_mutex_t log_mutex;
@@ -224,6 +225,7 @@ int log_global_min_severity_ = LOG_NOTICE;
 
 static void delete_log(logfile_t *victim);
 static void close_log(logfile_t *victim);
+static void close_log_sigsafe(logfile_t *victim);
 
 static char *domain_to_string(log_domain_mask_t domain,
                              char *buf, size_t buflen);
@@ -664,13 +666,24 @@ tor_log_update_sigsafe_err_fds(void)
   const logfile_t *lf;
   int found_real_stderr = 0;
 
-  int fds[TOR_SIGSAFE_LOG_MAX_FDS];
+  /* 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];
   int n_fds;
 
   LOCK_LOGS();
   /* Reserve the first one for stderr. This is safe because when we daemonize,
-   * we dup2 /dev/null to stderr, */
-  fds[0] = STDERR_FILENO;
+   * we dup2 /dev/null to stderr.
+   * For stderr, log_fds and err_fds are the same. */
+  log_fds[0] = err_fds[0] = STDERR_FILENO;
   n_fds = 1;
 
   for (lf = logfiles; lf; lf = lf->next) {
@@ -684,25 +697,39 @@ tor_log_update_sigsafe_err_fds(void)
         (LD_BUG|LD_GENERAL)) {
       if (lf->fd == STDERR_FILENO)
         found_real_stderr = 1;
-      /* Avoid duplicates */
-      if (int_array_contains(fds, n_fds, lf->fd))
+      /* Avoid duplicates by checking the log module fd against log_fds */
+      if (int_array_contains(log_fds, n_fds, lf->fd))
         continue;
-      fds[n_fds++] = lf->fd;
+      /* 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;
+      }
+      n_fds++;
       if (n_fds == TOR_SIGSAFE_LOG_MAX_FDS)
         break;
     }
   }
 
   if (!found_real_stderr &&
-      int_array_contains(fds, n_fds, STDOUT_FILENO)) {
+      int_array_contains(log_fds, n_fds, STDOUT_FILENO)) {
     /* Don't use a virtual stderr when we're also logging to stdout. */
     raw_assert(n_fds >= 2); /* Don't tor_assert inside log fns */
-    fds[0] = fds[--n_fds];
+    --n_fds;
+    log_fds[0] = log_fds[n_fds];
+    err_fds[0] = err_fds[n_fds];
   }
 
   UNLOCK_LOGS();
 
-  tor_log_set_sigsafe_err_fds(fds, n_fds);
+  tor_log_set_sigsafe_err_fds(err_fds, n_fds);
 }
 
 /** Add to <b>out</b> a copy of every currently configured log file name. Used
@@ -805,7 +832,34 @@ logs_free_all(void)
   }
 
   /* We _could_ destroy the log mutex here, but that would screw up any logs
-   * that happened between here and the end of execution. */
+   * that happened between here and the end of execution.
+   * If tor is re-initialized, log_mutex_initialized will still be 1. So we
+   * won't trigger any undefined behaviour by trying to re-initialize the
+   * log mutex. */
+}
+
+/** Close signal-safe log files.
+ * Closing the log files makes the process and OS flush log buffers.
+ *
+ * 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.
+ */
+void
+logs_close_sigsafe(void)
+{
+  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. */
+  next = logfiles;
+  logfiles = NULL;
+  while (next) {
+    victim = next;
+    next = next->next;
+    if (victim->needs_close) {
+      close_log_sigsafe(victim);
+    }
+  }
 }
 
 /** Remove and free the log entry <b>victim</b> from the linked-list
@@ -834,13 +888,26 @@ delete_log(logfile_t *victim)
 }
 
 /** Helper: release system resources (but not memory) held by a single
- * logfile_t. */
+ * signal-safe logfile_t. If the log's resources can not be released in
+ * a signal handler, does nothing. */
 static void
-close_log(logfile_t *victim)
+close_log_sigsafe(logfile_t *victim)
 {
   if (victim->needs_close && victim->fd >= 0) {
+    /* We can't do anything useful here if close() fails: we're shutting
+     * down logging, and the err module only does fatal errors. */
     close(victim->fd);
     victim->fd = -1;
+  }
+}
+
+/** Helper: release system resources (but not memory) held by a single
+ * logfile_t. */
+static void
+close_log(logfile_t *victim)
+{
+  if (victim->needs_close) {
+    close_log_sigsafe(victim);
   } else if (victim->is_syslog) {
 #ifdef HAVE_SYSLOG_H
     if (--syslog_count == 0) {
@@ -1021,7 +1088,7 @@ flush_pending_log_callbacks(void)
   do {
     SMARTLIST_FOREACH_BEGIN(messages, pending_log_message_t *, msg) {
       const int severity = msg->severity;
-      const int domain = msg->domain;
+      const log_domain_mask_t domain = msg->domain;
       for (lf = logfiles; lf; lf = lf->next) {
         if (! lf->callback || lf->seems_dead ||
             ! (lf->severities->masks[SEVERITY_MASK_IDX(severity)] & domain)) {
@@ -1232,7 +1299,7 @@ add_android_log(const log_severity_list_t *severity,
   UNLOCK_LOGS();
   return 0;
 }
-#endif // HAVE_ANDROID_LOG_H.
+#endif /* defined(HAVE_ANDROID_LOG_H) */
 
 /** If <b>level</b> is a valid log severity, return the corresponding
  * numeric value.  Otherwise, return -1. */
@@ -1268,9 +1335,14 @@ static const char *domain_list[] = {
   "GENERAL", "CRYPTO", "NET", "CONFIG", "FS", "PROTOCOL", "MM",
   "HTTP", "APP", "CONTROL", "CIRC", "REND", "BUG", "DIR", "DIRSERV",
   "OR", "EDGE", "ACCT", "HIST", "HANDSHAKE", "HEARTBEAT", "CHANNEL",
-  "SCHED", "GUARD", "CONSDIFF", "DOS", "PROCESS", "PT", "BTRACK", NULL
+  "SCHED", "GUARD", "CONSDIFF", "DOS", "PROCESS", "PT", "BTRACK", "MESG",
+  NULL
 };
 
+CTASSERT(ARRAY_LENGTH(domain_list) == N_LOGGING_DOMAINS + 1);
+
+CTASSERT((UINT64_C(1)<<(N_LOGGING_DOMAINS-1)) < LOWEST_RESERVED_LD_FLAG_);
+
 /** Return a bitmask for the log domain for which <b>domain</b> is the name,
  * or 0 if there is no such name. */
 static log_domain_mask_t
@@ -1371,7 +1443,7 @@ parse_log_severity_config(const char **cfg_ptr,
             if (!strcmp(domain, "*")) {
               domains = ~0u;
             } else {
-              int d;
+              log_domain_mask_t d;
               int negate=0;
               if (*domain == '~') {
                 negate = 1;