From 20eb38a588313bba2fd35c32dc0e1c63ef4b8497 Mon Sep 17 00:00:00 2001 From: Linus Nordberg Date: Tue, 27 Mar 2012 15:00:34 +0200 Subject: [PATCH 1/4] Refactor dirserv_orconn_tls_done(). Look up the router using the digest instead of looping over all routers. --- src/or/dirserv.c | 55 +++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index a7bbe983c0..01a083369e 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -3264,8 +3264,8 @@ dirserv_get_routerdescs(smartlist_t *descs_out, const char *key, * router listening at address:or_port, and has yielded * a certificate with digest digest_rcvd. * - * Also, if as_advertised is 1, then inform the reachability checker - * that we could get to this guy. + * If as_advertised is 1, then inform the reachability checker that we + * could get to this guy. */ void dirserv_orconn_tls_done(const char *address, @@ -3273,37 +3273,40 @@ dirserv_orconn_tls_done(const char *address, const char *digest_rcvd, int as_advertised) { - routerlist_t *rl = router_get_routerlist(); + routerinfo_t *ri = NULL; time_t now = time(NULL); - int bridge_auth = authdir_mode_bridge(get_options()); tor_assert(address); tor_assert(digest_rcvd); - /* XXX023 Doing a loop like this is stupid. We should just look up the - * router by digest_rcvd, and see if address, orport, and as_advertised - * match up. -NM */ - SMARTLIST_FOREACH_BEGIN(rl->routers, routerinfo_t *, ri) { - if (!strcasecmp(address, ri->address) && or_port == ri->or_port && - as_advertised && - fast_memeq(ri->cache_info.identity_digest, digest_rcvd, DIGEST_LEN)) { - /* correct digest. mark this router reachable! */ - if (!bridge_auth || ri->purpose == ROUTER_PURPOSE_BRIDGE) { - tor_addr_t addr, *addrp=NULL; - log_info(LD_DIRSERV, "Found router %s to be reachable at %s:%d. Yay.", - router_describe(ri), - address, ri->or_port); - if (tor_addr_parse(&addr, ri->address) != -1) - addrp = &addr; - else - log_warn(LD_BUG, "Couldn't parse IP address \"%s\"", ri->address); - rep_hist_note_router_reachable(digest_rcvd, addrp, or_port, now); - ri->last_reachable = now; - } - } - } SMARTLIST_FOREACH_END(ri); /* FFFF Maybe we should reinstate the code that dumps routers with the same * addr/port but with nonmatching keys, but instead of dumping, we should * skip testing. */ + + if (!as_advertised) + return; + + ri = router_get_mutable_by_digest(digest_rcvd); + if (ri == NULL) + return; + + if (!strcasecmp(address, ri->address) && or_port == ri->or_port) { + /* Found the right router. */ + if (!authdir_mode_bridge(get_options()) || + ri->purpose == ROUTER_PURPOSE_BRIDGE) { + /* This is a bridge or we're not a bridge authorititative -- + mark it as reachable. */ + tor_addr_t addr, *addrp=NULL; + log_info(LD_DIRSERV, "Found router %s to be reachable at %s:%d. Yay.", + router_describe(ri), + address, ri->or_port); + if (tor_addr_parse(&addr, ri->address) != -1) + addrp = &addr; + else + log_warn(LD_BUG, "Couldn't parse IP address \"%s\"", ri->address); + rep_hist_note_router_reachable(digest_rcvd, addrp, or_port, now); + ri->last_reachable = now; + } + } } /** Called when we, as an authority, receive a new router descriptor either as From 5cb82e44d185fff4586ca2b92039e5686b9e753b Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 29 Mar 2012 16:37:50 -0400 Subject: [PATCH 2/4] simplify further --- src/or/connection_or.c | 7 ++----- src/or/dirserv.c | 9 ++------- src/or/dirserv.h | 3 +-- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 30d92b2728..0d3f9d87f2 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1453,7 +1453,6 @@ int connection_or_client_learned_peer_id(or_connection_t *conn, const uint8_t *peer_id) { - int as_expected = 1; const or_options_t *options = get_options(); int severity = server_mode(options) ? LOG_PROTOCOL_WARN : LOG_WARN; @@ -1492,14 +1491,12 @@ connection_or_client_learned_peer_id(or_connection_t *conn, control_event_bootstrap_problem( "Unexpected identity in router certificate", END_OR_CONN_REASON_OR_IDENTITY); - as_expected = 0; + return -1; } if (authdir_mode_tests_reachability(options)) { dirserv_orconn_tls_done(conn->_base.address, conn->_base.port, - (const char*)peer_id, as_expected); + (const char*)peer_id); } - if (!as_expected) - return -1; return 0; } diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 01a083369e..467129c728 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -3264,14 +3264,12 @@ dirserv_get_routerdescs(smartlist_t *descs_out, const char *key, * router listening at address:or_port, and has yielded * a certificate with digest digest_rcvd. * - * If as_advertised is 1, then inform the reachability checker that we - * could get to this guy. + * Inform the reachability checker that we could get to this guy. */ void dirserv_orconn_tls_done(const char *address, uint16_t or_port, - const char *digest_rcvd, - int as_advertised) + const char *digest_rcvd) { routerinfo_t *ri = NULL; time_t now = time(NULL); @@ -3282,9 +3280,6 @@ dirserv_orconn_tls_done(const char *address, * addr/port but with nonmatching keys, but instead of dumping, we should * skip testing. */ - if (!as_advertised) - return; - ri = router_get_mutable_by_digest(digest_rcvd); if (ri == NULL) return; diff --git a/src/or/dirserv.h b/src/or/dirserv.h index 6a86b944ea..fc48e489e8 100644 --- a/src/or/dirserv.h +++ b/src/or/dirserv.h @@ -108,8 +108,7 @@ int dirserv_get_routerdescs(smartlist_t *descs_out, const char *key, const char **msg); void dirserv_orconn_tls_done(const char *address, uint16_t or_port, - const char *digest_rcvd, - int as_advertised); + const char *digest_rcvd); int dirserv_should_launch_reachability_test(const routerinfo_t *ri, const routerinfo_t *ri_old); void dirserv_single_reachability_test(time_t now, routerinfo_t *router); From 3031def7268ce0cc6f3ab015c0df382295494bb9 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 29 Mar 2012 16:45:25 -0400 Subject: [PATCH 3/4] checking "same addr/port but with nonmatching keys" is obsolete Specifically, I believe it dates back to when extend cells had address:port but no digest in them. The special edge case is certainly not worth the complexity these days. --- src/or/dirserv.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 467129c728..dfecbc8a09 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -3271,16 +3271,11 @@ dirserv_orconn_tls_done(const char *address, uint16_t or_port, const char *digest_rcvd) { - routerinfo_t *ri = NULL; + routerinfo_t *ri = router_get_mutable_by_digest(digest_rcvd); time_t now = time(NULL); tor_assert(address); tor_assert(digest_rcvd); - /* FFFF Maybe we should reinstate the code that dumps routers with the same - * addr/port but with nonmatching keys, but instead of dumping, we should - * skip testing. */ - - ri = router_get_mutable_by_digest(digest_rcvd); if (ri == NULL) return; From 491ffa540fda6a9cd8d75aad7d20945a3ae1df65 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 30 Mar 2012 10:14:31 -0400 Subject: [PATCH 4/4] Move router lookup to _after_ we assert that its argument is set A previous commit in the 5527 branch had moved router_get_mutable_by_digest(digest_rcvd) to happen before we did tor_assert(digest_rcvd), which would have defeated the purpose of the assert. --- src/or/dirserv.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index dfecbc8a09..11f235caf4 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -3271,11 +3271,13 @@ dirserv_orconn_tls_done(const char *address, uint16_t or_port, const char *digest_rcvd) { - routerinfo_t *ri = router_get_mutable_by_digest(digest_rcvd); + routerinfo_t *ri; time_t now = time(NULL); tor_assert(address); tor_assert(digest_rcvd); + ri = router_get_mutable_by_digest(digest_rcvd); + if (ri == NULL) return;