r15991@catbus: nickm | 2007-10-20 20:08:29 -0400

 Fix a nasty bug in DownloadExtraInfo implementation where we would discard, download, discard, download ad infinitum.


svn:r12069

5 files changed, 30 insertions, 22 deletions

diff --git a/ChangeLog b/ChangeLog
index b9df1592cc..d62e6c05cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -23,6 +23,9 @@ Changes in version 0.2.0.9-alpha - 2007-10-??
       that it shouldn't be considered to exist at all anymore. Now we
       clear all the flags for routers that fall out of the networkstatus
       consensus. Fixes bug 529.
+    - Fix awful behavior in DownloadExtraInfo option where we'd fetch
+      extrainfo documents and then discard them immediately for not
+      matching the latest router.
 
   o Minor features (v3 directory protocol):
     - Allow tor-gencert to generate a new certificate without replacing the
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index 00bf89ee6f..420bf08d1d 100644
--- a/src/or/dirserv.c
+++ b/src/or/dirserv.c
@@ -701,7 +701,7 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg)
     extrainfo_free(ei);
     return -1;
   }
-  if ((r = routerinfo_incompatible_with_extrainfo(ri, ei, msg))) {
+  if ((r = routerinfo_incompatible_with_extrainfo(ri, ei, NULL, msg))) {
     extrainfo_free(ei);
     return r < 0 ? 0 : -1;
   }
diff --git a/src/or/or.h b/src/or/or.h
index 2c9f436f80..167035a7c3 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -3612,7 +3612,7 @@ void routerlist_reset_warnings(void);
 void router_set_status(const char *digest, int up);
 int router_add_to_routerlist(routerinfo_t *router, const char **msg,
                              int from_cache, int from_fetch);
-void router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg,
+int router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg,
                                         int from_cache, int from_fetch);
 void routerlist_remove_old_routers(void);
 int router_load_single_router(const char *s, uint8_t purpose, int cache,
@@ -3643,6 +3643,7 @@ void router_dir_info_changed(void);
 void router_reset_descriptor_download_failures(void);
 int router_differences_are_cosmetic(routerinfo_t *r1, routerinfo_t *r2);
 int routerinfo_incompatible_with_extrainfo(routerinfo_t *ri, extrainfo_t *ei,
+                                           signed_descriptor_t *sd,
                                            const char **msg);
 void routerlist_assert_ok(routerlist_t *rl);
 const char *esc_router_info(routerinfo_t *router);
diff --git a/src/or/router.c b/src/or/router.c
index 550699ab23..90403106a3 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -1305,7 +1305,7 @@ router_rebuild_descriptor(int force)
   router_get_router_hash(ri->cache_info.signed_descriptor_body,
                          ri->cache_info.signed_descriptor_digest);
 
-  tor_assert(! routerinfo_incompatible_with_extrainfo(ri, ei, NULL));
+  tor_assert(! routerinfo_incompatible_with_extrainfo(ri, ei, NULL, NULL));
 
   if (desc_routerinfo)
     routerinfo_free(desc_routerinfo);
diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index 1317c6230f..ca00cefeaa 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -2205,7 +2205,8 @@ extrainfo_insert(routerlist_t *rl, extrainfo_t *ei)
   int r = 0;
   routerinfo_t *ri = rimap_get(rl->identity_map,
                                ei->cache_info.identity_digest);
-  signed_descriptor_t *sd;
+  signed_descriptor_t *sd =
+    sdmap_get(rl->desc_by_eid_map, ei->cache_info.signed_descriptor_digest);
   extrainfo_t *ei_tmp;
 
   {
@@ -2218,16 +2219,8 @@ extrainfo_insert(routerlist_t *rl, extrainfo_t *ei)
     /* This router is unknown; we can't even verify the signature. Give up.*/
     goto done;
   }
-  if (routerinfo_incompatible_with_extrainfo(ri, ei, NULL)) {
-    if (ei->bad_sig) /* If the signature didn't check, it's just wrong. */
-      goto done;
-    sd = sdmap_get(rl->desc_by_eid_map,
-                   ei->cache_info.signed_descriptor_digest);
-    if (!sd ||
-        memcmp(sd->identity_digest, ei->cache_info.identity_digest,
-               DIGEST_LEN) ||
-        sd->published_on != ei->cache_info.published_on)
-      goto done;
+  if (routerinfo_incompatible_with_extrainfo(ri, ei, sd, NULL)) {
+    goto done;
   }
 
   /* Okay, if we make it here, we definitely have a router corresponding to
@@ -2740,8 +2733,10 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg,
 }
 
 /** Insert <b>ei</b> into the routerlist, or free it. Other arguments are
- * as for router_add_to_routerlist(). */
-void
+ * as for router_add_to_routerlist().
+ * DOCDOC Inserted
+ */
+int
 router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg,
                                    int from_cache, int from_fetch)
 {
@@ -2754,6 +2749,8 @@ router_add_extrainfo_to_routerlist(extrainfo_t *ei, const char **msg,
   if (inserted && !from_cache)
     signed_desc_append_to_journal(&ei->cache_info,
                                   &routerlist->extrainfo_store);
+
+  return inserted;
 }
 
 /** Sorting helper: return &lt;0, 0, or &gt;0 depending on whether the
@@ -3174,7 +3171,9 @@ router_load_extrainfo_from_string(const char *s, const char *eos,
   log_info(LD_DIR, "%d elements to add", smartlist_len(extrainfo_list));
 
   SMARTLIST_FOREACH(extrainfo_list, extrainfo_t *, ei, {
-      if (requested_fingerprints) {
+      int added =
+        router_add_extrainfo_to_routerlist(ei, &msg, from_cache, !from_cache);
+      if (added && requested_fingerprints) {
         char fp[HEX_DIGEST_LEN+1];
         base16_encode(fp, sizeof(fp), descriptor_digests ?
                         ei->cache_info.signed_descriptor_digest :
@@ -3184,7 +3183,6 @@ router_load_extrainfo_from_string(const char *s, const char *eos,
         /* XXX020 We silently let people stuff us with extrainfos we
          * didn't ask for. Is this a problem? -RD */
       }
-      router_add_extrainfo_to_routerlist(ei, &msg, from_cache, !from_cache);
     });
 
   routerlist_assert_ok(routerlist);
