[tor-commits] [tor/main] Prefer mmap()ed consensus files over cached_dir_t entries.

[dgoulet at torproject.org]

commit d12b16614d09b303f9ef9624107b589264537341
Author: Nick Mathewson <nickm at torproject.org>
Date:   Wed May 26 13:02:56 2021 -0400

    Prefer mmap()ed consensus files over cached_dir_t entries.
    
    Cached_dir_t is a somewhat "legacy" kind of storage when used for
    consensus documents, and it appears that there are cases when
    changing our settings causes us to stop updating those entries.
    
    This can cause trouble, as @arma found out in #40375, where he
    changed his settings around, and consensus diff application got
    messed up: consensus diffs were being _requested_ based on the
    latest consensus, but were being (incorrectly) applied to a
    consensus that was no longer the latest one.
    
    This patch is a minimal fix for backporting purposes: it has Tor do
    the same search when applying consensus diffs as we use to request
    them.  This should be sufficient for correct behavior.
    
    There's a similar case in GETINFO handling; I've fixed that too.
    
    Fixes #40375; bugfix on 0.3.1.1-alpha.
---
 changes/bug40375                      |  5 +++++
 src/feature/control/control_getinfo.c | 32 +++++++++++++++-----------------
 src/feature/dirclient/dirclient.c     | 21 +++++++++++++--------
 3 files changed, 33 insertions(+), 25 deletions(-)

diff --git a/changes/bug40375 b/changes/bug40375
new file mode 100644
index 0000000000..7ac32bc628
--- /dev/null
+++ b/changes/bug40375
@@ -0,0 +1,5 @@
+  o Minor bugfixes (consensus handling):
+    - Avoid a set of bugs that could be caused by inconsistently preferring
+      an out-of-date consensus stored in a stale directory cache over
+      a more recent one stored on disk as the latest consensus.
+      Fixes bug 40375; bugfix on 0.3.1.1-alpha.
diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c
index 5feadd23d1..899f188546 100644
--- a/src/feature/control/control_getinfo.c
+++ b/src/feature/control/control_getinfo.c
@@ -353,26 +353,24 @@ getinfo_helper_current_consensus(consensus_flavor_t flavor,
     *errmsg = "Internal error: unrecognized flavor name.";
     return -1;
   }
-  if (we_want_to_fetch_flavor(get_options(), flavor)) {
-    /** Check from the cache */
-    const cached_dir_t *consensus = dirserv_get_consensus(flavor_name);
-    if (consensus) {
-      *answer = tor_strdup(consensus->dir);
-    }
+  tor_mmap_t *mapped = networkstatus_map_cached_consensus(flavor_name);
+  if (mapped) {
+    *answer = tor_memdup_nulterm(mapped->data, mapped->size);
+    tor_munmap_file(mapped);
   }
-  if (!*answer) { /* try loading it from disk */
-
-    tor_mmap_t *mapped = networkstatus_map_cached_consensus(flavor_name);
-    if (mapped) {
-      *answer = tor_memdup_nulterm(mapped->data, mapped->size);
-      tor_munmap_file(mapped);
-    }
-    if (!*answer) { /* generate an error */
-      *errmsg = "Could not open cached consensus. "
-        "Make sure FetchUselessDescriptors is set to 1.";
-      return -1;
+  if (!*answer) { /* Maybe it's in the cache? */
+    if (we_want_to_fetch_flavor(get_options(), flavor)) {
+      const cached_dir_t *consensus = dirserv_get_consensus(flavor_name);
+      if (consensus) {
+        *answer = tor_strdup(consensus->dir);
+      }
     }
   }
+  if (!*answer) { /* generate an error */
+    *errmsg = "Could not open cached consensus. "
+      "Make sure FetchUselessDescriptors is set to 1.";
+    return -1;
+  }
   return 0;
 }
 
diff --git a/src/feature/dirclient/dirclient.c b/src/feature/dirclient/dirclient.c
index a5dd856729..b3159966d2 100644
--- a/src/feature/dirclient/dirclient.c
+++ b/src/feature/dirclient/dirclient.c
@@ -2263,18 +2263,23 @@ handle_response_fetch_consensus(dir_connection_t *conn,
 
   if (looks_like_a_consensus_diff(body, body_len)) {
     /* First find our previous consensus. Maybe it's in ram, maybe not. */
-    cached_dir_t *cd = dirserv_get_consensus(flavname);
+    cached_dir_t *cd = NULL;
     const char *consensus_body = NULL;
     size_t consensus_body_len;
     tor_mmap_t *mapped_consensus = NULL;
-    if (cd) {
-      consensus_body = cd->dir;
-      consensus_body_len = cd->dir_len;
+
+    /* We prefer the mmap'd version over the cached_dir_t version,
+     * since that matches the logic we used when we picked a consensus
+     * back in dir_consensus_request_set_additional_headers. */
+    mapped_consensus = networkstatus_map_cached_consensus(flavname);
+    if (mapped_consensus) {
+      consensus_body = mapped_consensus->data;
+      consensus_body_len = mapped_consensus->size;
     } else {
-      mapped_consensus = networkstatus_map_cached_consensus(flavname);
-      if (mapped_consensus) {
-        consensus_body = mapped_consensus->data;
-        consensus_body_len = mapped_consensus->size;
+      cd = dirserv_get_consensus(flavname);
+      if (cd) {
+        consensus_body = cd->dir;
+        consensus_body_len = cd->dir_len;
       }
     }
     if (!consensus_body) {