[tor-commits] [tor/release-0.3.2] ns: Call notify_networkstatus_changed() after the new consensus is set globally

[nickm at torproject.org]

commit 3a247ca92a06c864a2cb634fbe2bc23cf48fb977
Author: David Goulet <dgoulet at torproject.org>
Date:   Wed Jan 31 11:08:33 2018 -0500

    ns: Call notify_networkstatus_changed() after the new consensus is set globally
    
    In 0.3.2.1-alpha, we've added this function in order to have a way to notify
    other subsystems that the consensus just changed. The old consensus and the
    new one are passed to it.
    
    Before this patch, this was done _before_ the new consensus was set globally
    (thus NOT accessible by getting the latest consensus). The scheduler
    notification was assuming that it was set and select_scheduler() is looking at
    the latest consensus to get the parameters it might needs. This was very wrong
    because at that point it is still the old consensus set globally.
    
    With this commit, notify_networkstatus_changed() has been moved _after_ the
    new consensus is set globally. The main obvious reasons is to fix the bug
    described above and in #24975. The other reason is that this notify function
    doesn't return anything which could be allowing the possibility of refusing to
    set the new consensus on error. In other words, the new consensus is set right
    after the notification whatever happens.
    
    It does no harm or change in behavior to set the new consensus first and then
    notify the subsystems. The two functions currently used are for the control
    port using the old and new consensus and sending the diff. The second is the
    scheduler that needs the new consensus to be set globally before being called.
    
    Of course, the function has been documented accordinly to clearly state it is
    done _after_ the new consensus is set.
    
    Fixes #24975
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/bug24975       |  6 ++++++
 src/or/networkstatus.c | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/changes/bug24975 b/changes/bug24975
new file mode 100644
index 000000000..fe937d74d
--- /dev/null
+++ b/changes/bug24975
@@ -0,0 +1,6 @@
+  o Major bugfixes (scheduler, consensus):
+    - A logic in the code was preventing the scheduler subystem to properly
+      make a decision based on the latest consensus when it arrives. This lead
+      to the scheduler failing to notice any consensus parameters that might
+      change from one consensus to another. Fixes bug 24975; bugfix on
+      0.3.2.1-alpha.
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index e0a3e4cdc..4ae114f35 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1564,7 +1564,11 @@ notify_control_networkstatus_changed(const networkstatus_t *old_c,
   smartlist_free(changed);
 }
 
-/* Called when the consensus has changed from old_c to new_c. */
+/* Called when the consensus has changed from old_c to new_c.
+ *
+ * IMPORTANT: This is called _after_ the new consensus has been set in the
+ * global state so this is safe for anything getting the latest consensus from
+ * that state. */
 static void
 notify_networkstatus_changed(const networkstatus_t *old_c,
                              const networkstatus_t *new_c)
@@ -1897,9 +1901,6 @@ networkstatus_set_current_consensus(const char *consensus,
 
   const int is_usable_flavor = flav == usable_consensus_flavor();
 
-  if (is_usable_flavor) {
-    notify_networkstatus_changed(networkstatus_get_latest_consensus(), c);
-  }
   if (flav == FLAV_NS) {
     if (current_ns_consensus) {
       networkstatus_copy_old_consensus_info(c, current_ns_consensus);
@@ -1922,6 +1923,13 @@ networkstatus_set_current_consensus(const char *consensus,
     free_consensus = 0; /* avoid free */
   }
 
+  /* Called _after_ the consensus is set in its global variable so any
+   * functions called from this notification can safely get the latest
+   * consensus being the new one. */
+  if (is_usable_flavor) {
+    notify_networkstatus_changed(networkstatus_get_latest_consensus(), c);
+  }
+
   waiting = &consensus_waiting_for_certs[flav];
   if (waiting->consensus &&
       waiting->consensus->valid_after <= c->valid_after) {