Skip to content

Commit

Permalink
prop224: Clear list of prev hsdirs before we upload all descs.
Browse files Browse the repository at this point in the history
This fixes a serious bug in our hsdir set change logic:

We used to add nodes in the list of previous hsdirs everytime we
uploaded to a new hsdir and we only cleared the list when we built a new
descriptor. This means that our prev_hsdirs list could end up with 7
hsdirs, if for some reason we ended up uploading our desc to 7 hsdirs
before rebuilding our descriptor (e.g. this can happen if the set of
hsdirs changed).

After our previous hdsir set had 7 nodes, then our old algorithm would
always think that the set has changed since it was comparing a smartlist
with 7 elements against a smartlist with 6 elements.

This commit fixes this bug, by clearning the prev_hsdirs list before we
upload to all hsdirs. This makes sure that our prev_hsdirs list always
contains the latest hsdirs!
  • Loading branch information
asn-d6 committed Aug 30, 2017
1 parent 1dc21b8 commit b9f849b
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
4 changes: 4 additions & 0 deletions src/or/hs_service.c
Original file line number Diff line number Diff line change
Expand Up @@ -2316,6 +2316,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) {
Expand Down
4 changes: 2 additions & 2 deletions src/test/test_hs_common.c
Original file line number Diff line number Diff line change
Expand Up @@ -613,10 +613,10 @@ test_desc_reupload_logic(void *arg)
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 how we end up with 7 hsdirs */
/* 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, 7);
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 */
Expand Down

0 comments on commit b9f849b

Please sign in to comment.