From c980be951157b68e6bd658ce01850e96ce97d422 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 24 Aug 2017 16:17:26 +0300 Subject: [PATCH] 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. --- src/or/hs_service.c | 48 +++++++++++++++++--------------------- src/or/hs_service.h | 4 ++++ src/test/test_hs_common.c | 7 +++--- src/test/test_hs_service.c | 13 +++++++++++ 4 files changed, 41 insertions(+), 31 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index e530a10a16..0fc5a91445 100644 --- 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 5bd19931fc..d902e0b2c7 100644 --- 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 ec938ff5d1..b1e47adf24 100644 --- 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 cf1c36a528..4c7880c0ee 100644 --- 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_);