Merge branch 'bug24367_032_squashed' into maint-0.3.2

9 files changed, 73 insertions, 58 deletions

diff --git a/changes/bug24367 b/changes/bug24367
new file mode 100644
index 0000000000..09ef3bb877
--- /dev/null
+++ b/changes/bug24367
@@ -0,0 +1,13 @@
+  o Minor bugfixes (bridge clients, bootstrap):
+    - Retry directory downloads when we get our first bridge descriptor
+      during bootstrap or while reconnecting to the network. Keep retrying
+      every time we get a bridge descriptor, until we have a reachable bridge.
+      Fixes bug 24367; bugfix on 0.2.0.3-alpha.
+    - Stop delaying bridge descriptor fetches when we have cached bridge
+      descriptors. Instead, only delay bridge descriptor fetches when we
+      have at least one reachable bridge.
+      Fixes bug 24367; bugfix on 0.2.0.3-alpha.
+    - Stop delaying directory fetches when we have cached bridge descriptors.
+      Instead, only delay bridge descriptor fetches when all our bridges are
+      definitely unreachable.
+      Fixes bug 24367; bugfix on 0.2.0.3-alpha.
diff --git a/src/or/bridges.c b/src/or/bridges.c
index 320f5ee632..0b1bbbd158 100644
--- a/src/or/bridges.c
+++ b/src/or/bridges.c
@@ -796,7 +796,11 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
   tor_assert(ri);
   tor_assert(ri->purpose == ROUTER_PURPOSE_BRIDGE);
   if (get_options()->UseBridges) {
-    int first = num_bridges_usable() <= 1;
+    /* Retry directory downloads whenever we get a bridge descriptor:
+     * - when bootstrapping, and
+     * - when we aren't sure if any of our bridges are reachable.
+     * Keep on retrying until we have at least one reachable bridge. */
+    int first = num_bridges_usable(0) < 1;
     bridge_info_t *bridge = get_configured_bridge_by_routerinfo(ri);
     time_t now = time(NULL);
     router_set_status(ri->cache_info.identity_digest, 1);
@@ -826,8 +830,8 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
 
       log_notice(LD_DIR, "new bridge descriptor '%s' (%s): %s", ri->nickname,
                  from_cache ? "cached" : "fresh", router_describe(ri));
-      /* set entry->made_contact so if it goes down we don't drop it from
-       * our entry node list */
+      /* If we didn't have a reachable bridge before this one, try directory
+       * documents again. */
       if (first) {
         routerlist_retry_directory_downloads(now);
       }
@@ -835,34 +839,6 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
   }
 }
 
-/** Return the number of bridges that have descriptors that
- * are marked with purpose 'bridge' and are running.
- *
- * We use this function to decide if we're ready to start building
- * circuits through our bridges, or if we need to wait until the
- * directory "server/authority" requests finish. */
-MOCK_IMPL(int,
-any_bridge_descriptors_known, (void))
-{
-  if (BUG(!get_options()->UseBridges)) {
-    return 0;
-  }
-
-  if (!bridge_list)
-    return 0;
-
-  SMARTLIST_FOREACH_BEGIN(bridge_list, bridge_info_t *, bridge) {
-    const node_t *node;
-    if (!tor_digest_is_zero(bridge->identity) &&
-        (node = node_get_by_id(bridge->identity)) != NULL &&
-        node->ri) {
-      return 1;
-    }
-  } SMARTLIST_FOREACH_END(bridge);
-
-  return 0;
-}
-
 /** Return a smartlist containing all bridge identity digests */
 MOCK_IMPL(smartlist_t *,
 list_bridge_identities, (void))
diff --git a/src/or/bridges.h b/src/or/bridges.h
index 263c7d51b2..54a6250259 100644
--- a/src/or/bridges.h
+++ b/src/or/bridges.h
@@ -45,7 +45,6 @@ void bridge_add_from_config(struct bridge_line_t *bridge_line);
 void retry_bridge_descriptor_fetch_directly(const char *digest);
 void fetch_bridge_descriptors(const or_options_t *options, time_t now);
 void learned_bridge_descriptor(routerinfo_t *ri, int from_cache);
