Merge remote-tracking branch 'asn/bug23346'

3 files changed, 90 insertions, 82 deletions

diff --git a/src/or/hs_service.c b/src/or/hs_service.c
index 82adc7d455..796efa0c9b 100644
--- a/src/or/hs_service.c
+++ b/src/or/hs_service.c
@@ -1562,7 +1562,6 @@ service_desc_note_upload(hs_service_descriptor_t *desc, const node_t *hsdir)
 
   if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) {
     smartlist_add_strdup(desc->previous_hsdirs, b64_digest);
-    smartlist_sort_strings(desc->previous_hsdirs);
   }
 }
 
@@ -2333,6 +2332,10 @@ upload_descriptor_to_all(const hs_service_t *service,
   hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num,
                             for_next_period, 0, responsible_dirs);
 
+  /** Clear list of previous hsdirs since we are about to upload to a new
+   *  list. Let's keep it up to date. */
+  service_desc_clear_previous_hsdirs(desc);
+
   /* For each responsible HSDir we have, initiate an upload command. */
   SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *,
                           hsdir_rs) {
@@ -2370,9 +2373,8 @@ STATIC int
 service_desc_hsdirs_changed(const hs_service_t *service,
                             const hs_service_descriptor_t *desc)
 {
-  int retval = 0;
+  int should_reupload = 0;
   smartlist_t *responsible_dirs = smartlist_new();
-  smartlist_t *b64_responsible_dirs = smartlist_new();
 
   /* No desc upload has happened yet: it will happen eventually */
   if (!desc->previous_hsdirs || !smartlist_len(desc->previous_hsdirs)) {
@@ -2383,32 +2385,24 @@ service_desc_hsdirs_changed(const hs_service_t *service,
   hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num,
                             service->desc_next == desc, 0, responsible_dirs);
 
-  /* Make a second list with their b64ed identity digests, so that we can
-   * compare it with out previous list of hsdirs */
+  /* Check if any new hsdirs have been added to the responsible hsdirs set:
+   * Iterate over the list of new hsdirs, and reupload if any of them is not
+   * present in the list of previous hsdirs.
+   */
   SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) {
     char b64_digest[BASE64_DIGEST_LEN+1] = {0};
     digest_to_base64(b64_digest, hsdir_rs->identity_digest);
-    smartlist_add_strdup(b64_responsible_dirs, b64_digest);
-  } SMARTLIST_FOREACH_END(hsdir_rs);
 
-  /* Sort this new smartlist so that we can compare it with the other one */
-  smartlist_sort_strings(b64_responsible_dirs);
-
-  /* Check whether the set of HSDirs changed */
-  if (!smartlist_strings_eq(b64_responsible_dirs, desc->previous_hsdirs)) {
-    log_info(LD_GENERAL, "Received new dirinfo and set of hsdirs changed!");
-    retval = 1;
-  } else {
-    log_debug(LD_GENERAL, "No change in hsdir set!");
-  }
+    if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) {
+      should_reupload = 1;
+      break;
+    }
+  } SMARTLIST_FOREACH_END(hsdir_rs);
 
  done:
   smartlist_free(responsible_dirs);
 
-  SMARTLIST_FOREACH(b64_responsible_dirs, char*, s, tor_free(s));
-  smartlist_free(b64_responsible_dirs);
-
-  return retval;
+  return should_reupload;
 }
 
 /* Return 1 if the given descriptor from the given service can be uploaded
diff --git a/src/or/hs_service.h b/src/or/hs_service.h
index cd4874c8ee..317b9d795d 100644
--- a/src/or/hs_service.h
+++ b/src/or/hs_service.h
@@ -126,7 +126,7 @@ typedef struct hs_service_descriptor_t {
   /** List of the responsible HSDirs (their b64ed identity digest) last time we
    *  uploaded this descriptor. If the set of responsible HSDirs is different
    *  from this list, this means we received new dirinfo and we need to
-   *  reupload our descriptor. This list is always sorted lexicographically. */
+   *  reupload our descriptor. */
   smartlist_t *previous_hsdirs;
 } hs_service_descriptor_t;
 
diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c
index b9215ea187..9980892951 100644
--- a/src/test/test_hs_common.c
+++ b/src/test/test_hs_common.c
@@ -21,6 +21,7 @@
 #include "networkstatus.h"
 #include "directory.h"
 #include "nodelist.h"
+#include "routerlist.h"
 #include "statefile.h"
 
 /** Test the validation of HS v3 addresses */
@@ -362,20 +363,26 @@ test_desc_overlap_period_testnet(void *arg)
 
 static void
 helper_add_hsdir_to_networkstatus(networkstatus_t *ns,
-                                  const uint8_t *identity,
-                                  const uint8_t *curr_hsdir_index,
+                                  int identity_idx,
                                   const char *nickname,
                                   int is_hsdir)
 {
   routerstatus_t *rs = tor_malloc_zero(sizeof(routerstatus_t));
   routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t));
-
+  uint8_t identity[DIGEST_LEN];
+  uint8_t curr_hsdir_index[DIGEST256_LEN];
   tor_addr_t ipv4_addr;
+
+  memset(identity, identity_idx, sizeof(identity));
+  memset(curr_hsdir_index, identity_idx, sizeof(curr_hsdir_index));
+
   memcpy(rs->identity_digest, identity, DIGEST_LEN);
   rs->is_hs_dir = is_hsdir;
   rs->supports_v3_hsdir = 1;
+  strlcpy(rs->nickname, nickname, sizeof(rs->nickname));
   tor_addr_parse(&ipv4_addr, "1.2.3.4");
   ri->addr = tor_addr_to_ipv4h(&ipv4_addr);
+  rs->addr = tor_addr_to_ipv4h(&ipv4_addr);
   ri->nickname = tor_strdup(nickname);
   ri->protocol_list = tor_strdup("HSDir=1-2 LinkAuth=3");
   memcpy(ri->cache_info.identity_digest, identity, DIGEST_LEN);
@@ -388,7 +395,7 @@ helper_add_hsdir_to_networkstatus(networkstatus_t *ns,
   smartlist_add(ns->routerstatus_list, rs);
 
  done:
-  ;
+  routerinfo_free(ri);
 }
 
 static networkstatus_t *mock_ns = NULL;
@@ -412,6 +419,7 @@ mock_networkstatus_get_latest_consensus(void)
   mock_ns->valid_until = now+2;
   /* Create routerstatus list */
   mock_ns->routerstatus_list = smartlist_new();
+  mock_ns->type = NS_TYPE_CONSENSUS;
 
   return mock_ns;
 }
