From cc896f7c84c221d7dcfff1e0155533dce3ac5518 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 11 Feb 2013 17:09:10 -0500 Subject: [PATCH 1/5] Teach resolve_my_address() to return a cached answer I didn't make any of the callers use this feature yet. --- src/or/config.c | 20 ++++++++++++++++++-- src/or/config.h | 3 ++- src/or/dirserv.c | 4 ++-- src/or/router.c | 6 +++--- src/or/routerlist.c | 3 ++- 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 7ca20e46b4..e37b148b7d 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1908,12 +1908,16 @@ static uint32_t last_resolved_addr = 0; * holding that hostname. (If we didn't get our address by resolving a * hostname, set *hostname_out to NULL.) * + * If use_cached_addr is true, and we have a plausible answer, + * provide that answer and return. + * * XXXX ipv6 */ int resolve_my_address(int warn_severity, const or_options_t *options, uint32_t *addr_out, - const char **method_out, char **hostname_out) + const char **method_out, char **hostname_out, + int use_cached_addr) { struct in_addr in; uint32_t addr; /* host order */ @@ -1930,6 +1934,18 @@ resolve_my_address(int warn_severity, const or_options_t *options, tor_assert(addr_out); + /* + * Step zero: if used_cached_addr is true, and we have a cached answer, + * just return it and be done. + */ + + if (use_cached_addr && last_resolved_addr) { + *addr_out = last_resolved_addr; + if (method_out) + *method_out = "CACHED"; + return 0; + } + /* * Step one: Fill in 'hostname' to be our best guess. */ @@ -2343,7 +2359,7 @@ options_validate(or_options_t *old_options, or_options_t *options, if (authdir_mode(options)) { /* confirm that our address isn't broken, so we can complain now */ uint32_t tmp; - if (resolve_my_address(LOG_WARN, options, &tmp, NULL, NULL) < 0) + if (resolve_my_address(LOG_WARN, options, &tmp, NULL, NULL, 0) < 0) REJECT("Failed to resolve/guess local address. See logs for details."); } diff --git a/src/or/config.h b/src/or/config.h index e0748a07bf..7ec52e39db 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -28,7 +28,8 @@ setopt_err_t options_trial_assign(config_line_t *list, int use_defaults, int resolve_my_address(int warn_severity, const or_options_t *options, uint32_t *addr_out, - const char **method_out, char **hostname_out); + const char **method_out, char **hostname_out, + int use_cached_addr); int is_local_addr(const tor_addr_t *addr); void options_init(or_options_t *options); char *options_dump(const or_options_t *options, int minimal); diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 0819d4bd24..280c6b429a 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -2750,7 +2750,7 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key, tor_assert(private_key); tor_assert(cert); - if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname)<0) { + if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname, 0)<0) { log_warn(LD_NET, "Couldn't resolve my hostname"); return NULL; } @@ -2960,7 +2960,7 @@ generate_v2_networkstatus_opinion(void) private_key = get_server_identity_key(); - if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname)<0) { + if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname, 0)<0) { log_warn(LD_NET, "Couldn't resolve my hostname"); goto done; } diff --git a/src/or/router.c b/src/or/router.c index 4492ed271f..bc1c6d215f 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1712,7 +1712,7 @@ static int router_guess_address_from_dir_headers(uint32_t *guess); int router_pick_published_address(const or_options_t *options, uint32_t *addr) { - if (resolve_my_address(LOG_INFO, options, addr, NULL, NULL) < 0) { + if (resolve_my_address(LOG_INFO, options, addr, NULL, NULL, 0) < 0) { log_info(LD_CONFIG, "Could not determine our address locally. " "Checking if directory headers provide any hints."); if (router_guess_address_from_dir_headers(addr) < 0) { @@ -2103,7 +2103,7 @@ check_descriptor_ipaddress_changed(time_t now) /* XXXX ipv6 */ prev = desc_routerinfo->addr; - if (resolve_my_address(LOG_INFO, options, &cur, &method, &hostname) < 0) { + if (resolve_my_address(LOG_INFO, options, &cur, &method, &hostname, 0) < 0) { log_info(LD_CONFIG,"options->Address didn't resolve into an IP."); return; } @@ -2159,7 +2159,7 @@ router_new_address_suggestion(const char *suggestion, } /* XXXX ipv6 */ - if (resolve_my_address(LOG_INFO, options, &cur, NULL, NULL) >= 0) { + if (resolve_my_address(LOG_INFO, options, &cur, NULL, NULL, 0) >= 0) { /* We're all set -- we already know our address. Great. */ tor_addr_from_ipv4h(&last_guessed_ip, cur); /* store it in case we need it later */ diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 90b707bcdb..2dde89b7e1 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -3951,7 +3951,8 @@ trusted_dir_server_new(const char *nickname, const char *address, dir_server_t *result; if (!address) { /* The address is us; we should guess. */ - if (resolve_my_address(LOG_WARN, get_options(), &a, NULL, &hostname) < 0) { + if (resolve_my_address(LOG_WARN, get_options(), + &a, NULL, &hostname, 0) < 0) { log_warn(LD_CONFIG, "Couldn't find a suitable address when adding ourself as a " "trusted directory server."); From 17089302fdb454e03f173cc0c70c5a69c5c1cd7b Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 11 Feb 2013 21:48:18 -0500 Subject: [PATCH 2/5] Stop trying to resolve our hostname so often For example, we were doing a resolve every time we think about doing a directory fetch. Now we reuse the cached answer in some cases. Fixes bugs 1992 (bugfix on 0.2.0.20-rc) and 2410 (bugfix on 0.1.2.2-alpha). --- changes/bug1992 | 6 ++++++ src/or/router.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changes/bug1992 diff --git a/changes/bug1992 b/changes/bug1992 new file mode 100644 index 0000000000..6fa4eaea09 --- /dev/null +++ b/changes/bug1992 @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Stop trying to resolve our hostname so often (e.g. every time we + think about doing a directory fetch). Now we reuse the cached + answer in some cases. Fixes bugs 1992 (bugfix on 0.2.0.20-rc) + and 2410 (bugfix on 0.1.2.2-alpha). + diff --git a/src/or/router.c b/src/or/router.c index bc1c6d215f..a668c24bed 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1712,7 +1712,7 @@ static int router_guess_address_from_dir_headers(uint32_t *guess); int router_pick_published_address(const or_options_t *options, uint32_t *addr) { - if (resolve_my_address(LOG_INFO, options, addr, NULL, NULL, 0) < 0) { + if (resolve_my_address(LOG_INFO, options, addr, NULL, NULL, 1) < 0) { log_info(LD_CONFIG, "Could not determine our address locally. " "Checking if directory headers provide any hints."); if (router_guess_address_from_dir_headers(addr) < 0) { @@ -2159,7 +2159,7 @@ router_new_address_suggestion(const char *suggestion, } /* XXXX ipv6 */ - if (resolve_my_address(LOG_INFO, options, &cur, NULL, NULL, 0) >= 0) { + if (resolve_my_address(LOG_INFO, options, &cur, NULL, NULL, 1) >= 0) { /* We're all set -- we already know our address. Great. */ tor_addr_from_ipv4h(&last_guessed_ip, cur); /* store it in case we need it later */ From 5911fc0c17176bbff379921b7905ec990f505f85 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 11 Feb 2013 21:57:32 -0500 Subject: [PATCH 3/5] Check for IP address change every minute, not 15 minutes Relays used to check every 10 to 60 seconds, as an accidental side effect of calling directory_fetches_from_authorities() when considering doing a directory fetch. The fix for bug 1992 removes that side effect. At the same time, bridge relays never had the side effect, leading to confused bridge operators who tried crazy tricks to get their bridges to notice IP address changes (see ticket 1913). The new behavior is to reinstate an every-60-seconds check for both public relays and bridge relays, now that the side effect is gone. --- changes/bug1992 | 5 +++++ src/or/main.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/changes/bug1992 b/changes/bug1992 index 6fa4eaea09..6a751dc7e6 100644 --- a/changes/bug1992 +++ b/changes/bug1992 @@ -4,3 +4,8 @@ answer in some cases. Fixes bugs 1992 (bugfix on 0.2.0.20-rc) and 2410 (bugfix on 0.1.2.2-alpha). + o Minor features: + - Make bridge relays check once a minute for whether their IP + address has changed, rather than only every 15 minutes. Resolves + bugs 1913 and 1992. + diff --git a/src/or/main.c b/src/or/main.c index aa601e5a4f..98d3359cf8 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1407,7 +1407,7 @@ run_scheduled_events(time_t now) * that would require an upload? */ #define CHECK_DESCRIPTOR_INTERVAL (60) /** How often do we (as a router) check whether our IP address has changed? */ -#define CHECK_IPADDRESS_INTERVAL (15*60) +#define CHECK_IPADDRESS_INTERVAL (60) /* 2b. Once per minute, regenerate and upload the descriptor if the old * one is inaccurate. */ From b166e9edb96288e8f94776b738c1dc0874a9cffa Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 11 Feb 2013 22:07:19 -0500 Subject: [PATCH 4/5] simplify timing checks now that both timers are on the same schedule, there's no point tracking separate timers. --- src/or/main.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index 98d3359cf8..b5d1e2da34 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1157,7 +1157,6 @@ run_scheduled_events(time_t now) static time_t time_to_check_v3_certificate = 0; static time_t time_to_check_listeners = 0; static time_t time_to_check_descriptor = 0; - static time_t time_to_check_ipaddress = 0; static time_t time_to_shrink_memory = 0; static time_t time_to_try_getting_descriptors = 0; static time_t time_to_reset_descriptor_failures = 0; @@ -1403,11 +1402,10 @@ run_scheduled_events(time_t now) /** 2. Periodically, we consider force-uploading our descriptor * (if we've passed our internal checks). */ -/** How often do we check whether part of our router info has changed in a way - * that would require an upload? */ +/** How often do we check whether part of our router info has changed in a + * way that would require an upload? That includes checking whether our IP + * address has changed. */ #define CHECK_DESCRIPTOR_INTERVAL (60) -/** How often do we (as a router) check whether our IP address has changed? */ -#define CHECK_IPADDRESS_INTERVAL (60) /* 2b. Once per minute, regenerate and upload the descriptor if the old * one is inaccurate. */ @@ -1415,10 +1413,7 @@ run_scheduled_events(time_t now) static int dirport_reachability_count = 0; time_to_check_descriptor = now + CHECK_DESCRIPTOR_INTERVAL; check_descriptor_bandwidth_changed(now); - if (time_to_check_ipaddress < now) { - time_to_check_ipaddress = now + CHECK_IPADDRESS_INTERVAL; - check_descriptor_ipaddress_changed(now); - } + check_descriptor_ipaddress_changed(now); mark_my_descriptor_dirty_if_too_old(now); consider_publishable_server(0); /* also, check religiously for reachability, if it's within the first From 178599f026d9c7575a2b790c7cb5ccfba7ba1635 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Tue, 12 Feb 2013 04:25:42 -0500 Subject: [PATCH 5/5] get rid of the new caching notion in resolve_my_address() and replace it with the good old-fashioned two functions approach --- src/or/config.c | 27 +++++++++------------------ src/or/config.h | 4 ++-- src/or/dirserv.c | 4 ++-- src/or/router.c | 10 +++++++--- src/or/routerlist.c | 2 +- 5 files changed, 21 insertions(+), 26 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index e37b148b7d..a11c44bdfa 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1893,6 +1893,13 @@ list_torrc_options(void) /** Last value actually set by resolve_my_address. */ static uint32_t last_resolved_addr = 0; + +/** Accessor for last_resolved_addr from outside this file. */ +uint32_t get_last_resolved_addr(void) +{ + return last_resolved_addr; +} + /** * Use options-\>Address to guess our public IP address. * @@ -1908,16 +1915,12 @@ static uint32_t last_resolved_addr = 0; * holding that hostname. (If we didn't get our address by resolving a * hostname, set *hostname_out to NULL.) * - * If use_cached_addr is true, and we have a plausible answer, - * provide that answer and return. - * * XXXX ipv6 */ int resolve_my_address(int warn_severity, const or_options_t *options, uint32_t *addr_out, - const char **method_out, char **hostname_out, - int use_cached_addr) + const char **method_out, char **hostname_out) { struct in_addr in; uint32_t addr; /* host order */ @@ -1934,18 +1937,6 @@ resolve_my_address(int warn_severity, const or_options_t *options, tor_assert(addr_out); - /* - * Step zero: if used_cached_addr is true, and we have a cached answer, - * just return it and be done. - */ - - if (use_cached_addr && last_resolved_addr) { - *addr_out = last_resolved_addr; - if (method_out) - *method_out = "CACHED"; - return 0; - } - /* * Step one: Fill in 'hostname' to be our best guess. */ @@ -2359,7 +2350,7 @@ options_validate(or_options_t *old_options, or_options_t *options, if (authdir_mode(options)) { /* confirm that our address isn't broken, so we can complain now */ uint32_t tmp; - if (resolve_my_address(LOG_WARN, options, &tmp, NULL, NULL, 0) < 0) + if (resolve_my_address(LOG_WARN, options, &tmp, NULL, NULL) < 0) REJECT("Failed to resolve/guess local address. See logs for details."); } diff --git a/src/or/config.h b/src/or/config.h index 7ec52e39db..ef4acac514 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -26,10 +26,10 @@ const char *get_short_version(void); setopt_err_t options_trial_assign(config_line_t *list, int use_defaults, int clear_first, char **msg); +uint32_t get_last_resolved_addr(void); int resolve_my_address(int warn_severity, const or_options_t *options, uint32_t *addr_out, - const char **method_out, char **hostname_out, - int use_cached_addr); + const char **method_out, char **hostname_out); int is_local_addr(const tor_addr_t *addr); void options_init(or_options_t *options); char *options_dump(const or_options_t *options, int minimal); diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 280c6b429a..0819d4bd24 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -2750,7 +2750,7 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key, tor_assert(private_key); tor_assert(cert); - if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname, 0)<0) { + if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname)<0) { log_warn(LD_NET, "Couldn't resolve my hostname"); return NULL; } @@ -2960,7 +2960,7 @@ generate_v2_networkstatus_opinion(void) private_key = get_server_identity_key(); - if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname, 0)<0) { + if (resolve_my_address(LOG_WARN, options, &addr, NULL, &hostname)<0) { log_warn(LD_NET, "Couldn't resolve my hostname"); goto done; } diff --git a/src/or/router.c b/src/or/router.c index a668c24bed..7b1fdc0dfe 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1712,7 +1712,9 @@ static int router_guess_address_from_dir_headers(uint32_t *guess); int router_pick_published_address(const or_options_t *options, uint32_t *addr) { - if (resolve_my_address(LOG_INFO, options, addr, NULL, NULL, 1) < 0) { + *addr = get_last_resolved_addr(); + if (!*addr && + resolve_my_address(LOG_INFO, options, addr, NULL, NULL) < 0) { log_info(LD_CONFIG, "Could not determine our address locally. " "Checking if directory headers provide any hints."); if (router_guess_address_from_dir_headers(addr) < 0) { @@ -2103,7 +2105,7 @@ check_descriptor_ipaddress_changed(time_t now) /* XXXX ipv6 */ prev = desc_routerinfo->addr; - if (resolve_my_address(LOG_INFO, options, &cur, &method, &hostname, 0) < 0) { + if (resolve_my_address(LOG_INFO, options, &cur, &method, &hostname) < 0) { log_info(LD_CONFIG,"options->Address didn't resolve into an IP."); return; } @@ -2159,7 +2161,9 @@ router_new_address_suggestion(const char *suggestion, } /* XXXX ipv6 */ - if (resolve_my_address(LOG_INFO, options, &cur, NULL, NULL, 1) >= 0) { + cur = get_last_resolved_addr(); + if (cur || + resolve_my_address(LOG_INFO, options, &cur, NULL, NULL) >= 0) { /* We're all set -- we already know our address. Great. */ tor_addr_from_ipv4h(&last_guessed_ip, cur); /* store it in case we need it later */ diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 2dde89b7e1..2f08167f18 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -3952,7 +3952,7 @@ trusted_dir_server_new(const char *nickname, const char *address, if (!address) { /* The address is us; we should guess. */ if (resolve_my_address(LOG_WARN, get_options(), - &a, NULL, &hostname, 0) < 0) { + &a, NULL, &hostname) < 0) { log_warn(LD_CONFIG, "Couldn't find a suitable address when adding ourself as a " "trusted directory server.");