[tor-commits] [tor/master] Don't look at any routerstatus_t when the networkstatus is inconsistent

[nickm at torproject.org]

commit 9b5a19c64b8195298753b3cd7059e8fcaaabe5bd
Author: Nick Mathewson <nickm at torproject.org>
Date:   Mon Sep 19 12:03:58 2016 -0400

    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.
---
 changes/bug20103       |  7 +++++++
 src/or/networkstatus.c | 24 +++++++++++++-----------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/changes/bug20103 b/changes/bug20103
new file mode 100644
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 51fc011..1cedfef 100644
--- 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,