Merge branches 'bug24099_031' and 'bug24086_031' into maint-0.3.2

changes/bug24086

@@ -0,0 +1,7 @@
+  o Minor bugfixes (directory cache):
+    - When a consensus diff calculation is only partially successful, only
+      record the successful parts as having succeeded. Partial success
+      can happen if (for example) one compression method fails but
+      the others succeed. Previously we misrecorded all the calculations as
+      having succeeded, which would later cause a nonfatal assertion failure.
+      Fixes bug 24086; bugfix on 0.3.1.1-alpha.

changes/bug24099

@@ -0,0 +1,4 @@
+  o Minor bugfixes (directory cache):
+    - Recover better from empty or corrupt files in the consensus cache
+      directory. Fixes bug 24099; bugfix on 0.3.1.1-alpha.
+

src/common/storagedir.c

@@ -187,14 +187,19 @@ storage_dir_get_usage(storage_dir_t *d)
   return d->usage;
 }
 
-/** Mmap a specified file within <b>d</b>. */
+/** Mmap a specified file within <b>d</b>.
+ *
+ * On failure, return NULL and set errno as for tor_mmap_file(). */
 tor_mmap_t *
 storage_dir_map(storage_dir_t *d, const char *fname)
 {
   char *path = NULL;
   tor_asprintf(&path, "%s/%s", d->directory, fname);
   tor_mmap_t *result = tor_mmap_file(path);
+  int errval = errno;
   tor_free(path);
+  if (result == NULL)
+    errno = errval;
   return result;
 }
 
@@ -364,6 +369,10 @@ storage_dir_save_labeled_to_file(storage_dir_t *d,
  * the data's size into *<b>sz_out</b>. On success, also return a tor_mmap_t
  * object whose contents should not be used -- it needs to be kept around,
  * though, for as long as <b>data_out</b> is going to be valid.
+ *
+ * On failure, set errno as for tor_mmap_file() if the file was missing or
+ * empty, and set errno to EINVAL if the file was not in the labeled
+ * format expected.
  */
 tor_mmap_t *
 storage_dir_map_labeled(storage_dir_t *dir,
@@ -373,13 +382,20 @@ storage_dir_map_labeled(storage_dir_t *dir,
                          size_t *sz_out)
 {
   tor_mmap_t *m = storage_dir_map(dir, fname);
-  if (! m)
+  int errval;
+  if (! m) {
+    errval = errno;
     goto err;
+  }
   const char *nulp = memchr(m->data, '\0', m->size);
-  if (! nulp)
+  if (! nulp) {
+    errval = EINVAL;
     goto err;
-  if (labels_out && config_get_lines(m->data, labels_out, 0) < 0)
+  }
+  if (labels_out && config_get_lines(m->data, labels_out, 0) < 0) {
+    errval = EINVAL;
     goto err;
+  }
   size_t offset = nulp - m->data + 1;
   tor_assert(offset <= m->size);
   *data_out = (const uint8_t *)(m->data + offset);
@@ -388,6 +404,7 @@ storage_dir_map_labeled(storage_dir_t *dir,
   return m;
  err:
   tor_munmap_file(m);
+  errno = errval;
   return NULL;
 }
 

src/or/conscache.c

@@ -539,9 +539,20 @@ consensus_cache_rescan(consensus_cache_t *cache)
     map = storage_dir_map_labeled(cache->dir, fname,
                                   &labels, &body, &bodylen);
     if (! map) {
-      /* Can't load this; continue */
-      log_warn(LD_FS, "Unable to map file %s from consensus cache: %s",
-               escaped(fname), strerror(errno));
+      /* The ERANGE error might come from tor_mmap_file() -- it means the file
+       * was empty. EINVAL might come from ..map_labeled() -- it means the
+       * file was misformatted. In both cases, we should just delete it.
+       */
+      if (errno == ERANGE || errno == EINVAL) {
+        log_warn(LD_FS, "Found %s file %s in consensus cache; removing it.",
+                 errno == ERANGE ? "empty" : "misformatted",
+                 escaped(fname));
+        storage_dir_remove_file(cache->dir, fname);
+      } else {
+        /* Can't load this; continue */
+        log_warn(LD_FS, "Unable to map file %s from consensus cache: %s",
+                 escaped(fname), strerror(errno));
+      }
       continue;
     }
     consensus_cache_entry_t *ent =

src/or/consdiffmgr.c

@@ -283,6 +283,10 @@ cdm_diff_ht_set_status(consensus_flavor_t flav,
                        int status,
                        consensus_cache_entry_handle_t *handle)
 {
+  if (handle == NULL) {
+    tor_assert_nonfatal(status != CDM_DIFF_PRESENT);
+  }
+
   struct cdm_diff_t search, *ent;
   memset(&search, 0, sizeof(cdm_diff_t));
   search.flavor = flav;
@@ -1589,8 +1593,13 @@ consensus_diff_worker_replyfn(void *work_)
   for (u = 0; u < ARRAY_LENGTH(handles); ++u) {
     compress_method_t method = compress_diffs_with[u];
     if (cache) {
-      cdm_diff_ht_set_status(flav, from_sha3, to_sha3, method, status,
-                             handles[u]);
+      consensus_cache_entry_handle_t *h = handles[u];
+      int this_status = status;
+      if (h == NULL) {
+        this_status = CDM_DIFF_ERROR;
+      }
+      tor_assert_nonfatal(h != NULL || this_status == CDM_DIFF_ERROR);
+      cdm_diff_ht_set_status(flav, from_sha3, to_sha3, method, this_status, h);
     } else {
       consensus_cache_entry_handle_free(handles[u]);
     }