From a4ea335a6906eb4f8f58b5cf458cf290d322d10f Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 6 Jun 2019 08:45:57 +1000 Subject: [PATCH 1/3] dirauth: Fix some comments in the router status processing code. Fixes comments in dirserv_router_get_status() and was_router_added_t. Preparation for 30780 and 16564. --- src/feature/dirauth/process_descs.c | 15 ++++++++++----- src/feature/nodelist/routerlist.c | 13 +++++++------ src/feature/nodelist/routerlist.h | 4 ++-- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index 656922233e..17936add5f 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -216,9 +216,14 @@ dirserv_load_fingerprint_file(void) #define DISABLE_DISABLING_ED25519 -/** Check whether router has a nickname/identity key combination that - * we recognize from the fingerprint list, or an IP we automatically act on - * according to our configuration. Return the appropriate router status. +/** Check whether router has: + * - a nickname/identity key combination that we recognize from the fingerprint + * list, + * - an IP we automatically act on according to our configuration, + * - an appropriate version, and + * - matching pinned keys. + * + * Return the appropriate router status. * * If the status is 'FP_REJECT' and msg is provided, set * *msg to an explanation of why. */ @@ -236,7 +241,7 @@ dirserv_router_get_status(const routerinfo_t *router, const char **msg, return FP_REJECT; } - /* Check for the more usual versions to reject a router first. */ + /* Check for the more common reasons to reject a router first. */ const uint32_t r = dirserv_get_status_impl(d, router->nickname, router->addr, router->or_port, router->platform, msg, severity); @@ -535,7 +540,7 @@ dirserv_add_multiple_descriptors(const char *desc, size_t desclen, int general = purpose == ROUTER_PURPOSE_GENERAL; tor_assert(msg); - r=ROUTER_ADDED_SUCCESSFULLY; /*Least severe return value. */ + r=ROUTER_ADDED_SUCCESSFULLY; /* Least severe return value. */ if (!string_is_utf8_no_bom(desc, desclen)) { *msg = "descriptor(s) or extrainfo(s) not valid UTF-8 or had BOM."; diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index 5788347a0e..5948445c96 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -1459,12 +1459,13 @@ router_descriptor_is_older_than,(const routerinfo_t *router, int seconds)) } /** Add router to the routerlist, if we don't already have it. Replace - * older entries (if any) with the same key. Note: Callers should not hold - * their pointers to router if this function fails; router - * will either be inserted into the routerlist or freed. Similarly, even - * if this call succeeds, they should not hold their pointers to - * router after subsequent calls with other routerinfo's -- they - * might cause the original routerinfo to get freed. + * older entries (if any) with the same key. + * + * Note: Callers should not hold their pointers to router if this + * function fails; router will either be inserted into the routerlist or + * freed. Similarly, even if this call succeeds, they should not hold their + * pointers to router after subsequent calls with other routerinfo's -- + * they might cause the original routerinfo to get freed. * * Returns the status for the operation. Might set *msg if it wants * the poster of the router to know something. diff --git a/src/feature/nodelist/routerlist.h b/src/feature/nodelist/routerlist.h index 5771ebb1ab..d7f44cb807 100644 --- a/src/feature/nodelist/routerlist.h +++ b/src/feature/nodelist/routerlist.h @@ -37,8 +37,8 @@ typedef enum was_router_added_t { ROUTER_WAS_NOT_WANTED = -6, /* Router descriptor was rejected because it was older than * OLD_ROUTER_DESC_MAX_AGE. */ - ROUTER_WAS_TOO_OLD = -7, /* note contrast with 'NOT_NEW' */ - /* DOCDOC */ + ROUTER_WAS_TOO_OLD = -7, /* note contrast with 'ROUTER_IS_ALREADY_KNOWN' */ + /* Some certs on this router are expired. */ ROUTER_CERTS_EXPIRED = -8 } was_router_added_t; From 19bf5806adb80e513bb2707a1686216225fef420 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 6 Jun 2019 08:52:13 +1000 Subject: [PATCH 2/3] dirauth: Return a distinct status when formatting annotations fails Adds ROUTER_AUTHDIR_BUG_ANNOTATIONS to was_router_added_t. The out-of-order numbering is deliberate: it will be fixed by later commits for 16564. Fixes bug 30780; bugfix on 0.2.0.8-alpha. --- changes/bug30780 | 3 +++ src/feature/dirauth/process_descs.c | 4 +--- src/feature/nodelist/routerlist.h | 5 ++++- 3 files changed, 8 insertions(+), 4 deletions(-) create mode 100644 changes/bug30780 diff --git a/changes/bug30780 b/changes/bug30780 new file mode 100644 index 0000000000..5731d201a2 --- /dev/null +++ b/changes/bug30780 @@ -0,0 +1,3 @@ + o Minor bugfixes (directory authorities): + - Return a distinct status when formatting annotations fails. + Fixes bug 30780; bugfix on 0.2.0.8-alpha. diff --git a/src/feature/dirauth/process_descs.c b/src/feature/dirauth/process_descs.c index 17936add5f..a68d155651 100644 --- a/src/feature/dirauth/process_descs.c +++ b/src/feature/dirauth/process_descs.c @@ -556,9 +556,7 @@ dirserv_add_multiple_descriptors(const char *desc, size_t desclen, !general ? router_purpose_to_string(purpose) : "", !general ? "\n" : "")<0) { *msg = "Couldn't format annotations"; - /* XXX Not cool: we return -1 below, but (was_router_added_t)-1 is - * ROUTER_BAD_EI, which isn't what's gone wrong here. :( */ - return -1; + return ROUTER_AUTHDIR_BUG_ANNOTATIONS; } s = desc; diff --git a/src/feature/nodelist/routerlist.h b/src/feature/nodelist/routerlist.h index d7f44cb807..dc9203e015 100644 --- a/src/feature/nodelist/routerlist.h +++ b/src/feature/nodelist/routerlist.h @@ -39,7 +39,10 @@ typedef enum was_router_added_t { * OLD_ROUTER_DESC_MAX_AGE. */ ROUTER_WAS_TOO_OLD = -7, /* note contrast with 'ROUTER_IS_ALREADY_KNOWN' */ /* Some certs on this router are expired. */ - ROUTER_CERTS_EXPIRED = -8 + ROUTER_CERTS_EXPIRED = -8, + /* We couldn't format the annotations for this router. This is a directory + * authority bug. */ + ROUTER_AUTHDIR_BUG_ANNOTATIONS = -10 } was_router_added_t; /** How long do we avoid using a directory server after it's given us a 503? */ From 6be9d3aed88bf73cf10f06edb99f876fd8eeb1d9 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 6 Jun 2019 18:24:17 +1000 Subject: [PATCH 3/3] practracker: accept one extra line in routerlist.c practracker exception for 30780. --- scripts/maint/practracker/exceptions.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index eb0625b8d3..4f723c7c1e 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -216,7 +216,7 @@ problem function-size /src/feature/nodelist/node_select.c:router_pick_directory_ problem function-size /src/feature/nodelist/node_select.c:compute_weighted_bandwidths() 206 problem function-size /src/feature/nodelist/node_select.c:router_pick_trusteddirserver_impl() 114 problem function-size /src/feature/nodelist/nodelist.c:compute_frac_paths_available() 193 -problem file-size /src/feature/nodelist/routerlist.c 3238 +problem file-size /src/feature/nodelist/routerlist.c 3239 problem function-size /src/feature/nodelist/routerlist.c:router_rebuild_store() 148 problem function-size /src/feature/nodelist/routerlist.c:router_add_to_routerlist() 169 problem function-size /src/feature/nodelist/routerlist.c:routerlist_remove_old_routers() 121