@@ -435,36 +443,15 @@ test_responsible_hsdirs(void *arg)
   ns = networkstatus_get_latest_consensus();
 
   { /* First router: HSdir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "let_me";
-    memset(identity, 1, sizeof(identity));
-    memset(curr_hsdir_index, 1, sizeof(curr_hsdir_index));
-
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 1);
+    helper_add_hsdir_to_networkstatus(ns, 1, "igor", 1);
   }
 
   { /* Second HSDir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "show_you";
-    memset(identity, 2, sizeof(identity));
-    memset(curr_hsdir_index, 2, sizeof(curr_hsdir_index));
-
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 1);
+    helper_add_hsdir_to_networkstatus(ns, 2, "victor", 1);
   }
 
   { /* Third relay but not HSDir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "how_to_dance";
-    memset(identity, 3, sizeof(identity));
-    memset(curr_hsdir_index, 3, sizeof(curr_hsdir_index));
-
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 0);
+    helper_add_hsdir_to_networkstatus(ns, 3, "spyro", 0);
   }
 
   ed25519_keypair_t kp;
@@ -579,32 +566,19 @@ test_desc_reupload_logic(void *arg)
   tt_int_op(hs_service_get_num_services(), OP_EQ, 1);
 
   /* Now let's create our hash ring: */
-  { /* First HSDir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "let_me";
-    memset(identity, 1, sizeof(identity));
-    memset(curr_hsdir_index, 1, sizeof(curr_hsdir_index));
-
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 1);
-  }
-
-  { /* Second HSDir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "show_you";
-    memset(identity, 2, sizeof(identity));
-    memset(curr_hsdir_index, 2, sizeof(curr_hsdir_index));
-
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 1);
+  {
+    helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1);
+    helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1);
+    helper_add_hsdir_to_networkstatus(ns, 3, "aaron", 1);
+    helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1);
+    helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1);
+    helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1);
   }
 
   /* Now let's upload our desc to all hsdirs */
   upload_descriptor_to_all(service, desc, 0);
   /* Check that previous hsdirs were populated */
-  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 2);
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
   /* Poison next upload time so that we can see if it was changed by
    * router_dir_info_changed(). No changes in hash ring so far, so the upload
@@ -613,17 +587,23 @@ test_desc_reupload_logic(void *arg)
   router_dir_info_changed();
   tt_int_op(desc->next_upload_time, OP_EQ, 42);
 
-  /* Now change the HSDir hash ring by adding another node */
-
-  { /* Third HSDir */
-    uint8_t identity[DIGEST_LEN];
-    uint8_t curr_hsdir_index[DIGEST256_LEN];
-    char nickname[] = "how_to_dance";
-    memset(identity, 3, sizeof(identity));
-    memset(curr_hsdir_index, 3, sizeof(curr_hsdir_index));
+  /* Now change the HSDir hash ring by swapping nora for aaron.
+   * Start by clearing the hash ring */
+  {
+    SMARTLIST_FOREACH(ns->routerstatus_list,
+                      routerstatus_t *, rs, routerstatus_free(rs));
+    smartlist_clear(ns->routerstatus_list);
+    nodelist_free_all();
+    routerlist_free_all();
+  }
 
-    helper_add_hsdir_to_networkstatus(ns, identity,
-                                      curr_hsdir_index, nickname, 1);
+  { /* Now add back all the nodes */
+    helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1);
+    helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1);
+    helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1);
+    helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1);
+    helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1);
+    helper_add_hsdir_to_networkstatus(ns, 7, "nora", 1);
   }
 
   /* Now call service_desc_hsdirs_changed() and see that it detected the hash
@@ -631,6 +611,35 @@ test_desc_reupload_logic(void *arg)
   time_t now = approx_time();
   tt_assert(now);
   tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1);
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
+
+  /* Now order another upload and see that we keep having 6 prev hsdirs */
+  upload_descriptor_to_all(service, desc, 0);
+  /* Check that previous hsdirs were populated */
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
+
+  /* Now restore the HSDir hash ring to its original state by swapping back
+     aaron for nora */
+  /* First clear up the hash ring */
+  {
+    SMARTLIST_FOREACH(ns->routerstatus_list,
+                      routerstatus_t *, rs, routerstatus_free(rs));
+    smartlist_clear(ns->routerstatus_list);
+    nodelist_free_all();
+    routerlist_free_all();
+  }
+
+  { /* Now populate the hash ring again */
+    helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1);
+    helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1);
+    helper_add_hsdir_to_networkstatus(ns, 3, "aaron", 1);
+    helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1);
+    helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1);
+    helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1);
+  }
+
+  /* Check that our algorithm catches this change of hsdirs */
+  tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1);
 
   /* Now pretend that the descriptor changed, and order a reupload to all
      HSDirs. Make sure that the set of previous HSDirs was cleared. */
@@ -639,9 +648,14 @@ test_desc_reupload_logic(void *arg)
 
   /* Now reupload again: see that the prev hsdir set got populated again. */
   upload_descriptor_to_all(service, desc, 0);
-  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 3);
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
  done:
+  SMARTLIST_FOREACH(ns->routerstatus_list,
+                    routerstatus_t *, rs, routerstatus_free(rs));
+  smartlist_clear(ns->routerstatus_list);
+  networkstatus_vote_free(ns);
+  nodelist_free_all();
   hs_free_all();
 }