From 743a5ef2b3781b593f5299758d0a7fc78b5816c4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 14 Jan 2021 09:42:56 -0500 Subject: relay: Don't flag that we published if descriptor build fails In case building the descriptor would fail, we could still flag that we did in fact publish the descriptors leading to no more attempt at publishing it which in turn makes the relay silent for some hours and not try to rebuild the descriptor later. This has been spotted with #40231 because the operator used a localhost address for the ORPort and "AssumeReachable 1" leading to this code path where the descriptor failed to build but all conditions to "can I publish" were met. Related to #40231 Signed-off-by: David Goulet --- src/feature/relay/relay_periodic.c | 2 +- src/feature/relay/router.c | 18 +++++++++--------- src/feature/relay/router.h | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/feature/relay/relay_periodic.c b/src/feature/relay/relay_periodic.c index a857ea8d92..a917d90f1a 100644 --- a/src/feature/relay/relay_periodic.c +++ b/src/feature/relay/relay_periodic.c @@ -104,7 +104,7 @@ rotate_onion_key_callback(time_t now, const or_options_t *options) log_info(LD_GENERAL,"Rotating onion key."); rotate_onion_key(); cpuworkers_rotate_keyinfo(); - if (router_rebuild_descriptor(1)<0) { + if (!router_rebuild_descriptor(1)) { log_info(LD_CONFIG, "Couldn't rebuild router descriptor"); } if (advertised_server_mode() && !net_is_disabled()) diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index e018561556..eb1d5a63f1 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -1427,10 +1427,9 @@ consider_publishable_server(int force) return; rebuilt = router_rebuild_descriptor(0); - if (decide_if_publishable_server()) { + if (rebuilt && decide_if_publishable_server()) { set_server_advertised(1); - if (rebuilt == 0) - router_upload_dir_desc_to_dirservers(force); + router_upload_dir_desc_to_dirservers(force); } else { set_server_advertised(0); } @@ -1817,7 +1816,7 @@ router_get_my_extrainfo(void) { if (!server_mode(get_options())) return NULL; - if (router_rebuild_descriptor(0)) + if (!router_rebuild_descriptor(0)) return NULL; return desc_extrainfo; } @@ -2414,9 +2413,10 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e) /** If force is true, or our descriptor is out-of-date, rebuild a fresh * routerinfo, signed server descriptor, and extra-info document for this OR. - * Return 0 on success, -1 on temporary error. + * + * Return true on success, else false on temporary error. */ -int +bool router_rebuild_descriptor(int force) { int err = 0; @@ -2424,13 +2424,13 @@ router_rebuild_descriptor(int force) extrainfo_t *ei; if (desc_clean_since && !force) - return 0; + return true; log_info(LD_OR, "Rebuilding relay descriptor%s", force ? " (forced)" : ""); err = router_build_fresh_descriptor(&ri, &ei); if (err < 0) { - return err; + return false; } routerinfo_free(desc_routerinfo); @@ -2446,7 +2446,7 @@ router_rebuild_descriptor(int force) } desc_dirty_reason = NULL; control_event_my_descriptor_changed(); - return 0; + return true; } /** Called when we have a new set of consensus parameters. */ diff --git a/src/feature/relay/router.h b/src/feature/relay/router.h index 2648bb5112..aa03c27142 100644 --- a/src/feature/relay/router.h +++ b/src/feature/relay/router.h @@ -102,7 +102,7 @@ int router_extrainfo_digest_is_me(const char *digest); int router_is_me(const routerinfo_t *router); bool router_addr_is_my_published_addr(const tor_addr_t *addr); int router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e); -int router_rebuild_descriptor(int force); +bool router_rebuild_descriptor(int force); char *router_dump_router_to_string(routerinfo_t *router, const crypto_pk_t *ident_key, const crypto_pk_t *tap_key, -- cgit v1.2.3-54-g00ecf From f0c29f0883045283d86b584f1634770358d84f41 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 14 Jan 2021 09:56:11 -0500 Subject: relay: Don't BUG() if we can't find authority descriptor We can end up trying to find our address from an authority while we don't have yet its descriptor. In this case, don't BUG() and just come back later. Closes #40231 Signed-off-by: David Goulet --- changes/ticket40231 | 4 ++++ src/feature/relay/relay_find_addr.c | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 changes/ticket40231 diff --git a/changes/ticket40231 b/changes/ticket40231 new file mode 100644 index 0000000000..a5ba598fd1 --- /dev/null +++ b/changes/ticket40231 @@ -0,0 +1,4 @@ + o Minor bugfixes (relay): + - If we were unable to build our descriptor, don't mark that we've + advertised our descriptor. Also remove an harmless BUG(). Fixes bug 40231; + bugfix on 0.4.5.1-alpha. diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 9c2c8b281c..2da2328b14 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -198,9 +198,13 @@ relay_addr_learn_from_dirauth(void) return; } const node_t *node = node_get_by_id(rs->identity_digest); - if (BUG(!node)) { - /* If there is a routerstatus_t, there is a node_t thus this should - * never fail. */ + if (!node) { + /* This can happen if we are still in the early starting stage where no + * descriptors we actually fetched and thus we have the routerstatus_t + * for the authority but not its descriptor which is needed to build a + * circuit and thus learn our address. */ + log_info(LD_GENERAL, "Can't build a circuit to an authority. Unable to " + "learn for now our address from them."); return; } extend_info_t *ei = extend_info_from_node(node, 1); -- cgit v1.2.3-54-g00ecf