From 114a371c0ea43aae667b71b2ba1e0a7754d08ea5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 2 Nov 2010 11:20:09 -0400 Subject: [PATCH 1/3] Fix the assert in bug 1776 In the case where old_router == NULL but sdmap has an entry for the router, we can currently safely infer that the old_router was not a bridge. Add an assert to ensure that this remains true, and fix the logic not to die with the tor_assert(old_router) call. --- changes/bug1776_redux | 5 +++++ src/or/routerlist.c | 17 ++++++++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) create mode 100644 changes/bug1776_redux diff --git a/changes/bug1776_redux b/changes/bug1776_redux new file mode 100644 index 0000000000..0e2e5cd57c --- /dev/null +++ b/changes/bug1776_redux @@ -0,0 +1,5 @@ + o Major bugfixes + - Avoid an assertion failure when we as an authority receive a + duplicate upload of a router descriptor that we already have, + but which we previously considered an obsolete descriptor. + Fixes another case of bug 1776. Bugfix on 0.2.2.16-alpha. diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 8ed9a7f5e6..94f57cc39f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2661,12 +2661,15 @@ signed_descriptor_free(signed_descriptor_t *sd) tor_free(sd); } -/** Extract a signed_descriptor_t from a routerinfo, and free the routerinfo. +/** Extract a signed_descriptor_t from a general routerinfo, and free the + * routerinfo. */ static signed_descriptor_t * signed_descriptor_from_routerinfo(routerinfo_t *ri) { - signed_descriptor_t *sd = tor_malloc_zero(sizeof(signed_descriptor_t)); + signed_descriptor_t *sd; + tor_assert(ri->purpose == ROUTER_PURPOSE_GENERAL); + sd = tor_malloc_zero(sizeof(signed_descriptor_t)); memcpy(sd, &(ri->cache_info), sizeof(signed_descriptor_t)); sd->routerlist_index = -1; ri->cache_info.signed_descriptor_body = NULL; @@ -3209,10 +3212,14 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg, /* If we have this descriptor already and the new descriptor is a bridge * descriptor, replace it. If we had a bridge descriptor before and the * new one is not a bridge descriptor, don't replace it. */ - tor_assert(old_router); + + /* Only members of routerlist->identity_map can be bridges; we don't + * put bridges in old_routers. */ + const int was_bridge = old_router && + old_router->purpose == ROUTER_PURPOSE_BRIDGE; + if (! (routerinfo_is_a_configured_bridge(router) && - (router->purpose == ROUTER_PURPOSE_BRIDGE || - old_router->purpose != ROUTER_PURPOSE_BRIDGE))) { + (router->purpose == ROUTER_PURPOSE_BRIDGE || !was_bridge))) { log_info(LD_DIR, "Dropping descriptor that we already have for router '%s'", router->nickname); From accc51b68c3ec898488a170973bf306d91ebe691 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Nov 2010 14:55:00 -0500 Subject: [PATCH 2/3] Bulletproof the routerlist manipulation functions to handle reinserting the same descriptor --- src/or/routerlist.c | 56 ++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 94f57cc39f..0e9265fe4f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2760,6 +2760,7 @@ static void routerlist_insert(routerlist_t *rl, routerinfo_t *ri) { routerinfo_t *ri_old; + signed_descriptor_t *sd_old; { /* XXXX Remove if this slows us down. */ routerinfo_t *ri_generated = router_get_my_routerinfo(); @@ -2769,8 +2770,16 @@ routerlist_insert(routerlist_t *rl, routerinfo_t *ri) ri_old = rimap_set(rl->identity_map, ri->cache_info.identity_digest, ri); tor_assert(!ri_old); - sdmap_set(rl->desc_digest_map, ri->cache_info.signed_descriptor_digest, - &(ri->cache_info)); + + sd_old = sdmap_set(rl->desc_digest_map, + ri->cache_info.signed_descriptor_digest, + &(ri->cache_info)); + if (sd_old) { + rl->desc_store.bytes_dropped += sd_old->signed_descriptor_len; + sdmap_remove(rl->desc_by_eid_map, sd_old->extra_info_digest); + signed_descriptor_free(sd_old); + } + if (!tor_digest_is_zero(ri->cache_info.extra_info_digest)) sdmap_set(rl->desc_by_eid_map, ri->cache_info.extra_info_digest, &ri->cache_info); @@ -2989,6 +2998,7 @@ routerlist_replace(routerlist_t *rl, routerinfo_t *ri_old, routerinfo_t *ri_new) { int idx; + int same_descriptors; routerinfo_t *ri_tmp; extrainfo_t *ei_tmp; @@ -3033,8 +3043,15 @@ routerlist_replace(routerlist_t *rl, routerinfo_t *ri_old, &ri_new->cache_info); } + same_descriptors = ! memcmp(ri_old->cache_info.signed_descriptor_digest, + ri_new->cache_info.signed_descriptor_digest, + DIGEST_LEN); + if (should_cache_old_descriptors() && - ri_old->purpose == ROUTER_PURPOSE_GENERAL) { + ri_old->purpose == ROUTER_PURPOSE_GENERAL && + !same_descriptors) { + /* ri_old is going to become a signed_descriptor_t and go into + * old_routers */ signed_descriptor_t *sd = signed_descriptor_from_routerinfo(ri_old); smartlist_add(rl->old_routers, sd); sd->routerlist_index = smartlist_len(rl->old_routers)-1; @@ -3042,24 +3059,27 @@ routerlist_replace(routerlist_t *rl, routerinfo_t *ri_old, if (!tor_digest_is_zero(sd->extra_info_digest)) sdmap_set(rl->desc_by_eid_map, sd->extra_info_digest, sd); } else { - if (memcmp(ri_old->cache_info.signed_descriptor_digest, - ri_new->cache_info.signed_descriptor_digest, - DIGEST_LEN)) { - /* digests don't match; digestmap_set didn't replace */ + /* We're dropping ri_old. */ + if (!same_descriptors) { + /* digests don't match; The sdmap_set above didn't replace */ sdmap_remove(rl->desc_digest_map, ri_old->cache_info.signed_descriptor_digest); - } - ei_tmp = eimap_remove(rl->extra_info_map, - ri_old->cache_info.extra_info_digest); - if (ei_tmp) { - rl->extrainfo_store.bytes_dropped += - ei_tmp->cache_info.signed_descriptor_len; - extrainfo_free(ei_tmp); - } - if (!tor_digest_is_zero(ri_old->cache_info.extra_info_digest)) { - sdmap_remove(rl->desc_by_eid_map, - ri_old->cache_info.extra_info_digest); + if (memcmp(ri_old->cache_info.extra_info_digest, + ri_new->cache_info.extra_info_digest, DIGEST_LEN)) { + ei_tmp = eimap_remove(rl->extra_info_map, + ri_old->cache_info.extra_info_digest); + if (ei_tmp) { + rl->extrainfo_store.bytes_dropped += + ei_tmp->cache_info.signed_descriptor_len; + extrainfo_free(ei_tmp); + } + } + + if (!tor_digest_is_zero(ri_old->cache_info.extra_info_digest)) { + sdmap_remove(rl->desc_by_eid_map, + ri_old->cache_info.extra_info_digest); + } } rl->desc_store.bytes_dropped += ri_old->cache_info.signed_descriptor_len; routerinfo_free(ri_old); From 45b500d5a60ca6c43e5a545b9e15e203be1feeac Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 15 Nov 2010 19:38:19 -0500 Subject: [PATCH 3/3] Clean up my 1776 fix a bit Sebastian notes (and I think correctly) that one of our ||s should have been an &&, which simplifies a boolean expression to decide whether to replace bridges. I'm also refactoring out the negation at the start of the expression, to make it more readable. --- src/or/routerlist.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 0e9265fe4f..670574a9d3 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -3238,8 +3238,12 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg, const int was_bridge = old_router && old_router->purpose == ROUTER_PURPOSE_BRIDGE; - if (! (routerinfo_is_a_configured_bridge(router) && - (router->purpose == ROUTER_PURPOSE_BRIDGE || !was_bridge))) { + if (routerinfo_is_a_configured_bridge(router) && + router->purpose == ROUTER_PURPOSE_BRIDGE && + !was_bridge) { + log_info(LD_DIR, "Replacing non-bridge descriptor with bridge " + "descriptor for router '%s'", router->nickname); + } else { log_info(LD_DIR, "Dropping descriptor that we already have for router '%s'", router->nickname);