Merge commit 'nickm/1776_redux_v1' into maint-0.2.2

2 files changed, 60 insertions, 24 deletions

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..670574a9d3 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;
@@ -2757,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();
@@ -2766,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);
@@ -2986,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;
@@ -3030,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;
@@ -3039,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);
@@ -3209,10 +3232,18 @@ 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);
-    if (! (routerinfo_is_a_configured_bridge(router) &&
-            (router->purpose == ROUTER_PURPOSE_BRIDGE ||
-             old_router->purpose != ROUTER_PURPOSE_BRIDGE))) {
+
+    /* 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 &&
+        !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);