@@ -4042,13 +4040,19 @@ router_differences_are_cosmetic(routerinfo_t *r1, routerinfo_t *r2)
  * dropped.  Return 0 for "compatible", return 1 for "reject, and inform
  * whoever uploaded <b>ei</b>, and return -1 for "reject silently.".  If
  * <b>msg</b> is present, set *<b>msg</b> to a description of the
- * incompatibility (if any). */
+ * incompatibility (if any)
+ *
+ * DOCDOC sd.
+ **/
 int
 routerinfo_incompatible_with_extrainfo(routerinfo_t *ri, extrainfo_t *ei,
+                                       signed_descriptor_t *sd,
                                        const char **msg)
 {
   tor_assert(ri);
   tor_assert(ei);
+  if (!sd)
+    sd = &ri->cache_info;
 
   if (ei->bad_sig) {
     if (msg) *msg = "Extrainfo signature was bad, or signed with wrong key.";
@@ -4079,16 +4083,16 @@ routerinfo_incompatible_with_extrainfo(routerinfo_t *ri, extrainfo_t *ei,
     tor_free(ei->pending_sig);
   }
 
-  if (ei->cache_info.published_on < ri->cache_info.published_on) {
+  if (ei->cache_info.published_on < sd->published_on) {
     if (msg) *msg = "Extrainfo published time did not match routerdesc";
     return 1;
-  } else if (ei->cache_info.published_on > ri->cache_info.published_on) {
+  } else if (ei->cache_info.published_on > sd->published_on) {
     if (msg) *msg = "Extrainfo published time did not match routerdesc";
     return -1;
   }
 
   if (memcmp(ei->cache_info.signed_descriptor_digest,
-             ri->cache_info.extra_info_digest, DIGEST_LEN)) {
+             sd->extra_info_digest, DIGEST_LEN)) {
     if (msg) *msg = "Extrainfo digest did not match value from routerdesc";
     return 1; /* Digest doesn't match declared value. */
   }