From 8276a0ab85bdea97180459277d9d5b4531e216c4 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:18:25 +0300 Subject: [PATCH 01/10] Update entry guard state whenever we download a consensus. Update guard state even if we don't have enough dirinfo since that actually affects the future download of dirinfos. Fixes #23862 on 0.3.0.1-alpha --- src/or/main.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 7b1f4975f..0d91803d4 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -968,6 +968,15 @@ directory_info_has_arrived(time_t now, int from_cache, int suppress_logs) { const or_options_t *options = get_options(); + /* if we have enough dir info, then update our guard status with + * whatever we just learned. */ + int invalidate_circs = guards_update_all(); + + if (invalidate_circs) { + circuit_mark_all_unused_circs(); + circuit_mark_all_dirty_circs_as_unusable(); + } + if (!router_have_minimum_dir_info()) { int quiet = suppress_logs || from_cache || directory_too_idle_to_fetch_descriptors(options, now); @@ -981,15 +990,6 @@ directory_info_has_arrived(time_t now, int from_cache, int suppress_logs) update_all_descriptor_downloads(now); } - /* if we have enough dir info, then update our guard status with - * whatever we just learned. */ - int invalidate_circs = guards_update_all(); - - if (invalidate_circs) { - circuit_mark_all_unused_circs(); - circuit_mark_all_dirty_circs_as_unusable(); - } - /* Don't even bother trying to get extrainfo until the rest of our * directory info is up-to-date */ if (options->DownloadExtraInfo) From 6bd64e8212a29d21b20d68a3f69489ffdcbcb42b Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:18:38 +0300 Subject: [PATCH 02/10] Remove a duplicate call to update_microdesc_downloads() This call happens before we update our entry guards, so it needs to be removed for the fix to #23862 to work. --- src/or/directory.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 9494cf02b..f54bf176c 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2592,11 +2592,11 @@ handle_response_fetch_consensus(dir_connection_t *conn, /* If we launched other fetches for this consensus, cancel them. */ connection_dir_close_consensus_fetches(conn, flavname); - /* launches router downloads as needed */ + /* update the list of routers and directory guards */ routers_update_all_from_networkstatus(now, 3); update_microdescs_from_networkstatus(now); - update_microdesc_downloads(now); directory_info_has_arrived(now, 0, 0); + if (authdir_mode_v3(get_options())) { sr_act_post_consensus( networkstatus_get_latest_consensus_by_flavor(FLAV_NS)); From 43c34dfca04f4e10c85ed567f383ee5442be1a80 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:54:48 +0300 Subject: [PATCH 03/10] Add changes file for #23862. --- changes/bug23682 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug23682 diff --git a/changes/bug23682 b/changes/bug23682 new file mode 100644 index 000000000..301ce7367 --- /dev/null +++ b/changes/bug23682 @@ -0,0 +1,5 @@ + o Minor bugfixes (entry guards): + - Tor now updates its guard state when it reads a consensus regardless of + whether it's missing descriptors. That makes tor use its primary guards + to fetch descriptors in some edge cases where it would have used fallback + directories in the past. Fixes bug 23862; bugfix on 0.3.0.1-alpha. \ No newline at end of file From 210f0c24f039aa6d46b9396ec6bb13ff575a9e79 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:18:25 +0300 Subject: [PATCH 04/10] Update entry guard state whenever we download a consensus. Update guard state even if we don't have enough dirinfo since that actually affects the future download of dirinfos. Fixes #23862 on 0.3.0.1-alpha --- src/or/main.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 478316b79..1d899349e 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -965,6 +965,15 @@ directory_info_has_arrived(time_t now, int from_cache, int suppress_logs) { const or_options_t *options = get_options(); + /* if we have enough dir info, then update our guard status with + * whatever we just learned. */ + int invalidate_circs = guards_update_all(); + + if (invalidate_circs) { + circuit_mark_all_unused_circs(); + circuit_mark_all_dirty_circs_as_unusable(); + } + if (!router_have_minimum_dir_info()) { int quiet = suppress_logs || from_cache || directory_too_idle_to_fetch_descriptors(options, now); @@ -978,15 +987,6 @@ directory_info_has_arrived(time_t now, int from_cache, int suppress_logs) update_all_descriptor_downloads(now); } - /* if we have enough dir info, then update our guard status with - * whatever we just learned. */ - int invalidate_circs = guards_update_all(); - - if (invalidate_circs) { - circuit_mark_all_unused_circs(); - circuit_mark_all_dirty_circs_as_unusable(); - } - /* Don't even bother trying to get extrainfo until the rest of our * directory info is up-to-date */ if (options->DownloadExtraInfo) From 1c9f06348671ee72112f2f75d90d7e46cf412b2d Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:18:38 +0300 Subject: [PATCH 05/10] Remove a duplicate call to update_microdesc_downloads() This call happens before we update our entry guards, so it needs to be removed for the fix to #23862 to work. --- src/or/directory.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 534e3d610..48d912bd2 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2122,10 +2122,9 @@ connection_dir_client_reached_eof(dir_connection_t *conn) /* If we launched other fetches for this consensus, cancel them. */ connection_dir_close_consensus_fetches(conn, flavname); - /* launches router downloads as needed */ + /* update the list of routers and directory guards */ routers_update_all_from_networkstatus(now, 3); update_microdescs_from_networkstatus(now); - update_microdesc_downloads(now); directory_info_has_arrived(now, 0, 0); if (authdir_mode_v3(get_options())) { sr_act_post_consensus( From 7ae9e92ffbe5f95d10d8822bfc33eb6a1094be1d Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 25 Oct 2017 19:54:48 +0300 Subject: [PATCH 06/10] Add changes file for #23862. --- changes/bug23682 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/bug23682 diff --git a/changes/bug23682 b/changes/bug23682 new file mode 100644 index 000000000..301ce7367 --- /dev/null +++ b/changes/bug23682 @@ -0,0 +1,5 @@ + o Minor bugfixes (entry guards): + - Tor now updates its guard state when it reads a consensus regardless of + whether it's missing descriptors. That makes tor use its primary guards + to fetch descriptors in some edge cases where it would have used fallback + directories in the past. Fixes bug 23862; bugfix on 0.3.0.1-alpha. \ No newline at end of file From 472473ec5d1a49b852b2f20fd7b0ed3350877cd2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 26 Nov 2017 17:05:30 -0500 Subject: [PATCH 07/10] transport_new() cannot fail; do not check for it to fail. (It can't fail because the tor_malloc*() family of functions can never return NULL) Found with STACK. --- src/or/transports.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/or/transports.c b/src/or/transports.c index 68d135484..5fb24e11a 100644 --- a/src/or/transports.c +++ b/src/or/transports.c @@ -1094,8 +1094,6 @@ parse_smethod_line(const char *line, managed_proxy_t *mp) transport = transport_new(&tor_addr, port, method_name, PROXY_NONE, args_string); - if (!transport) - goto err; smartlist_add(mp->transports, transport); @@ -1186,8 +1184,6 @@ parse_cmethod_line(const char *line, managed_proxy_t *mp) } transport = transport_new(&tor_addr, port, method_name, socks_ver, NULL); - if (!transport) - goto err; smartlist_add(mp->transports, transport); From 3da15bcbe8992b0dc3350902248ee791d1054642 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 26 Nov 2017 17:16:25 -0500 Subject: [PATCH 08/10] Stop checking for sandbox:new_element() failures: it can't fail. (It can't fail because the tor_malloc*() family of functions can never return NULL) Found with STACK --- src/common/sandbox.c | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/src/common/sandbox.c b/src/common/sandbox.c index 0fd129d22..ba6c3efb9 100644 --- a/src/common/sandbox.c +++ b/src/common/sandbox.c @@ -1393,10 +1393,6 @@ sandbox_cfg_allow_stat_filename(sandbox_cfg_t **cfg, char *file) sandbox_cfg_t *elem = NULL; elem = new_element(SCMP_stat, file); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } elem->next = *cfg; *cfg = elem; @@ -1410,10 +1406,6 @@ sandbox_cfg_allow_open_filename(sandbox_cfg_t **cfg, char *file) sandbox_cfg_t *elem = NULL; elem = new_element(SCMP_SYS(open), file); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } elem->next = *cfg; *cfg = elem; @@ -1427,10 +1419,6 @@ sandbox_cfg_allow_chmod_filename(sandbox_cfg_t **cfg, char *file) sandbox_cfg_t *elem = NULL; elem = new_element(SCMP_SYS(chmod), file); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } elem->next = *cfg; *cfg = elem; @@ -1444,10 +1432,6 @@ sandbox_cfg_allow_chown_filename(sandbox_cfg_t **cfg, char *file) sandbox_cfg_t *elem = NULL; elem = new_element(SCMP_SYS(chown), file); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } elem->next = *cfg; *cfg = elem; @@ -1462,11 +1446,6 @@ sandbox_cfg_allow_rename(sandbox_cfg_t **cfg, char *file1, char *file2) elem = new_element2(SCMP_SYS(rename), file1, file2); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } - elem->next = *cfg; *cfg = elem; @@ -1479,10 +1458,6 @@ sandbox_cfg_allow_openat_filename(sandbox_cfg_t **cfg, char *file) sandbox_cfg_t *elem = NULL; elem = new_element(SCMP_SYS(openat), file); - if (!elem) { - log_err(LD_BUG,"(Sandbox) failed to register parameter!"); - return -1; - } elem->next = *cfg; *cfg = elem; From f539d89fd9d19e44596976a42c12d4fd41f1d0fc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 26 Nov 2017 17:34:49 -0500 Subject: [PATCH 09/10] Move subtraction in rephist to try to avoid STACK warning (I do not know why this one is happening) --- src/or/rephist.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/or/rephist.c b/src/or/rephist.c index 51e800bc3..b63cdbfcd 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -1811,7 +1811,7 @@ static time_t last_prediction_add_time=0; int predicted_ports_prediction_time_remaining(time_t now) { - time_t idle_delta = now - last_prediction_add_time; + time_t idle_delta; /* Protect against overflow of return value. This can happen if the clock * jumps backwards in time. Update the last prediction time (aka last @@ -1821,6 +1821,8 @@ predicted_ports_prediction_time_remaining(time_t now) if (last_prediction_add_time > now) { last_prediction_add_time = now; idle_delta = 0; + } else { + idle_delta = now - last_prediction_add_time; } /* Protect against underflow of the return value. This can happen for very From 35d56a127d9af913f0f74ec359b182fa33f49c50 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 26 Nov 2017 17:37:36 -0500 Subject: [PATCH 10/10] Add a changelog for the STACK fixes --- changes/stack | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/stack diff --git a/changes/stack b/changes/stack new file mode 100644 index 000000000..ffdf536cb --- /dev/null +++ b/changes/stack @@ -0,0 +1,7 @@ + o Minor bugfixes (correctness): + - Fix several places in our codebase where a C compiler would be likely + to eliminate a check, based on assuming that undefined behavior had not + happened elsewhere in the code. These cases are usually a sign of + redundant checking, or dubious arithmetic. Found by Georg Koppen using + the "STACK" tool from Wang, Zeldovich, Kaashoek, and + Solar-Lezama. Fixes bug 24423; bugfix on various Tor versions.