From 690f646bf8a5de9b099fb5295ba9ff252e5606f4 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 22 Nov 2017 01:50:46 +1100 Subject: [PATCH] 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(-) create mode 100644 changes/bug24367 diff --git a/changes/bug24367 b/changes/bug24367 new file mode 100644 index 0000000000..6081f7b5d4 --- /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 320f5ee632..ac759493cc 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 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 aa0df95652..8c9859bab7 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 66bdef2361..e49fd595c9 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 866e593350..826c0e48a0 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 d177f6cff4..ad6b47936f 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 f31529733d..2075fccc92 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 ee4a9780b1..78bf23608a 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; -- 2.11.4.GIT