[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor/master] Make sure primary-guards are up-to-date when we inspect them.



commit ac67819396ac9e96c3dd65a5b5b23715e11eeec5
Author: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
Date:   Wed Nov 23 10:04:23 2016 -0500

    Make sure primary-guards are up-to-date when we inspect them.
    
    (Plus some magic to prevent and detect recursive invocation of
    entry_guards_update_primary(), since that can cause some pretty
    tricky misbehavior.)
---
 src/or/entrynodes.c        | 58 +++++++++++++++++++++++++++++++++-------------
 src/or/entrynodes.h        |  8 +++++++
 src/test/test_entrynodes.c |  3 ++-
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index 9a753e6..bd30078 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -79,9 +79,6 @@
  **/
 /* DOCDOC -- expand this.
  *
- * XXXX prop271 -- make sure we check all of these properties everywhere we
- * should.
- *
  * Information invariants:
  *
  * [x] whenever a guard becomes unreachable, clear its usable_filtered flag.
@@ -100,11 +97,11 @@
  * [x] Whenever we remove a guard from the sample, remove it from the primary
  * and confirmed lists.
  *
- * [ ] When we make a guard confirmed, update the primary list.
+ * [x] When we make a guard confirmed, update the primary list.
  *
- * [ ] When we make a guard filtered or unfiltered, update the primary list.
+ * [x] When we make a guard filtered or unfiltered, update the primary list.
  *
- * [ ] When we are about to pick a guard, make sure that the primary list is
+ * [x] When we are about to pick a guard, make sure that the primary list is
  * full.
  *
  * [x] Before calling sample_reachable_filtered_entry_guards(), make sure
@@ -682,9 +679,12 @@ sampled_guards_update_from_consensus(guard_selection_t *gs)
   } SMARTLIST_FOREACH_END(guard);
 
   if (n_changes) {
-    /* Regnerate other things. XXXXXX prop271 */
-    // XXXX prop271 rebuild confirmed list.
+    gs->primary_guards_up_to_date = 0;
     entry_guards_update_filtered_sets(gs);
+    /* We don't need to rebuild the confirmed list right here -- we may have
+     * removed confirmed guards above, but we can't have added any new
+     * confirmed guards.
+     */
     entry_guards_changed_for_guard_selection(gs);
   }
 }
