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 f6e3e419d3..19a23ea86b 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -799,7 +799,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); @@ -829,8 +833,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); } @@ -838,34 +842,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 825535739f..f04448ffce 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -2074,8 +2074,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.", @@ -2083,7 +2087,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 dcdcff396f..c55f81bc62 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5345,12 +5345,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 cbdb36aa43..fa8d62a4f0 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 21ba706c5b..aa9c8fe193 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -386,8 +386,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 61cca1b9b6..deef62bd40 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1218,7 +1218,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 c85f7f0652..6092a5a4d0 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -5877,11 +5877,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. @@ -5910,17 +5911,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; @@ -6032,7 +6034,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); @@ -6041,7 +6043,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; @@ -6319,7 +6321,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),