From 1b22eae120ff379f7218b4e8b4fb62ed2bfede73 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 6 Nov 2016 19:50:08 -0500 Subject: [PATCH 1/6] Fix get_delay() code to avoid TIME_MAX overflow, not INT_MAX. Fixes bug 20587; bugfix on 35bbf2e4a4e8ccb in 0.2.8.1-alpha. --- changes/bug20587 | 6 ++++++ src/or/directory.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changes/bug20587 diff --git a/changes/bug20587 b/changes/bug20587 new file mode 100644 index 0000000000..a05933427c --- /dev/null +++ b/changes/bug20587 @@ -0,0 +1,6 @@ + + o Minor bugfixes (download timing): + - When determining when to download a directory object, handle times + after 2038 if the operating system supports that. (Someday this will be + important!) Fixes bug 20587; bugfix on 0.2.8.1-alpha. + diff --git a/src/or/directory.c b/src/or/directory.c index 1f894d9fb3..afe5796afb 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3876,9 +3876,9 @@ download_status_schedule_get_delay(download_status_t *dls, * non-negative allows us to safely do the wrapping check below. */ tor_assert(delay >= 0); - /* Avoid now+delay overflowing INT_MAX, by comparing with a subtraction + /* Avoid now+delay overflowing TIME_MAX, by comparing with a subtraction * that won't overflow (since delay is non-negative). */ - if (delay < INT_MAX && now <= INT_MAX - delay) { + if (delay < INT_MAX && now <= TIME_MAX - delay) { dls->next_attempt_at = now+delay; } else { dls->next_attempt_at = TIME_MAX; From e9ce1819550f40132c30433914ff95b212957db0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 6 Nov 2016 20:01:24 -0500 Subject: [PATCH 2/6] Change a BUG warning to be a warning, not an info. --- src/or/directory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/directory.c b/src/or/directory.c index afe5796afb..24490b7426 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3991,7 +3991,7 @@ download_status_increment_attempt(download_status_t *dls, const char *item, if (dls->increment_on == DL_SCHED_INCREMENT_FAILURE) { /* this schedule should retry on failure, and not launch any concurrent attempts */ - log_info(LD_BUG, "Tried to launch an attempt-based connection on a " + log_warn(LD_BUG, "Tried to launch an attempt-based connection on a " "failure-based schedule."); return TIME_MAX; } From 5385a023e105ffbb54f1c160298313c6a8cde57f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 6 Nov 2016 20:08:11 -0500 Subject: [PATCH 3/6] Do not apply 'max_failures' to random-exponential schedules. Fixes bug 20536; bugfix on 0.2.9.1-alpha. --- changes/bug20536 | 6 ++++++ src/or/directory.h | 12 +++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 changes/bug20536 diff --git a/changes/bug20536 b/changes/bug20536 new file mode 100644 index 0000000000..9e0dd164bb --- /dev/null +++ b/changes/bug20536 @@ -0,0 +1,6 @@ + o Major bugfixes (download scheduling): + - When using an exponential backoff schedule, do not give up on + dowloading just because we have failed a bunch of times. Since + each delay is longer than the last, retrying indefinitely won't + hurt. Fixes bug 20536; bugfix on 0.2.9.1-alpha. + diff --git a/src/or/directory.h b/src/or/directory.h index 9477948aa0..629b3ead90 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -114,9 +114,15 @@ static inline int download_status_is_ready(download_status_t *dls, time_t now, int max_failures) { - int under_failure_limit = (dls->n_download_failures <= max_failures - && dls->n_download_attempts <= max_failures); - return (under_failure_limit && dls->next_attempt_at <= now); + if (dls->backoff == DL_SCHED_DETERMINISTIC) { + /* Deterministic schedules can hit an endpoint; exponential backoff + * schedules just wait longer and longer. */ + int under_failure_limit = (dls->n_download_failures <= max_failures + && dls->n_download_attempts <= max_failures); + if (!under_failure_limit) + return 0; + } + return dls->next_attempt_at <= now; } static void download_status_mark_impossible(download_status_t *dl); From 1bb28cecd92cb72f6df3b7bdf47bdbfa53c5c6dc Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 7 Nov 2016 22:58:36 +1100 Subject: [PATCH 4/6] Ensure relays don't make multiple connections during bootstrap Relays do not deliberately launch multiple attempts, so the impact of this bug should be minimal. This fix also defends against bugs like #20499. Bugfix on 0.2.8.1-alpha. --- changes/bug20591 | 3 +++ src/or/networkstatus.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changes/bug20591 diff --git a/changes/bug20591 b/changes/bug20591 new file mode 100644 index 0000000000..deaa738f5e --- /dev/null +++ b/changes/bug20591 @@ -0,0 +1,3 @@ + o Minor bugfixes (relay bootstrap): + - Ensure relays don't make multiple connections during bootstrap. + Fixes bug 20591; bugfix on 0.2.8.1-alpha. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 49baeb83b7..a6656f5596 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -880,7 +880,7 @@ update_consensus_networkstatus_downloads(time_t now) resource = networkstatus_get_flavor_name(i); /* Check if we already have enough connections in progress */ - if (we_are_bootstrapping) { + if (we_are_bootstrapping && use_multi_conn) { max_in_progress_conns = options->ClientBootstrapConsensusMaxInProgressTries; } From e819d420c5b8e1d1b632c27d621de26f5f489663 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 8 Nov 2016 00:01:20 +1100 Subject: [PATCH 5/6] When downloading certificates, check for related failures If a consensus expires while we are waiting for certificates to download, stop waiting for certificates. If we stop waiting for certificates less than a minute after we started downloading them, do not consider the certificate download failure a separate failure. Fixes bug 20533; bugfix on commit e0204f21 in 0.2.0.9-alpha. --- changes/bug20533 | 7 +++++++ src/or/networkstatus.c | 17 +++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) create mode 100644 changes/bug20533 diff --git a/changes/bug20533 b/changes/bug20533 new file mode 100644 index 0000000000..7d1a456328 --- /dev/null +++ b/changes/bug20533 @@ -0,0 +1,7 @@ + o Minor bugfixes (consensus downloads): + - If a consensus expires while we are waiting for certificates to download, + stop waiting for certificates. + - If we stop waiting for certificates less than a minute after we started + downloading them, do not consider the certificate download failure a + separate failure. + Fixes bug 20533; bugfix on commit e0204f21 in 0.2.0.9-alpha. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 49baeb83b7..306857f018 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -815,9 +815,15 @@ we_want_to_fetch_flavor(const or_options_t *options, int flavor) * fetching certs before we check whether there is a better one? */ #define DELAY_WHILE_FETCHING_CERTS (20*60) +/** What is the minimum time we need to have waited fetching certs, before we + * increment the consensus download schedule on failure? */ +#define MIN_DELAY_FOR_FETCH_CERT_STATUS_FAILURE (1*60) + /* Check if a downloaded consensus flavor should still wait for certificates - * to download now. - * If so, return 1. If not, fail dls and return 0. */ + * to download now. If we decide not to wait, check if enough time has passed + * to consider the certificate download failure a separate failure. If so, + * fail dls. + * If waiting for certificates to download, return 1. If not, return 0. */ static int check_consensus_waiting_for_certs(int flavor, time_t now, download_status_t *dls) @@ -831,11 +837,14 @@ check_consensus_waiting_for_certs(int flavor, time_t now, waiting = &consensus_waiting_for_certs[flavor]; if (waiting->consensus) { /* XXXX make sure this doesn't delay sane downloads. */ - if (waiting->set_at + DELAY_WHILE_FETCHING_CERTS > now) { + if (waiting->set_at + DELAY_WHILE_FETCHING_CERTS > now && + waiting->consensus->valid_until > now) { return 1; } else { if (!waiting->dl_failed) { - download_status_failed(dls, 0); + if (waiting->set_at + MIN_DELAY_FOR_FETCH_CERT_STATUS_FAILURE > now) { + download_status_failed(dls, 0); + } waiting->dl_failed=1; } } From 858867a31a31b7a0065481b1f5ad108f02ab337a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 6 Nov 2016 20:14:34 -0500 Subject: [PATCH 6/6] Allow infinitely long delays in exponential-backoff downloads It's only safe to remove the failure limit (per 20536) if we are in fact waiting a bit longer each time we try to download. Fixes bug 20534; bugfix on 0.2.9.1-alpha. --- changes/bug20534 | 6 ++++++ src/or/directory.c | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 changes/bug20534 diff --git a/changes/bug20534 b/changes/bug20534 new file mode 100644 index 0000000000..1ffa1f32e9 --- /dev/null +++ b/changes/bug20534 @@ -0,0 +1,6 @@ + o Minor bugfixes (directory download scheduling): + - Remove the maximum delay on exponential-backoff scheduling. + Since we now allow an infinite number of failures (see ticket + 20536), we must now allow the time to grow longer on each failure. + Fixes bug 20534; bugfix on 0.2.9.1-alpha. + diff --git a/src/or/directory.c b/src/or/directory.c index 24490b7426..5fc15724cc 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3770,7 +3770,10 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, const smartlist_t *schedule = find_dl_schedule(dls, options); tor_assert(schedule != NULL && smartlist_len(schedule) >= 2); *min = *((int *)(smartlist_get(schedule, 0))); - *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); + if (dls->backoff == DL_SCHED_DETERMINISTIC) + *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); + else + *max = INT_MAX; } /** Advance one delay step. The algorithm is to use the previous delay to