Merge branch 'bug8787_squashed'

changes/bug8787

@@ -0,0 +1,5 @@
+  o Minor features:
+    - Always check return values for unlink, munmap, UnmapViewOfFile;
+      check strftime return values more often. In some cases all we
+      can do is report a warning, but this may help prevent deeper
+      bugs from going unnoticed. Closes ticket 8787.

src/common/compat.c

@@ -35,6 +35,12 @@
 #ifdef HAVE_UNAME
 #include <sys/utsname.h>
 #endif
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#ifdef HAVE_SYS_STAT_H
+#include <sys/stat.h>
+#endif
 #ifdef HAVE_UNISTD_H
 #include <unistd.h>
 #endif
@@ -178,9 +184,10 @@ tor_mmap_file(const char *filename)
 {
   int fd; /* router file */
   char *string;
-  int page_size;
+  int page_size, result;
   tor_mmap_t *res;
   size_t size, filesize;
+  struct stat st;
 
   tor_assert(filename);
 
@@ -194,9 +201,22 @@ tor_mmap_file(const char *filename)
     return NULL;
   }
 
-  /* XXXX why not just do fstat here? */
+  /* Get the size of the file */
-  size = filesize = (size_t) lseek(fd, 0, SEEK_END);
+  result = fstat(fd, &st);
-  lseek(fd, 0, SEEK_SET);
+  if (result != 0) {
+    int save_errno = errno;
+    log_warn(LD_FS,
+             "Couldn't fstat opened descriptor for \"%s\" during mmap: %s",
+             filename, strerror(errno));
+    close(fd);
+    errno = save_errno;
+    return NULL;
+  }
+  size = filesize = (size_t)(st.st_size);
+  /*
+   * Should we check for weird crap like mmapping a named pipe here,
+   * or just wait for if (!size) below to fail?
+   */
   /* ensure page alignment */
   page_size = getpagesize();
   size += (size%page_size) ? page_size-(size%page_size) : 0;
@@ -227,12 +247,27 @@ tor_mmap_file(const char *filename)
 
   return res;
 }
-/** Release storage held for a memory mapping. */
+/** Release storage held for a memory mapping; returns 0 on success,
-void
+ * or -1 on failure (and logs a warning). */
+int
 tor_munmap_file(tor_mmap_t *handle)
 {
-  munmap((char*)handle->data, handle->mapping_size);
+  int res;
-  tor_free(handle);
+
+  if (handle == NULL)
+    return 0;
+
+  res = munmap((char*)handle->data, handle->mapping_size);
+  if (res == 0) {
+    /* munmap() succeeded */
+    tor_free(handle);
+  } else {
+    log_warn(LD_FS, "Failed to munmap() in tor_munmap_file(): %s",
+             strerror(errno));
+    res = -1;
+  }
+
+  return res;
 }
 #elif defined(_WIN32)
 tor_mmap_t *
@@ -314,17 +349,29 @@ tor_mmap_file(const char *filename)
   tor_munmap_file(res);
   return NULL;
 }
-void
+
+/* Unmap the file, and return 0 for success or -1 for failure */
+int
 tor_munmap_file(tor_mmap_t *handle)
 {
-  if (handle->data)
+  if (handle == NULL)
+    return 0;
+
+  if (handle->data) {
     /* This is an ugly cast, but without it, "data" in struct tor_mmap_t would
        have to be redefined as non-const. */
-    UnmapViewOfFile( (LPVOID) handle->data);
+    BOOL ok = UnmapViewOfFile( (LPVOID) handle->data);
+    if (!ok) {
+      log_warn(LD_FS, "Failed to UnmapViewOfFile() in tor_munmap_file(): %d",
+               (int)GetLastError());
+    }
+  }
 
   if (handle->mmap_handle != NULL)
     CloseHandle(handle->mmap_handle);
   tor_free(handle);
+
+  return 0;
 }
 #else
 tor_mmap_t *
@@ -340,13 +387,25 @@ tor_mmap_file(const char *filename)
   handle->size = st.st_size;
   return handle;
 }
-void
+
+/** Unmap the file mapped with tor_mmap_file(), and return 0 for success
+ * or -1 for failure.
+ */
+
+int
 tor_munmap_file(tor_mmap_t *handle)
 {
-  char *d = (char*)handle->data;
+  char *d = NULL;
+  if (handle == NULL)
+    return 0;
+
+  d = (char*)handle->data;
   tor_free(d);
   memwipe(handle, 0, sizeof(tor_mmap_t));
   tor_free(handle);
+
+  /* Can't fail in this mmap()/munmap()-free case */
+  return 0;
 }
 #endif
 