-MOCK_DECL(int, any_bridge_descriptors_known, (void));
 const smartlist_t *get_socks_args_by_bridge_addrport(const tor_addr_t *addr,
                                                      uint16_t port);
 
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index ebaa46e301..7baa035696 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -2079,8 +2079,12 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
     if (!connection_get_by_type(CONN_TYPE_DIR)) {
       int severity = LOG_NOTICE;
       /* Retry some stuff that might help the connection work. */
-      if (entry_list_is_constrained(options) &&
-          guards_retry_optimistic(options)) {
+      /* If we are configured with EntryNodes or UseBridges */
+      if (entry_list_is_constrained(options)) {
+        /* Retry all our guards / bridges.
+         * guards_retry_optimistic() always returns true here. */
+        int rv = guards_retry_optimistic(options);
+        tor_assert_nonfatal_once(rv);
         log_fn(severity, LD_APP|LD_DIR,
                "Application request when we haven't %s. "
                "Optimistically trying known %s again.",
@@ -2088,7 +2092,12 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
                "used client functionality lately" :
                "received a consensus with exits",
                options->UseBridges ? "bridges" : "entrynodes");
-      } else if (!options->UseBridges || any_bridge_descriptors_known()) {
+      } else {
+        /* Getting directory documents doesn't help much if we have a limited
+         * number of guards */
+        tor_assert_nonfatal(!options->UseBridges);
+        tor_assert_nonfatal(!options->EntryNodes);
+        /* Retry our directory fetches, so we have a fresh set of guard info */
         log_fn(severity, LD_APP|LD_DIR,
                "Application request when we haven't %s. "
                "Optimistically trying directory fetches again.",
diff --git a/src/or/directory.c b/src/or/directory.c
index 66bdef2361..f14bdde90b 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -5337,12 +5337,13 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options)
         }
       }
     case DL_SCHED_BRIDGE:
-      /* A bridge client downloading bridge descriptors */
-      if (options->UseBridges && any_bridge_descriptors_known()) {
-        /* A bridge client with one or more running bridges */
+      if (options->UseBridges && num_bridges_usable(0) > 0) {
+        /* A bridge client that is sure that one or more of its bridges are
+         * running can afford to wait longer to update bridge descriptors. */
         return options->TestingBridgeDownloadSchedule;
       } else {
-        /* A bridge client with no running bridges */
+        /* A bridge client which might have no running bridges, must try to
+         * get bridge descriptors straight away. */
         return options->TestingBridgeBootstrapDownloadSchedule;
       }
     default:
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index 3959da64cf..67b0259243 100644
--- a/src/or/entrynodes.c
+++ b/src/or/entrynodes.c
@@ -3136,20 +3136,34 @@ entry_list_is_constrained(const or_options_t *options)
 }
 
 /** Return the number of bridges that have descriptors that are marked with
- * purpose 'bridge' and are running.
- */
-int
-num_bridges_usable(void)
+ * purpose 'bridge' and are running. If use_maybe_reachable is
+ * true, include bridges that might be reachable in the count.
+ * Otherwise, if it is false, only include bridges that have recently been
+ * found running in the count.
+ *
+ * We use this function to decide if we're ready to start building
+ * circuits through our bridges, or if we need to wait until the
+ * directory "server/authority" requests finish. */
+MOCK_IMPL(int,
+num_bridges_usable,(int use_maybe_reachable))
 {
   int n_options = 0;
 
-  tor_assert(get_options()->UseBridges);
+  if (BUG(!get_options()->UseBridges)) {
+    return 0;
+  }
   guard_selection_t *gs  = get_guard_selection_info();
-  tor_assert(gs->type == GS_TYPE_BRIDGE);
+  if (BUG(gs->type != GS_TYPE_BRIDGE)) {
+    return 0;
+  }
 
   SMARTLIST_FOREACH_BEGIN(gs->sampled_entry_guards, entry_guard_t *, guard) {
+    /* Definitely not usable */
     if (guard->is_reachable == GUARD_REACHABLE_NO)
       continue;
+    /* If we want to be really sure the bridges will work, skip maybes */
+    if (!use_maybe_reachable && guard->is_reachable == GUARD_REACHABLE_MAYBE)
+      continue;
     if (tor_digest_is_zero(guard->identity))
       continue;
     const node_t *node = node_get_by_id(guard->identity);
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h
index d177f6cff4..d909115b1f 100644
--- a/src/or/entrynodes.h
+++ b/src/or/entrynodes.h
@@ -383,8 +383,7 @@ void entry_guards_note_internet_connectivity(guard_selection_t *gs);
 
 int update_guard_selection_choice(const or_options_t *options);
 
-/* Used by bridges.c only. */
-int num_bridges_usable(void);
+MOCK_DECL(int,num_bridges_usable,(int use_maybe_reachable));
 
 #ifdef ENTRYNODES_PRIVATE
 /**
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index f31529733d..e0a3e4cdc6 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1209,7 +1209,9 @@ should_delay_dir_fetches(const or_options_t *options, const char **msg_out)
   }
 
   if (options->UseBridges) {
-    if (!any_bridge_descriptors_known()) {
+    /* If we know that none of our bridges can possibly work, avoid fetching
+     * directory documents. But if some of them might work, try again. */
+    if (num_bridges_usable(1) == 0) {
       if (msg_out) {
         *msg_out = "No running bridges";
       }
diff --git a/src/test/test_dir.c b/src/test/test_dir.c
index ee4a9780b1..997b110d62 100644
--- a/src/test/test_dir.c
+++ b/src/test/test_dir.c
@@ -5875,11 +5875,12 @@ mock_networkstatus_consensus_can_use_extra_fallbacks(
   return mock_networkstatus_consensus_can_use_extra_fallbacks_value;
 }
 
-static int mock_any_bridge_descriptors_known_value = 0;
+static int mock_num_bridges_usable_value = 0;
 static int
-mock_any_bridge_descriptors_known(void)
+mock_num_bridges_usable(int use_maybe_reachable)
 {
-  return mock_any_bridge_descriptors_known_value;
+  (void)use_maybe_reachable;
+  return mock_num_bridges_usable_value;
 }
 
 /* data is a 3 character nul-terminated string.
@@ -5908,17 +5909,18 @@ test_dir_find_dl_schedule(void* data)
   }
 
   if (str[2] == 'r') {
-    mock_any_bridge_descriptors_known_value = 1;
+    /* Any positive, non-zero value should work */
+    mock_num_bridges_usable_value = 2;
   } else {
-    mock_any_bridge_descriptors_known_value = 0;
+    mock_num_bridges_usable_value = 0;
   }
 
   MOCK(networkstatus_consensus_is_bootstrapping,
        mock_networkstatus_consensus_is_bootstrapping);
   MOCK(networkstatus_consensus_can_use_extra_fallbacks,
        mock_networkstatus_consensus_can_use_extra_fallbacks);
-  MOCK(any_bridge_descriptors_known,
-       mock_any_bridge_descriptors_known);
+  MOCK(num_bridges_usable,
+       mock_num_bridges_usable);
 
   download_status_t dls;
   smartlist_t server, client, server_cons, client_cons;
@@ -6030,7 +6032,7 @@ test_dir_find_dl_schedule(void* data)
   /* client */
   mock_options->ClientOnly = 1;
   mock_options->UseBridges = 1;
-  if (any_bridge_descriptors_known()) {
+  if (num_bridges_usable(0) > 0) {
     tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge);
   } else {
     tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge_bootstrap);
@@ -6039,7 +6041,7 @@ test_dir_find_dl_schedule(void* data)
  done:
   UNMOCK(networkstatus_consensus_is_bootstrapping);
   UNMOCK(networkstatus_consensus_can_use_extra_fallbacks);
-  UNMOCK(any_bridge_descriptors_known);
+  UNMOCK(num_bridges_usable);
   UNMOCK(get_options);
   tor_free(mock_options);
   mock_options = NULL;
@@ -6217,7 +6219,7 @@ struct testcase_t dir_tests[] = {
   DIR(download_status_schedule, 0),
   DIR(download_status_random_backoff, 0),
   DIR(download_status_random_backoff_ranges, 0),
-  DIR(download_status_increment, 0),
+  DIR(download_status_increment, TT_FORK),
   DIR(authdir_type_to_string, 0),
   DIR(conn_purpose_to_string, 0),
   DIR(should_use_directory_guards, 0),