From 435952538da2d99e7ce46f510ac3f7dc3f910fca Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 29 Aug 2017 13:48:30 +1000 Subject: [PATCH 1/5] Make the download defaults in the tor manual match the code Documentation fix on commit 667ba77, which was part of #20534. --- doc/tor.1.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 03af9c4be8..9949f9dd0e 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1571,8 +1571,8 @@ The following options are useful only for clients (that is, if live consensus). Only used by clients fetching from a list of fallback directory mirrors. This schedule is advanced by (potentially concurrent) connection attempts, unlike other schedules, which are advanced by - connection failures. (Default: 10, 11, 3600, 10800, 25200, 54000, - 111600, 262800) + connection failures. (Default: 6, 11, 3600, 10800, 25200, 54000, 111600, + 262800) [[ClientBootstrapConsensusFallbackDownloadSchedule]] **ClientBootstrapConsensusFallbackDownloadSchedule** __N__,__N__,__...__:: Schedule for when clients should download consensuses from fallback @@ -1602,7 +1602,7 @@ The following options are useful only for clients (that is, if [[ClientBootstrapConsensusMaxInProgressTries]] **ClientBootstrapConsensusMaxInProgressTries** __NUM__:: Try this many simultaneous connections to download a consensus before - waiting for one to complete, timeout, or error out. (Default: 4) + waiting for one to complete, timeout, or error out. (Default: 3) SERVER OPTIONS -------------- @@ -2541,7 +2541,7 @@ The following options are used for running a testing Tor network. [[TestingBridgeDownloadSchedule]] **TestingBridgeDownloadSchedule** __N__,__N__,__...__:: Schedule for when clients should download bridge descriptors. Changing this - requires that **TestingTorNetwork** is set. (Default: 3600, 900, 900, 3600) + requires that **TestingTorNetwork** is set. (Default: 1200, 900, 900, 3600) [[TestingClientMaxIntervalWithoutRequest]] **TestingClientMaxIntervalWithoutRequest** __N__ **seconds**|**minutes**:: When directory clients have only a few descriptors to request, they batch From 97249c4f5e389141cb4ff3562a3813bd09bff6ad Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 29 Aug 2017 14:04:24 +1000 Subject: [PATCH 2/5] Make bridge clients download bridge descriptors immediately The download schedule tells Tor to wait 15 minutes before downloading bridge descriptors. But 17750 made Tor ignore that and start immediately. Since we fixed 17750, Tor waits 15 minutes for bridge client bootstrap, like the schedule says. This fixes the download schedule to start immediately, and to try each bridge 3 times in the first 30 seconds. This should make bridge bootstraps more reliable. Fixes 23347. --- changes/bug23347 | 6 ++++++ doc/tor.1.txt | 5 +++-- src/or/bridges.c | 11 +++++++++-- src/or/config.c | 9 +++++++-- src/or/directory.c | 4 ++-- 5 files changed, 27 insertions(+), 8 deletions(-) create mode 100644 changes/bug23347 diff --git a/changes/bug23347 b/changes/bug23347 new file mode 100644 index 0000000000..b0883784d2 --- /dev/null +++ b/changes/bug23347 @@ -0,0 +1,6 @@ + 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. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 9949f9dd0e..e3939d115a 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2475,7 +2475,7 @@ 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 60, 30, 30, 60 + TestingBridgeDownloadSchedule 0, 5, 10, 30, 60 TestingClientMaxIntervalWithoutRequest 5 seconds TestingDirConnectionMaxStall 30 seconds TestingConsensusMaxDownloadTries 80 @@ -2541,7 +2541,8 @@ The following options are used for running a testing Tor network. [[TestingBridgeDownloadSchedule]] **TestingBridgeDownloadSchedule** __N__,__N__,__...__:: Schedule for when clients should download bridge descriptors. Changing this - requires that **TestingTorNetwork** is set. (Default: 1200, 900, 900, 3600) + requires that **TestingTorNetwork** is set. (Default: 0, 8, 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 0818fb0812..f2244004f3 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -632,7 +632,9 @@ fetch_bridge_descriptors(const or_options_t *options, time_t now) continue; } - /* schedule another fetch as if this one will fail, in case it does */ + /* schedule another fetch as if this one will fail, in case it does + * (we can't increment after a failure, because sometimes we use the + * bridge authority, and sometimes we use the bridge direct) */ download_status_failed(&bridge->fetch_status, 0); can_use_bridge_authority = !tor_digest_is_zero(bridge->identity) && @@ -787,8 +789,13 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) if (bridge) { /* if we actually want to use this one */ node_t *node; /* it's here; schedule its re-fetch for a long time from now. */ - if (!from_cache) + if (!from_cache) { download_status_reset(&bridge->fetch_status); + /* We have two quick attempts in the bridge schedule, and then slow + * ones */ + download_status_failed(&bridge->fetch_status, 0); + download_status_failed(&bridge->fetch_status, 0); + } node = node_get_mutable_by_id(ri->cache_info.identity_digest); tor_assert(node); diff --git a/src/or/config.c b/src/or/config.c index 30853724e4..54df6c3e58 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -586,7 +586,10 @@ static config_var_t option_vars_[] = { * blackholed. Clients will try 3 directories simultaneously. * (Relays never use simultaneous connections.) */ V(ClientBootstrapConsensusMaxInProgressTries, UINT, "3"), - V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "1200, 900, 900, 3600"), + /* The bridge code relies on the third item in this schedule being slow + * (~ 1 consensus interval) */ + V(TestingBridgeDownloadSchedule, CSV_INTERVAL, + "0, 8, 3600, 10800, 25200, 54000, 111600, 262800"), V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "10 minutes"), V(TestingDirConnectionMaxStall, INTERVAL, "5 minutes"), V(TestingConsensusMaxDownloadTries, UINT, "8"), @@ -648,7 +651,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"), - V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "60, 30, 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(TestingClientMaxIntervalWithoutRequest, INTERVAL, "5 seconds"), V(TestingDirConnectionMaxStall, INTERVAL, "30 seconds"), V(TestingConsensusMaxDownloadTries, UINT, "80"), diff --git a/src/or/directory.c b/src/or/directory.c index 57dfdd9cac..6b5e16bfd4 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -5663,8 +5663,8 @@ download_status_get_initial_delay_from_now(const download_status_t *dls) * (We find the zeroth element of the download schedule, and set * next_attempt_at to be the appropriate offset from 'now'. In most * cases this means setting it to 'now', so the item will be immediately - * downloadable; in the case of bridge descriptors, the zeroth element - * is an hour from now.) */ + * downloadable; when using authorities with fallbacks, there is a few seconds' + * delay.) */ void download_status_reset(download_status_t *dls) { From 6370fb77c586e9ad68c7d1ecb95be36137cb9067 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 29 Aug 2017 15:25:33 +1000 Subject: [PATCH 3/5] Refactor bridge download statuses to increment on attempt We were always incrementing bridge download statuses on each attempt, but we were using the "increment on failure" functions to do it. And we never incremented them on failure. No behaviour change. --- src/or/bridges.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/or/bridges.c b/src/or/bridges.c index f2244004f3..fc39ccaa94 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -454,6 +454,7 @@ bridge_add_from_config(bridge_line_t *bridge_line) b->transport_name = bridge_line->transport_name; b->fetch_status.schedule = DL_SCHED_BRIDGE; b->fetch_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; + b->fetch_status.increment_on = DL_SCHED_INCREMENT_ATTEMPT; b->socks_args = bridge_line->socks_args; if (!bridge_list) bridge_list = smartlist_new(); @@ -632,10 +633,13 @@ fetch_bridge_descriptors(const or_options_t *options, time_t now) continue; } - /* schedule another fetch as if this one will fail, in case it does - * (we can't increment after a failure, because sometimes we use the - * bridge authority, and sometimes we use the bridge direct) */ - download_status_failed(&bridge->fetch_status, 0); + /* schedule the next attempt + * we can't increment after a failure, because sometimes we use the + * bridge authority, and sometimes we use the bridge direct */ + download_status_increment_attempt( + &bridge->fetch_status, + safe_str_client(fmt_and_decorate_addr(&bridge->addr)), + now); can_use_bridge_authority = !tor_digest_is_zero(bridge->identity) && num_bridge_auths; @@ -793,8 +797,14 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) download_status_reset(&bridge->fetch_status); /* We have two quick attempts in the bridge schedule, and then slow * ones */ - download_status_failed(&bridge->fetch_status, 0); - download_status_failed(&bridge->fetch_status, 0); + 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); From 93a8ed3b83b5f20768562ca2aff4eba7aca667d8 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 11 Sep 2017 13:56:35 +1000 Subject: [PATCH 4/5] 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 b0883784d2..e73aa48f01 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 e3939d115a..d8dbcaa13f 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 fc39ccaa94..257bb8920b 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 3bfc782f9a..19341818f4 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 54df6c3e58..90ab0e57fe 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 6b5e16bfd4..9ee6fae4dc 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 5d55094a02..78b658a793 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 b6b0936f83..dd910cd0c5 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 5f35c47f79..a9f18c8a82 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); From 1b5e34badb06bb1a844a6e70164fc5c894d95d0a Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 11 Sep 2017 14:03:42 +1000 Subject: [PATCH 5/5] Add a missing bridge download status reset 17750 made unused download schedules automatically reset. But we should make that explicit. --- src/or/bridges.c | 2 ++ src/test/test_entrynodes.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/or/bridges.c b/src/or/bridges.c index 257bb8920b..461f86260f 100644 --- a/src/or/bridges.c +++ b/src/or/bridges.c @@ -455,6 +455,8 @@ bridge_add_from_config(bridge_line_t *bridge_line) b->fetch_status.schedule = DL_SCHED_BRIDGE; b->fetch_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; b->fetch_status.increment_on = DL_SCHED_INCREMENT_ATTEMPT; + /* This will fail if UseBridges is not set */ + download_status_reset(&b->fetch_status); b->socks_args = bridge_line->socks_args; if (!bridge_list) bridge_list = smartlist_new(); diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index f30bb5ffa1..d87670d82b 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -917,6 +917,7 @@ test_entry_guard_node_filter(void *arg) routerset_parse(get_options_mutable()->ExcludeNodes, "144.144.0.0/16", ""); /* 4: Bridge. */ + get_options_mutable()->UseBridges = 1; sweep_bridge_list(); bl = tor_malloc_zero(sizeof(bridge_line_t)); tor_addr_from_ipv4h(&bl->addr, n[4]->rs->addr); @@ -924,6 +925,7 @@ test_entry_guard_node_filter(void *arg) memcpy(bl->digest, n[4]->identity, 20); bridge_add_from_config(bl); bl = NULL; // prevent free. + get_options_mutable()->UseBridges = 0; /* 5: Unreachable. This stays in the filter, but isn't in usable-filtered */ g[5]->last_tried_to_connect = approx_time(); // prevent retry.