src/common/compat.h

@@ -292,7 +292,7 @@ typedef struct tor_mmap_t {
 } tor_mmap_t;
 
 tor_mmap_t *tor_mmap_file(const char *filename) ATTR_NONNULL((1));
-void tor_munmap_file(tor_mmap_t *handle) ATTR_NONNULL((1));
+int tor_munmap_file(tor_mmap_t *handle) ATTR_NONNULL((1));
 
 int tor_snprintf(char *str, size_t size, const char *format, ...)
   CHECK_PRINTF(3,4) ATTR_NONNULL((1,3));

src/common/tortls.c

@@ -2312,6 +2312,7 @@ log_cert_lifetime(int severity, const X509 *cert, const char *problem)
   char mytime[33];
   time_t now = time(NULL);
   struct tm tm;
+  size_t n;
 
   if (problem)
     tor_log(severity, LD_GENERAL,
@@ -2337,11 +2338,17 @@ log_cert_lifetime(int severity, const X509 *cert, const char *problem)
   BIO_get_mem_ptr(bio, &buf);
   s2 = tor_strndup(buf->data, buf->length);
 
-  strftime(mytime, 32, "%b %d %H:%M:%S %Y UTC", tor_gmtime_r(&now, &tm));
+  n = strftime(mytime, 32, "%b %d %H:%M:%S %Y UTC", tor_gmtime_r(&now, &tm));
-
+  if (n > 0) {
-  tor_log(severity, LD_GENERAL,
+    tor_log(severity, LD_GENERAL,
-      "(certificate lifetime runs from %s through %s. Your time is %s.)",
+        "(certificate lifetime runs from %s through %s. Your time is %s.)",
-      s1,s2,mytime);
+        s1,s2,mytime);
+  } else {
+    tor_log(severity, LD_GENERAL,
+        "(certificate lifetime runs from %s through %s. "
+        "Couldn't get your time.)",
+        s1, s2);
+  }
 
  end:
   /* Not expected to get invoked */

src/common/util.c

@@ -2141,6 +2141,7 @@ static int
 finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
 {
   int r = 0;
+
   tor_assert(file_data && file_data->filename);
   if (file_data->stdio_file) {
     if (fclose(file_data->stdio_file)) {
@@ -2157,7 +2158,13 @@ finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
   if (file_data->rename_on_close) {
     tor_assert(file_data->tempname && file_data->filename);
     if (abort_write) {
-      unlink(file_data->tempname);
+      int res = unlink(file_data->tempname);
+      if (res != 0) {
+        /* We couldn't unlink and we'll leave a mess behind */
+        log_warn(LD_FS, "Failed to unlink %s: %s",
+                 file_data->tempname, strerror(errno));
+        r = -1;
+      }
     } else {
       tor_assert(strcmp(file_data->filename, file_data->tempname));
       if (replace_file(file_data->tempname, file_data->filename)) {

src/or/config.c

@@ -6458,7 +6458,10 @@ remove_file_if_very_old(const char *fname, time_t now)
     format_local_iso_time(buf, st.st_mtime);
     log_notice(LD_GENERAL, "Obsolete file %s hasn't been modified since %s. "
                "Removing it.", fname, buf);
-    unlink(fname);
+    if (unlink(fname) != 0) {
+      log_warn(LD_FS, "Failed to unlink %s: %s",
+               fname, strerror(errno));
+    }
   }
 }
 

src/or/hibernate.c

@@ -648,7 +648,15 @@ read_bandwidth_usage(void)
 
   {
     char *fname = get_datadir_fname("bw_accounting");
-    unlink(fname);
+    int res;
+
+    res = unlink(fname);
+    if (res != 0) {
+      log_warn(LD_FS,
+               "Failed to unlink %s: %s",
+               fname, strerror(errno));
+    }
+
     tor_free(fname);
   }
 

src/or/main.c

@@ -2574,10 +2574,19 @@ tor_cleanup(void)
     time_t now = time(NULL);
     /* Remove our pid file. We don't care if there was an error when we
      * unlink, nothing we could do about it anyways. */
-    if (options->PidFile)
+    if (options->PidFile) {
-      unlink(options->PidFile);
+      if (unlink(options->PidFile) != 0) {
-    if (options->ControlPortWriteToFile)
+        log_warn(LD_FS, "Couldn't unlink pid file %s: %s",
-      unlink(options->ControlPortWriteToFile);
+                 options->PidFile, strerror(errno));
+      }
+    }
+    if (options->ControlPortWriteToFile) {
+      if (unlink(options->ControlPortWriteToFile) != 0) {
+        log_warn(LD_FS, "Couldn't unlink control port file %s: %s",
+                 options->ControlPortWriteToFile,
+                 strerror(errno));
+      }
+    }
     if (accounting_is_enabled(options))
       accounting_record_bandwidth_usage(now, get_or_state());
     or_state_mark_dirty(get_or_state(), 0); /* force an immediate save. */

src/or/microdesc.c

@@ -275,6 +275,7 @@ void
 microdesc_cache_clear(microdesc_cache_t *cache)
 {
   microdesc_t **entry, **next;
+
   for (entry = HT_START(microdesc_map, &cache->map); entry; entry = next) {
     microdesc_t *md = *entry;
     next = HT_NEXT_RMV(microdesc_map, &cache->map, entry);
@@ -283,7 +284,13 @@ microdesc_cache_clear(microdesc_cache_t *cache)
   }
   HT_CLEAR(microdesc_map, &cache->map);
   if (cache->cache_content) {
-    tor_munmap_file(cache->cache_content);
+    int res = tor_munmap_file(cache->cache_content);
+    if (res != 0) {
+      log_warn(LD_FS,
+               "tor_munmap_file() failed clearing microdesc cache; "
+               "we are probably about to leak memory.");
+      /* TODO something smarter? */
+    }
     cache->cache_content = NULL;
   }
   cache->total_len_seen = 0;
@@ -479,7 +486,7 @@ int
 microdesc_cache_rebuild(microdesc_cache_t *cache, int force)
 {
   open_file_t *open_file;
-  int fd = -1;
+  int fd = -1, res;
   microdesc_t **mdp;
   smartlist_t *wrote;
   ssize_t size;
@@ -546,8 +553,14 @@ microdesc_cache_rebuild(microdesc_cache_t *cache, int force)
 
   /* We must do this unmap _before_ we call finish_writing_to_file(), or
    * windows will not actually replace the file. */
-  if (cache->cache_content)
+  if (cache->cache_content) {
-    tor_munmap_file(cache->cache_content);
+    res = tor_munmap_file(cache->cache_content);
+    if (res != 0) {
+      log_warn(LD_FS,
+               "Failed to unmap old microdescriptor cache while rebuilding");
+    }
+    cache->cache_content = NULL;
+  }
 
   if (finish_writing_to_file(open_file) < 0) {
     log_warn(LD_DIR, "Error rebuilding microdescriptor cache: %s",

src/or/networkstatus.c

@@ -1254,7 +1254,11 @@ networkstatus_set_current_consensus(const char *consensus,
         /* Even if we had enough signatures, we'd never use this as the
          * latest consensus. */
         if (was_waiting_for_certs && from_cache)
-          unlink(unverified_fname);
+          if (unlink(unverified_fname) != 0) {
+            log_warn(LD_FS,
+                     "Failed to unlink %s: %s",
+                     unverified_fname, strerror(errno));
+          }
       }
       goto done;
     } else {
@@ -1264,8 +1268,13 @@ networkstatus_set_current_consensus(const char *consensus,
                  "consensus");
         result = -2;
       }
-      if (was_waiting_for_certs && (r < -1) && from_cache)
+      if (was_waiting_for_certs && (r < -1) && from_cache) {
-        unlink(unverified_fname);
+        if (unlink(unverified_fname) != 0) {
+            log_warn(LD_FS,
+                     "Failed to unlink %s: %s",
+                     unverified_fname, strerror(errno));
+        }
+      }
       goto done;
     }
   }
@@ -1313,7 +1322,11 @@ networkstatus_set_current_consensus(const char *consensus,
       waiting->body = NULL;
     waiting->set_at = 0;
     waiting->dl_failed = 0;
-    unlink(unverified_fname);
+    if (unlink(unverified_fname) != 0) {
+      log_warn(LD_FS,
+               "Failed to unlink %s: %s",
+               unverified_fname, strerror(errno));
+    }
   }
 
   /* Reset the failure count only if this consensus is actually valid. */

src/or/routerlist.c

@@ -1064,8 +1064,11 @@ router_rebuild_store(int flags, desc_store_t *store)
 
   /* Our mmap is now invalid. */
   if (store->mmap) {
-    tor_munmap_file(store->mmap);
+    int res = tor_munmap_file(store->mmap);
     store->mmap = NULL;
+    if (res != 0) {
+      log_warn(LD_FS, "Unable to munmap route store in %s", fname);
+    }
   }
 
   if (replace_file(fname_tmp, fname)<0) {
@@ -1139,9 +1142,16 @@ router_reload_router_list_impl(desc_store_t *store)
 
   fname = get_datadir_fname(store->fname_base);
 
-  if (store->mmap) /* get rid of it first */
+  if (store->mmap) {
-    tor_munmap_file(store->mmap);
+    /* get rid of it first */
-  store->mmap = NULL;
+    int res = tor_munmap_file(store->mmap);
+    store->mmap = NULL;
+    if (res != 0) {
+      log_warn(LD_FS, "Failed to munmap %s", fname);
+      tor_free(fname);
+      return -1;
+    }
+  }
 
   store->mmap = tor_mmap_file(fname);
   if (store->mmap) {
@@ -2794,10 +2804,18 @@ routerlist_free(routerlist_t *rl)
                     signed_descriptor_free(sd));
   smartlist_free(rl->routers);
   smartlist_free(rl->old_routers);
-  if (routerlist->desc_store.mmap)
+  if (rl->desc_store.mmap) {
-    tor_munmap_file(routerlist->desc_store.mmap);
+    int res = tor_munmap_file(routerlist->desc_store.mmap);
-  if (routerlist->extrainfo_store.mmap)
+    if (res != 0) {
-    tor_munmap_file(routerlist->extrainfo_store.mmap);
+      log_warn(LD_FS, "Failed to munmap routerlist->desc_store.mmap");
+    }
+  }
+  if (rl->extrainfo_store.mmap) {
+    int res = tor_munmap_file(routerlist->extrainfo_store.mmap);
+    if (res != 0) {
+      log_warn(LD_FS, "Failed to munmap routerlist->extrainfo_store.mmap");
+    }
+  }
   tor_free(rl);
 
   router_dir_info_changed();

src/or/statefile.c

@@ -260,7 +260,7 @@ or_state_set(or_state_t *new_state)
 static void
 or_state_save_broken(char *fname)
 {
-  int i;
+  int i, res;
   file_status_t status;
   char *fname2 = NULL;
   for (i = 0; i < 100; ++i) {
@@ -274,7 +274,13 @@ or_state_save_broken(char *fname)
     log_warn(LD_BUG, "Unable to parse state in \"%s\"; too many saved bad "
              "state files to move aside. Discarding the old state file.",
              fname);
-    unlink(fname);
+    res = unlink(fname);
+    if (res != 0) {
+      log_warn(LD_FS,
+               "Also couldn't discard old state file \"%s\" because "
+               "unlink() failed: %s",
+               fname, strerror(errno));
+    }
   } else {
     log_warn(LD_BUG, "Unable to parse state in \"%s\". Moving it aside "
              "to \"%s\".  This could be a bug in Tor; please tell "

src/test/test_util.c

@@ -1577,14 +1577,14 @@ test_util_mmap(void)
   test_eq(mapping->size, strlen("Short file."));
   test_streq(mapping->data, "Short file.");
 #ifdef _WIN32
-  tor_munmap_file(mapping);
+  tt_int_op(0, ==, tor_munmap_file(mapping));
   mapping = NULL;
   test_assert(unlink(fname1) == 0);
 #else
   /* make sure we can unlink. */
   test_assert(unlink(fname1) == 0);
   test_streq(mapping->data, "Short file.");
-  tor_munmap_file(mapping);
+  tt_int_op(0, ==, tor_munmap_file(mapping));
   mapping = NULL;
 #endif
 
@@ -1605,7 +1605,7 @@ test_util_mmap(void)
   test_assert(mapping);
   test_eq(mapping->size, buflen);
   test_memeq(mapping->data, buf, buflen);
-  tor_munmap_file(mapping);
+  tt_int_op(0, ==, tor_munmap_file(mapping));
   mapping = NULL;
 
   /* Now try a big aligned file. */
@@ -1614,7 +1614,7 @@ test_util_mmap(void)
   test_assert(mapping);
   test_eq(mapping->size, 16384);
   test_memeq(mapping->data, buf, 16384);
-  tor_munmap_file(mapping);
+  tt_int_op(0, ==, tor_munmap_file(mapping));
   mapping = NULL;
 
  done:
@@ -1627,8 +1627,7 @@ test_util_mmap(void)
   tor_free(fname3);
   tor_free(buf);
 
-  if (mapping)
+  tor_munmap_file(mapping);
-    tor_munmap_file(mapping);
 }
 
 /** Run unit tests for escaping/unescaping data for use by controllers. */