commit 9b5a19c64b8195298753b3cd7059e8fcaaabe5bd Author: Nick Mathewson <nickm@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,