Opened 11 years ago

Last modified 7 years ago

#763 closed defect (Fixed)

Inaccuracies in Rendezvous Descriptor Upload Logic

Reported by: karsten Owned by:
Priority: Low Milestone:
Component: Core Tor/Tor Version: 0.2.1.2-alpha
Severity: Keywords:
Cc: karsten, arma, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The current logic for publishing a rendezvous service descriptor is to wait
for 30 seconds after the last change to the set of introduction points.
However, in an evaluation it was found that in some test cases these
30 seconds were exceeded: In one test case, there was a 34 seconds gap
inbetween two introduction establishments, but without an attempt to upload
a descriptor. In another case, a descriptor was not uploaded until
41 seconds after an introduction point establishment.

An inspection of the log files revealed that the abandoning of an
introduction point *candidate* resets the 30-seconds counter, too, even
though it would have no effect on the uploaded descriptor. Hence, given
that the 30-seconds delay is a useful means to estimate when a descriptor
is stable, the 30-seconds counter should only be reset when an actual
change to the descriptor to be uploaded occurs.

A possible fix is to check whether an introduction point that is given up
was contained in the last published descriptor. Only in that case, the
descriptor should be marked as dirty and the 30-seconds countdown started.
A bugfix is attached below. Backport candidate for 0.2.0.x.

Index: /home/karsten/tor/tor-trunk/src/or/rendservice.c
===================================================================
--- /home/karsten/tor/tor-trunk/src/or/rendservice.c (revision 15806)
+++ /home/karsten/tor/tor-trunk/src/or/rendservice.c (working copy)
@@ -1260,10 +1260,20 @@

service->descriptor_version)) {

log_info(LD_REND,"Giving up on %s as intro point for %s.",

intro->extend_info->nickname, service->service_id);

+ if (service->desc) {
+ SMARTLIST_FOREACH(service->desc->intro_nodes, rend_intro_point_t *, dintro, {
+ if (!memcmp(dintro->extend_info->identity_digest,
+ intro->extend_info->identity_digest, DIGEST_LEN)) {
+ log_info(LD_REND, "The intro point we are giving up was included "
+ "in the last published descriptor. Marking "
+ "current descriptor as dirty.");
+ service->desc_is_dirty = now;
+ }
+ });
+ }

rend_intro_point_free(intro);
smartlist_del(service->intro_nodes,j--);
changed = 1;

  • service->desc_is_dirty = now;

}
smartlist_add(intro_routers, router);

}

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (4)

comment:1 Changed 11 years ago by arma

Looks good. Please add to 0.2.1.x.

I thought briefly about whether we should move the "changed = 1" up to
your 'if' block too. But I guess not. Right?

I also wonder if "smartlist_add(intro_routers, router);" should happen
even if router is null. That seems like a bad move, right? :)

As for backporting to 0.2.0.x, I don't think we need to do this. It isn't
a huge bug, and we're trying to leave 0.2.0.x alone at this point so we
can call it stable.

comment:2 Changed 11 years ago by karsten

Looks good. Please add to 0.2.1.x.

Added as r15825.

I thought briefly about whether we should move the "changed = 1" up to
your 'if' block too. But I guess not. Right?

No, it's still correct that the list of introduction points has 'changed'
locally. It just didn't have an effect on the list that has been published
with the last descriptor. If we would move 'changed = 1' to the 'if' block,
there might be some weird situations in which we gave up on an introduction
point but didn't pick a new one.

I also wonder if "smartlist_add(intro_routers, router);" should happen
even if router is null. That seems like a bad move, right? :)

Right, that looks strange. 'router' shouldn't be NULL though, and if it was,
it would be a bug. I separated the NULL check for 'router' and added a log
statement stating that such an event would be a bug.

As for backporting to 0.2.0.x, I don't think we need to do this. It isn't
a huge bug, and we're trying to leave 0.2.0.x alone at this point so we
can call it stable.

Yeah, my first thought was that this is easy to fix, but I agree that it
doesn't have an impact on most users. Alright, I'll leave it as it is.

comment:3 Changed 11 years ago by arma

flyspray2trac: bug closed.

comment:4 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.