@@ -749,6 +749,7 @@ entry_guard_set_filtered_flags(const or_options_t *options,
                                guard_selection_t *gs,
                                entry_guard_t *guard)
 {
+  unsigned was_filtered = guard->is_filtered_guard;
   guard->is_filtered_guard = 0;
   guard->is_usable_filtered_guard = 0;
 
@@ -763,6 +764,11 @@ entry_guard_set_filtered_flags(const or_options_t *options,
   log_debug(LD_GUARD, "Updated sampled guard %s: filtered=%d; "
             "reachable_filtered=%d.", entry_guard_describe(guard),
             guard->is_filtered_guard, guard->is_usable_filtered_guard);
+
+  if (!bool_eq(was_filtered, guard->is_filtered_guard)) {
+    /* This guard might now be primary or nonprimary. */
+    gs->primary_guards_up_to_date = 0;
+  }
 }
 
 /**
@@ -795,6 +801,7 @@ sample_reachable_filtered_entry_guards(guard_selection_t *gs,
   const unsigned exclude_confirmed = flags & SAMPLE_EXCLUDE_CONFIRMED;
   const unsigned exclude_primary = flags & SAMPLE_EXCLUDE_PRIMARY;
   const unsigned exclude_pending = flags & SAMPLE_EXCLUDE_PENDING;
+  const unsigned no_update_primary = flags & SAMPLE_NO_UPDATE_PRIMARY;
 
   SMARTLIST_FOREACH_BEGIN(gs->sampled_entry_guards, entry_guard_t *, guard) {
     entry_guard_consider_retry(guard);
@@ -810,6 +817,9 @@ sample_reachable_filtered_entry_guards(guard_selection_t *gs,
     entry_guards_expand_sample(gs);
   }
 
+  if (exclude_primary && !gs->primary_guards_up_to_date && !no_update_primary)
+    entry_guards_update_primary(gs);
+
   /* Build the set of reachable filtered guards. */
   smartlist_t *reachable_filtered_sample = smartlist_new();
   SMARTLIST_FOREACH_BEGIN(gs->sampled_entry_guards, entry_guard_t *, guard) {
@@ -908,24 +918,34 @@ make_guard_confirmed(guard_selection_t *gs, entry_guard_t *guard)
   guard->confirmed_idx = gs->next_confirmed_idx++;
   smartlist_add(gs->confirmed_entry_guards, guard);
 
+  // This confirmed guard might kick something else out of the primary
+  // guards.
+  gs->primary_guards_up_to_date = 0;
+
   entry_guards_changed_for_guard_selection(gs);
 }
 
 /**
  * Recalculate the list of primary guards (the ones we'd prefer to use) from
  * the filtered sample and the confirmed list.
- *
- * XXXXX prop271 are calling this enough ???
  */
 STATIC void
 entry_guards_update_primary(guard_selection_t *gs)
 {
   tor_assert(gs);
 
+  // prevent recursion. Recursion is potentially very bad here.
+  static int running = 0;
+  tor_assert(!running);
+  running = 1;
+
   smartlist_t *new_primary_guards = smartlist_new();
   smartlist_t *old_primary_guards = smartlist_new();
   smartlist_add_all(old_primary_guards, gs->primary_entry_guards);
 
+  /* Set this flag now, to prevent the calls below from recursing. */
+  gs->primary_guards_up_to_date = 1;
+
   /* First, can we fill it up with confirmed guards? */
   SMARTLIST_FOREACH_BEGIN(gs->confirmed_entry_guards, entry_guard_t *, guard) {
     if (smartlist_len(new_primary_guards) >= N_PRIMARY_GUARDS)
@@ -964,7 +984,8 @@ entry_guards_update_primary(guard_selection_t *gs)
   while (smartlist_len(new_primary_guards) < N_PRIMARY_GUARDS) {
     entry_guard_t *guard = sample_reachable_filtered_entry_guards(gs,
                                             SAMPLE_EXCLUDE_CONFIRMED|
-                                            SAMPLE_EXCLUDE_PRIMARY);
+                                            SAMPLE_EXCLUDE_PRIMARY|
+                                            SAMPLE_NO_UPDATE_PRIMARY);
     if (!guard)
       break;
     guard->is_primary = 1;
@@ -1007,6 +1028,8 @@ entry_guards_update_primary(guard_selection_t *gs)
   smartlist_free(old_primary_guards);
   smartlist_free(gs->primary_entry_guards);
   gs->primary_entry_guards = new_primary_guards;
+  gs->primary_guards_up_to_date = 1;
+  running = 0;
 }
 
 /**
@@ -1104,6 +1127,9 @@ select_entry_guard_for_circuit(guard_selection_t *gs, unsigned *state_out)
   tor_assert(gs);
   tor_assert(state_out);
 
+  if (!gs->primary_guards_up_to_date)
+    entry_guards_update_primary(gs);
+
   /* "If any entry in PRIMARY_GUARDS has {is_reachable} status of
       <maybe> or <yes>, return the first such guard." */
   SMARTLIST_FOREACH_BEGIN(gs->primary_entry_guards, entry_guard_t *, guard) {
@@ -1191,6 +1217,9 @@ entry_guards_note_guard_failure(guard_selection_t *gs,
 STATIC void
 mark_primary_guards_maybe_reachable(guard_selection_t *gs)
 {
+  if (!gs->primary_guards_up_to_date)
+    entry_guards_update_primary(gs);
+
   SMARTLIST_FOREACH_BEGIN(gs->primary_entry_guards, entry_guard_t *, guard) {
     if (guard->is_reachable != GUARD_REACHABLE_NO)
       continue;
@@ -1230,7 +1259,6 @@ entry_guards_note_guard_success(guard_selection_t *gs,
     guard->is_usable_filtered_guard = 1;
 
   if (guard->confirmed_idx < 0) {
-    // XXXX prop271 XXXX update primary guards, since we confirmed something?
     make_guard_confirmed(gs, guard);
   }
 
@@ -1248,9 +1276,6 @@ entry_guards_note_guard_success(guard_selection_t *gs,
     }
   }
 
-  // XXXX prop271 XXXX update primary guards, since we confirmed something?
-  // XXXX prop261 XXXX if so, here or above?
-
   log_info(LD_GUARD, "Recorded success for %s%sguard %s",
            guard->is_primary?"primary ":"",
            guard->confirmed_idx>=0?"confirmed ":"",
@@ -1468,8 +1493,9 @@ entry_guard_chan_failed(guard_selection_t *gs,
 static int
 entry_guards_all_primary_guards_are_down(guard_selection_t *gs)
 {
-  /* XXXXX prop271 do we have to call entry_guards_update_primary() ?? */
   tor_assert(gs);
+  if (!gs->primary_guards_up_to_date)
+    entry_guards_update_primary(gs);
   SMARTLIST_FOREACH_BEGIN(gs->primary_entry_guards, entry_guard_t *, guard) {
     entry_guard_consider_retry(guard);
     if (guard->is_reachable != GUARD_REACHABLE_NO)
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h
index 4cbfbf5..a514c13 100644
--- a/src/or/entrynodes.h
+++ b/src/or/entrynodes.h
@@ -223,6 +223,13 @@ struct guard_selection_s {
   int dirty;
 
   /**
+   * A value of 1 means that primary_entry_guards is up-to-date; 0
+   * means we need to recalculate it before using primary_entry_guards
+   * or the is_primary flag on any guard.
+   */
+  int primary_guards_up_to_date;
+
+  /**
    * A list of the sampled entry guards, as entry_guard_t structures.
    * Not in any particular order.  When we 'sample' a guard, we are
    * noting it as a possible guard to pick in the future. The use of
@@ -428,6 +435,7 @@ STATIC void entry_guards_update_filtered_sets(guard_selection_t *gs);
 #define SAMPLE_EXCLUDE_CONFIRMED   (1u<<0)
 #define SAMPLE_EXCLUDE_PRIMARY     (1u<<1)
 #define SAMPLE_EXCLUDE_PENDING     (1u<<2)
+#define SAMPLE_NO_UPDATE_PRIMARY   (1u<<3)
 /**@}*/
 STATIC entry_guard_t *sample_reachable_filtered_entry_guards(
                                     guard_selection_t *gs,
diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c
index d8b9ccb..cdf8672 100644
--- a/src/test/test_entrynodes.c
+++ b/src/test/test_entrynodes.c
@@ -1839,6 +1839,7 @@ test_entry_guard_sample_reachable_filtered(void *arg)
   g->pb.path_bias_disabled = 1;
 
   entry_guards_update_filtered_sets(gs);
+  gs->primary_guards_up_to_date = 1;
   tt_int_op(num_reachable_filtered_guards(gs), OP_EQ, n_guards - 1);
   tt_int_op(smartlist_len(gs->sampled_entry_guards), OP_EQ, n_guards);
 
@@ -1851,7 +1852,7 @@ test_entry_guard_sample_reachable_filtered(void *arg)
   } tests[] = {
     { 0, -1 },
     { SAMPLE_EXCLUDE_CONFIRMED, 1 },
-    { SAMPLE_EXCLUDE_PRIMARY, 2 },
+    { SAMPLE_EXCLUDE_PRIMARY|SAMPLE_NO_UPDATE_PRIMARY, 2 },
     { SAMPLE_EXCLUDE_PENDING, 0 },
     { -1, -1},
   };



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits