commit 690f646bf8a5de9b099fb5295ba9ff252e5606f4 Author: teor teor2345@gmail.com Date: Wed Nov 22 01:50:46 2017 +1100

Stop checking cached bridge descriptors for usable bridges

Stop checking for bridge descriptors when we actually want to know if any bridges are usable. This avoids potential bootstrapping issues. Fixes bug 24367; bugfix on 0.2.0.3-alpha.

Stop stalling when bridges are changed at runtime. Stop stalling when old bridge descriptors are cached, but they are not in use. Fixes bug 24367; bugfix on 23347 in 0.3.2.1-alpha. --- changes/bug24367 | 7 +++++++ src/or/bridges.c | 28 ---------------------------- src/or/bridges.h | 1 - src/or/circuituse.c | 2 +- src/or/directory.c | 2 +- src/or/entrynodes.c | 9 ++++++--- src/or/entrynodes.h | 3 +-- src/or/networkstatus.c | 2 +- src/test/test_dir.c | 19 ++++++++++--------- 9 files changed, 27 insertions(+), 46 deletions(-)

diff --git a/changes/bug24367 b/changes/bug24367 new file mode 100644 index 000000000..6081f7b5d --- /dev/null +++ b/changes/bug24367 @@ -0,0 +1,7 @@ + o Minor bugfixes (bridge clients, bootstrap): + - Stop checking for bridge descriptors when we actually want to know if + any bridges are usable. This avoids potential bootstrapping issues. + Fixes bug 24367; bugfix on 0.2.0.3-alpha. + - Stop stalling when bridges are changed at runtime. Stop stalling when + old bridge descriptors are cached, but they are not in use. + Fixes bug 24367; bugfix on 23347 in 0.3.2.1-alpha. diff --git a/src/or/bridges.c b/src/or/bridges.c index 320f5ee63..ac759493c 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -835,34 +835,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 263c7d51b..54a625025 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 aa0df9565..8c9859bab 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -2084,7 +2084,7 @@ 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 if (!options->UseBridges || num_bridges_usable() > 0) { 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 66bdef236..e49fd595c 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5338,7 +5338,7 @@ 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()) { + if (options->UseBridges && num_bridges_usable() > 0) { /* A bridge client with one or more running bridges */ return options->TestingBridgeDownloadSchedule; } else { diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 866e59335..826c0e48a 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -3136,9 +3136,12 @@ 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) + * + * 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,(void)) { int n_options = 0;

diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index d177f6cff..ad6b47936 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,(void));

#ifdef ENTRYNODES_PRIVATE /** diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index f31529733..2075fccc9 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1209,7 +1209,7 @@ should_delay_dir_fetches(const or_options_t *options, const char **msg_out) }

if (options->UseBridges) { - if (!any_bridge_descriptors_known()) { + if (num_bridges_usable() == 0) { if (msg_out) { *msg_out = "No running bridges"; } diff --git a/src/test/test_dir.c b/src/test/test_dir.c index ee4a9780b..78bf23608 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -5875,11 +5875,11 @@ 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(void) { - return mock_any_bridge_descriptors_known_value; + return mock_num_bridges_usable_value; }

/* data is a 3 character nul-terminated string. @@ -5908,17 +5908,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 +6031,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) { 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 +6040,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;