commit 93a8ed3b83b5f20768562ca2aff4eba7aca667d8 Author: teor teor2345@gmail.com Date: Mon Sep 11 13:56:35 2017 +1000
Make clients wait to refresh bridges when they have a recent descriptor
But when clients are just starting, make them try each bridge a few times before giving up on it.
These changes make the bridge download schedules more explicit: before 17750, they relied on undocumented behaviour and specific schedule entries. (And between 17750 and this fix, they were broken.)
Fixes 23347, not in any released version of tor. --- changes/bug23347 | 15 +++++++++------ doc/tor.1.txt | 16 ++++++++++++---- src/or/bridges.c | 17 +++++------------ src/or/bridges.h | 2 +- src/or/config.c | 19 +++++++++++++------ src/or/directory.c | 9 ++++++++- src/or/or.h | 4 ++++ src/test/test_dir.c | 48 +++++++++++++++++++++++++++++++++++++++--------- src/test/test_options.c | 1 + 9 files changed, 92 insertions(+), 39 deletions(-)
diff --git a/changes/bug23347 b/changes/bug23347 index b0883784d..e73aa48f0 100644 --- a/changes/bug23347 +++ b/changes/bug23347 @@ -1,6 +1,9 @@ - o Minor bug fixes (bridge client bootstrap): - - Make bridge clients download bridge descriptors immediately. - The bridge schedules were never correct, but we didn't realise until - the fix for 17750 made bridge clients hang for 15 minutes before - bootstrapping. - Fixes bug 23347. Not in any released version of Tor. + o Minor fixes (bridge client bootstrap): + - Make bridge clients with no running bridges try to download + bridge descriptors immediately. But when bridge clients have + running bridges, make them wait at least 3 hours before + refreshing recently received bridge descriptors. + Download schedules used to start with an implicit 0, but the + fix for 17750 changed this undocumented behaviour, and made + bridge clients hang for 15 minutes before bootstrapping. + Fixes bug 23347, not in any released version of Tor. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index e3939d115..d8dbcaa13 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2475,7 +2475,8 @@ The following options are used for running a testing Tor network. TestingClientDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 TestingServerConsensusDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 TestingClientConsensusDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 - TestingBridgeDownloadSchedule 0, 5, 10, 30, 60 + TestingBridgeDownloadSchedule 10, 30, 60 + TestingBridgeBootstrapDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60 TestingClientMaxIntervalWithoutRequest 5 seconds TestingDirConnectionMaxStall 30 seconds TestingConsensusMaxDownloadTries 80 @@ -2540,9 +2541,16 @@ The following options are used for running a testing Tor network. 1800, 3600, 3600, 3600, 10800, 21600, 43200)
[[TestingBridgeDownloadSchedule]] **TestingBridgeDownloadSchedule** __N__,__N__,__...__:: - Schedule for when clients should download bridge descriptors. Changing this - requires that **TestingTorNetwork** is set. (Default: 0, 8, 3600, 10800, - 25200, 54000, 111600, 262800) + Schedule for when clients should download each bridge descriptor when they + know that one or more of their configured bridges are running. Changing + this requires that **TestingTorNetwork** is set. (Default: 10800, 25200, + 54000, 111600, 262800) + +[[TestingBridgeBootstrapDownloadSchedule]] **TestingBridgeBootstrapDownloadSchedule** __N__,__N__,__...__:: + Schedule for when clients should download each bridge descriptor when they + have just started, or when they can not contact any of their bridges. + Changing this requires that **TestingTorNetwork** is set. (Default: 0, 30, + 90, 600, 3600, 10800, 25200, 54000, 111600, 262800)
[[TestingClientMaxIntervalWithoutRequest]] **TestingClientMaxIntervalWithoutRequest** __N__ **seconds**|**minutes**:: When directory clients have only a few descriptors to request, they batch diff --git a/src/or/bridges.c b/src/or/bridges.c index fc39ccaa9..257bb8920 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -794,17 +794,10 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) node_t *node; /* it's here; schedule its re-fetch for a long time from now. */ if (!from_cache) { + /* This schedules the re-fetch at a constant interval, which produces + * a pattern of bridge traffic. But it's better than trying all + * configured briges several times in the first few minutes. */ download_status_reset(&bridge->fetch_status); - /* We have two quick attempts in the bridge schedule, and then slow - * ones */ - download_status_increment_attempt( - &bridge->fetch_status, - safe_str_client(fmt_and_decorate_addr(&bridge->addr)), - now); - download_status_increment_attempt( - &bridge->fetch_status, - safe_str_client(fmt_and_decorate_addr(&bridge->addr)), - now); }
node = node_get_mutable_by_id(ri->cache_info.identity_digest); @@ -837,8 +830,8 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) * 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. */ -int -any_bridge_descriptors_known(void) +MOCK_IMPL(int, +any_bridge_descriptors_known, (void)) { tor_assert(get_options()->UseBridges);
diff --git a/src/or/bridges.h b/src/or/bridges.h index 3bfc782f9..19341818f 100644 --- a/src/or/bridges.h +++ b/src/or/bridges.h @@ -45,7 +45,7 @@ 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); -int any_bridge_descriptors_known(void); +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/config.c b/src/or/config.c index 54df6c3e5..90ab0e57f 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -586,10 +586,16 @@ static config_var_t option_vars_[] = { * blackholed. Clients will try 3 directories simultaneously. * (Relays never use simultaneous connections.) */ V(ClientBootstrapConsensusMaxInProgressTries, UINT, "3"), - /* The bridge code relies on the third item in this schedule being slow - * (~ 1 consensus interval) */ + /* When a client has any running bridges, check each bridge occasionally, + * whether or not that bridge is actually up. */ V(TestingBridgeDownloadSchedule, CSV_INTERVAL, - "0, 8, 3600, 10800, 25200, 54000, 111600, 262800"), + "10800, 25200, 54000, 111600, 262800"), + /* When a client is just starting, or has no running bridges, check each + * bridge a few times quickly, and then try again later. These schedules + * are much longer than the other schedules, because we try each and every + * configured bridge with this schedule. */ + V(TestingBridgeBootstrapDownloadSchedule, CSV_INTERVAL, + "0, 30, 90, 600, 3600, 10800, 25200, 54000, 111600, 262800"), V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "10 minutes"), V(TestingDirConnectionMaxStall, INTERVAL, "5 minutes"), V(TestingConsensusMaxDownloadTries, UINT, "8"), @@ -651,9 +657,9 @@ static const config_var_t testing_tor_network_defaults[] = { "15, 20, 30, 60"), V(TestingClientConsensusDownloadSchedule, CSV_INTERVAL, "0, 0, 5, 10, " "15, 20, 30, 60"), - /* The bridge code relies on the third item in this schedule being slow - * (~ 1 consensus interval) */ - V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "0, 5, 10, 30, 60"), + V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "10, 30, 60"), + V(TestingBridgeBootstrapDownloadSchedule, CSV_INTERVAL, "0, 0, 5, 10, " + "15, 20, 30, 60"), V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "5 seconds"), V(TestingDirConnectionMaxStall, INTERVAL, "30 seconds"), V(TestingConsensusMaxDownloadTries, UINT, "80"), @@ -4066,6 +4072,7 @@ options_validate(or_options_t *old_options, or_options_t *options, CHECK_DEFAULT(TestingServerConsensusDownloadSchedule); CHECK_DEFAULT(TestingClientConsensusDownloadSchedule); CHECK_DEFAULT(TestingBridgeDownloadSchedule); + CHECK_DEFAULT(TestingBridgeBootstrapDownloadSchedule); CHECK_DEFAULT(TestingClientMaxIntervalWithoutRequest); CHECK_DEFAULT(TestingDirConnectionMaxStall); CHECK_DEFAULT(TestingConsensusMaxDownloadTries); diff --git a/src/or/directory.c b/src/or/directory.c index 6b5e16bfd..9ee6fae4d 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5342,7 +5342,14 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options) } } case DL_SCHED_BRIDGE: - return options->TestingBridgeDownloadSchedule; + /* A bridge client downloading bridge descriptors */ + if (any_bridge_descriptors_known()) { + /* A bridge client with one or more running bridges */ + return options->TestingBridgeDownloadSchedule; + } else { + /* A bridge client with no running bridges */ + return options->TestingBridgeBootstrapDownloadSchedule; + } default: tor_assert(0); } diff --git a/src/or/or.h b/src/or/or.h index 5d55094a0..78b658a79 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4314,6 +4314,10 @@ typedef struct { * altered on testing networks. */ smartlist_t *TestingBridgeDownloadSchedule;
+ /** Schedule for when clients should download bridge descriptors when they + * have no running bridges. Only altered on testing networks. */ + smartlist_t *TestingBridgeBootstrapDownloadSchedule; + /** When directory clients have only a few descriptors to request, they * batch them until they have more, or until this amount of time has * passed. Only altered on testing networks. */ diff --git a/src/test/test_dir.c b/src/test/test_dir.c index b6b0936f8..dd910cd0c 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -18,6 +18,7 @@ #define RELAY_PRIVATE
#include "or.h" +#include "bridges.h" #include "confparse.h" #include "config.h" #include "control.h" @@ -4245,7 +4246,7 @@ test_dir_download_status_increment(void *arg) /* Put it in the options */ mock_options = &test_options; reset_options(mock_options, &mock_get_options_calls); - mock_options->TestingBridgeDownloadSchedule = schedule; + mock_options->TestingBridgeBootstrapDownloadSchedule = schedule; mock_options->TestingClientDownloadSchedule = schedule;
MOCK(get_options, mock_get_options); @@ -4425,6 +4426,7 @@ test_dir_download_status_increment(void *arg)
/* Check that failure increments do happen on attempt-based schedules, * but that the retry is set at the end of time */ + mock_options->UseBridges = 1; mock_get_options_calls = 0; next_at = download_status_increment_failure(&dls_attempt, 404, "test", 0, current_time); @@ -4539,6 +4541,7 @@ test_dir_download_status_increment(void *arg) tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0); tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0); tt_int_op(mock_get_options_calls, OP_GE, 1); + mock_options->UseBridges = 0;
/* Check that attempt increments don't happen on failure-based schedules, * and that the attempt is set at the end of time */ @@ -5859,9 +5862,17 @@ mock_networkstatus_consensus_can_use_extra_fallbacks( return mock_networkstatus_consensus_can_use_extra_fallbacks_value; }
-/* data is a 2 character nul-terminated string. +static int mock_any_bridge_descriptors_known_value = 0; +static int +mock_any_bridge_descriptors_known(void) +{ + return mock_any_bridge_descriptors_known_value; +} + +/* data is a 3 character nul-terminated string. * If data[0] is 'b', set bootstrapping, anything else means not bootstrapping * If data[1] is 'f', set extra fallbacks, anything else means no extra + * If data[2] is 'f', set running bridges, anything else means no extra * fallbacks. */ static void @@ -5869,7 +5880,7 @@ test_dir_find_dl_schedule(void* data) { const char *str = (const char *)data;
- tt_assert(strlen(data) == 2); + tt_assert(strlen(data) == 3);
if (str[0] == 'b') { mock_networkstatus_consensus_is_bootstrapping_value = 1; @@ -5883,15 +5894,23 @@ test_dir_find_dl_schedule(void* data) mock_networkstatus_consensus_can_use_extra_fallbacks_value = 0; }
+ if (str[2] == 'r') { + mock_any_bridge_descriptors_known_value = 1; + } else { + mock_any_bridge_descriptors_known_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);
download_status_t dls; smartlist_t server, client, server_cons, client_cons; smartlist_t client_boot_auth_only_cons, client_boot_auth_cons; - smartlist_t client_boot_fallback_cons, bridge; + smartlist_t client_boot_fallback_cons, bridge, bridge_bootstrap;
mock_options = tor_malloc(sizeof(or_options_t)); reset_options(mock_options, &mock_get_options_calls); @@ -5908,6 +5927,7 @@ test_dir_find_dl_schedule(void* data) mock_options->ClientBootstrapConsensusFallbackDownloadSchedule = &client_boot_fallback_cons; mock_options->TestingBridgeDownloadSchedule = &bridge; + mock_options->TestingBridgeBootstrapDownloadSchedule = &bridge_bootstrap;
dls.schedule = DL_SCHED_GENERIC; /* client */ @@ -5996,11 +6016,17 @@ test_dir_find_dl_schedule(void* data) dls.schedule = DL_SCHED_BRIDGE; /* client */ mock_options->ClientOnly = 1; - tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge); + mock_options->UseBridges = 1; + if (any_bridge_descriptors_known()) { + 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); + }
done: UNMOCK(networkstatus_consensus_is_bootstrapping); UNMOCK(networkstatus_consensus_can_use_extra_fallbacks); + UNMOCK(any_bridge_descriptors_known); UNMOCK(get_options); tor_free(mock_options); mock_options = NULL; @@ -6165,10 +6191,14 @@ struct testcase_t dir_tests[] = { DIR(dump_unparseable_descriptors, 0), DIR(populate_dump_desc_fifo, 0), DIR(populate_dump_desc_fifo_2, 0), - DIR_ARG(find_dl_schedule, TT_FORK, "bf"), - DIR_ARG(find_dl_schedule, TT_FORK, "ba"), - DIR_ARG(find_dl_schedule, TT_FORK, "cf"), - DIR_ARG(find_dl_schedule, TT_FORK, "ca"), + DIR_ARG(find_dl_schedule, TT_FORK, "bfd"), + DIR_ARG(find_dl_schedule, TT_FORK, "bad"), + DIR_ARG(find_dl_schedule, TT_FORK, "cfd"), + DIR_ARG(find_dl_schedule, TT_FORK, "cad"), + DIR_ARG(find_dl_schedule, TT_FORK, "bfr"), + DIR_ARG(find_dl_schedule, TT_FORK, "bar"), + DIR_ARG(find_dl_schedule, TT_FORK, "cfr"), + DIR_ARG(find_dl_schedule, TT_FORK, "car"), DIR(assumed_flags, 0), DIR(networkstatus_compute_bw_weights_v10, 0), END_OF_TESTCASES diff --git a/src/test/test_options.c b/src/test/test_options.c index 5f35c47f7..a9f18c8a8 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -2230,6 +2230,7 @@ test_options_validate__testing(void *ignored) ENSURE_DEFAULT(TestingServerConsensusDownloadSchedule, 3000); ENSURE_DEFAULT(TestingClientConsensusDownloadSchedule, 3000); ENSURE_DEFAULT(TestingBridgeDownloadSchedule, 3000); + ENSURE_DEFAULT(TestingBridgeBootstrapDownloadSchedule, 3000); ENSURE_DEFAULT(TestingClientMaxIntervalWithoutRequest, 3000); ENSURE_DEFAULT(TestingDirConnectionMaxStall, 3000); ENSURE_DEFAULT(TestingConsensusMaxDownloadTries, 3000);