Don't look at any routerstatus_t when the networkstatus is inconsistent

For a brief moment in networkstatus_set_current_consensus(), the old
consensus has been freed, but the node_t objects still have dead
pointers to the routerstatus_t objects within it.  During that
interval, we absolutely must not do anything that would cause Tor to
look at those dangling pointers.

Unfortunately, calling the (badly labeled!) current_consensus macro
or anything else that calls into we_use_microdescriptors_for_circuits(),
can make us look at the nodelist.

The fix is to make sure we identify the main consensus flavor
_outside_ the danger zone, and to make the danger zone much much
smaller.

Fixes bug 20103.  This bug has been implicitly present for AGES; we
just got lucky for a very long time.  It became a crash bug in
0.2.8.2-alpha when we merged 35bbf2e4a4e8ccb to make
find_dl_schedule start looking at the consensus, and 4460feaf2850ef0
which made node_get_all_orports less (accidentally) tolerant of
nodes with a valid ri pointer but dangling rs pointer.

diff --git a/changes/bug20103 b/changes/bug20103
new file mode 100644 (file)
index 0000000..c2b81d3
--- /dev/null
+++ b/changes/bug20103
@@ -0,0 +1,7 @@
+  o Major bug fixes (crash):
+
+    - Fix a complicated crash bug that could affect Tor clients
+      configured to use bridges when replacing a networkstatus consensus
+      in which one of their bridges was mentioned. OpenBSD users saw
+      more crashes here, but all platforms were potentially affected.
+      Fixes bug 20103; bugfix on 0.2.8.2-alpha.

diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 51fc01108f81311049eaad6a8aa89fae5c556c99..1cedfef9b77096f019dba25623c5ecccefc9e04e 100644 (file)
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1631,7 +1631,9 @@ networkstatus_set_current_consensus(const char *consensus,
   if (r != 1 && dl_certs)
     authority_certs_fetch_missing(c, now);
 
-  if (flav == usable_consensus_flavor()) {
+  const int is_usable_flavor = flav == usable_consensus_flavor();
+
+  if (is_usable_flavor) {
     notify_control_networkstatus_changed(current_consensus, c);
   }
   if (flav == FLAV_NS) {
@@ -1674,20 +1676,12 @@ networkstatus_set_current_consensus(const char *consensus,
     }
   }
 
-  /* Reset the failure count only if this consensus is actually valid. */
-  if (c->valid_after <= now && now <= c->valid_until) {
-    download_status_reset(&consensus_dl_status[flav]);
-  } else {
-    if (!from_cache)
-      download_status_failed(&consensus_dl_status[flav], 0);
-  }
+  if (is_usable_flavor) {
+    nodelist_set_consensus(c);
 
-  if (flav == usable_consensus_flavor()) {
     /* XXXXNM Microdescs: needs a non-ns variant. ???? NM*/
     update_consensus_networkstatus_fetch_time(now);
 
-    nodelist_set_consensus(current_consensus);
-
     dirvote_recalculate_timing(options, now);
     routerstatus_list_update_named_server_map();
 
@@ -1711,6 +1705,14 @@ networkstatus_set_current_consensus(const char *consensus,
         current_consensus);
   }
 
+  /* Reset the failure count only if this consensus is actually valid. */
+  if (c->valid_after <= now && now <= c->valid_until) {
+    download_status_reset(&consensus_dl_status[flav]);
+  } else {
+    if (!from_cache)
+      download_status_failed(&consensus_dl_status[flav], 0);
+  }
+
   if (directory_caches_dir_info(options)) {
     dirserv_set_cached_consensus_networkstatus(consensus,
                                                flavor,