prop224: Refactor descriptor reupload logic.

We refactor the descriptor reupload logic to be similar to the v2 logic
where we update a global 'consider_republishing_rend_descriptors' flag
and then we use that to check for hash ring changes during the global
hidden service callbacks.

This fixes bugs where we would inspect the hash ring immediately as we
receive new dirinfo (e.g. consensus) but before running the hidden
service housekeeping events. That was leaving us in an inconsistent
state wrt hsdir indices and causing bugs all around.

diff --git a/src/or/hs_service.c b/src/or/hs_service.c
index e530a10a16707cb81f1069356f935f966d7ec8cf..0fc5a91445b71a0e87238165a5f1c622966c634b 100644 (file)
--- a/src/or/hs_service.c
+++ b/src/or/hs_service.c
@@ -72,6 +72,11 @@ static const char address_tld[] = "onion";
  * loading keys requires that we are an actual running tor process. */
 static smartlist_t *hs_service_staging_list;
 
+/** True if the list of available router descriptors might have changed which
+ *  might result in an altered hash ring. Check if the hash ring changed and
+ *  reupload if needed */
+static int consider_republishing_hs_descriptors = 0;
+
 static void set_descriptor_revision_counter(hs_descriptor_t *hs_desc);
 
 /* Helper: Function to compare two objects in the service map. Return 1 if the
@@ -2429,7 +2434,14 @@ run_upload_descriptor_event(time_t now)
     FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
       int for_next_period = 0;
 
-      /* Can this descriptor be uploaed? */
+      /* If we were asked to re-examine the hash ring, and it changed, then
+         schedule an upload */
+      if (consider_republishing_hs_descriptors &&
+          service_desc_hsdirs_changed(service, desc)) {
+        service_desc_schedule_upload(desc, now, 0);
+      }
+
+      /* Can this descriptor be uploaded? */
       if (!should_service_upload_descriptor(service, desc, now)) {
         continue;
       }
@@ -2456,6 +2468,9 @@ run_upload_descriptor_event(time_t now)
       upload_descriptor_to_all(service, desc, for_next_period);
     } FOR_EACH_DESCRIPTOR_END;
   } FOR_EACH_SERVICE_END;
+
+  /* We are done considering whether to republish rend descriptors */
+  consider_republishing_hs_descriptors = 0;
 }
 
 /* Called when the introduction point circuit is done building and ready to be
@@ -2738,7 +2753,7 @@ service_add_fnames_to_list(const hs_service_t *service, smartlist_t *list)
 
 /** The set of HSDirs have changed: check if the change affects our descriptor
  *  HSDir placement, and if it does, reupload the desc. */
-static int
+int
 service_desc_hsdirs_changed(const hs_service_t *service,
                             const hs_service_descriptor_t *desc)
 {
@@ -2788,34 +2803,13 @@ service_desc_hsdirs_changed(const hs_service_t *service,
 /* ========== */
 
 /* We just received a new batch of descriptors which might affect the shape of
- * the HSDir hash ring. Signal that we should re-upload our HS descriptors. */
+ * the HSDir hash ring. Signal that we should reexamine the hash ring and
+ * re-upload our HS descriptors if needed. */
 void
 hs_hsdir_set_changed_consider_reupload(void)
 {
-  time_t now = approx_time();
-
-  /* Check if HS subsystem is initialized */
-  if (!hs_service_map) {
-    return;
-  }
-
-  /* Basic test: If we have not bootstrapped 100% yet, no point in even trying
-     to upload descriptor. */
-  if (!router_have_minimum_dir_info()) {
-    return;
-  }
-
-  log_info(LD_GENERAL, "Received new dirinfo: Checking hash ring for changes");
-
-  /* Go over all descriptors and check if the set of HSDirs changed for any of
-   * them. Schedule reupload if so. */
-  FOR_EACH_SERVICE_BEGIN(service) {
-    FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
-      if (service_desc_hsdirs_changed(service, desc)) {
-        service_desc_schedule_upload(desc, now, 0);
-      }
-    } FOR_EACH_DESCRIPTOR_END;
-  } FOR_EACH_SERVICE_END;
+  log_info(LD_REND, "New dirinfo arrived: consider reuploading descriptor");
+  consider_republishing_hs_descriptors = 1;
 }
 
 /* Return the number of service we have configured and usable. */

diff --git a/src/or/hs_service.h b/src/or/hs_service.h
index 5bd19931fc60c5f929af7545a6a4d0375623c2cb..d902e0b2c7faf98ec2fb1524e709c80762a600d8 100644 (file)
--- a/src/or/hs_service.h
+++ b/src/or/hs_service.h
@@ -280,6 +280,10 @@ int hs_service_receive_introduce2(origin_circuit_t *circ,
 
 void hs_service_intro_circ_has_closed(origin_circuit_t *circ);
 
+int service_desc_hsdirs_changed(const hs_service_t *service,
+                                const hs_service_descriptor_t *desc);
+
+
 #ifdef HS_SERVICE_PRIVATE
 
 #ifdef TOR_UNIT_TESTS

diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c
index ec938ff5d1735370b7b21bc496ff6c70dfe8d76f..b1e47adf24ffc8ccd6dc69605019ae727d404b11 100644 (file)
--- a/src/test/test_hs_common.c
+++ b/src/test/test_hs_common.c
@@ -626,12 +626,11 @@ test_desc_reupload_logic(void *arg)
                                       curr_hsdir_index, nickname, 1);
   }
 
-  /* Now call router_dir_info_changed() again and see that it detected the hash
-     ring change and updated the upload time */
+  /* Now call service_desc_hsdirs_changed() and see that it detected the hash
+     ring change */
   time_t now = approx_time();
   tt_assert(now);
-  router_dir_info_changed();
-  tt_int_op(desc->next_upload_time, OP_EQ, now);
+  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. */

diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c
index cf1c36a528b5270f58ebb7a442a26636aa99fadc..4c7880c0ee8d8e6e207d57c6f9dc39a5ff360f78 100644 (file)
--- a/src/test/test_hs_service.c
+++ b/src/test/test_hs_service.c
@@ -1005,6 +1005,13 @@ test_rotate_descriptors(void *arg)
             OP_EQ, 0);
   tt_assert(service->desc_next == NULL);
 
+  /* Now let's re-create desc_next and get out of overlap period. We should
+     test that desc_current gets replaced by desc_next, and desc_next becomes
+     NULL. */
+  desc_next = service_descriptor_new();
+  desc_next->next_upload_time = 240; /* Our marker to recognize it. */
+  service->desc_next = desc_next;
+
   /* Going out of the overlap period. */
   ret = parse_rfc1123_time("Sat, 26 Oct 1985 12:00:00 UTC",
                            &mock_ns.valid_after);
@@ -1017,6 +1024,12 @@ test_rotate_descriptors(void *arg)
   tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next));
   tt_assert(service->desc_next == NULL);
 
+  /* Calling rotate_all_descriptors() another time should do nothing */
+  rotate_all_descriptors(now);
+  tt_int_op(service->state.in_overlap_period, OP_EQ, 0);
+  tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next));
+  tt_assert(service->desc_next == NULL);
+
  done:
   hs_free_all();
   UNMOCK(circuit_mark_for_close_);