Opened 7 years ago

Closed 7 years ago

#8231 closed defect (fixed)

Clients can go crazy when unable to find directory guards

Reported by: nickm Owned by:
Priority: Very High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When every possible node seems down, clients appear to go crazy adding the same nodes as guards over and over. I am betting that this is due to the new directory guard code.

I have a fix for this (in need of review) in branch "no_dup_guards" in my public repository.

Found while testing different failure modes with Chutney.

Child Tickets

Change History (6)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

See branch "no_dup_guards" in my public repository.

comment:2 Changed 7 years ago by arma

+  if (entry_guard_get_by_id_digest(node->identity) != NULL) {
+    log_info(LD_CIRC, "I was about to add a duplicate entry guard.");
+    /* This can happen if we choose a guard, then the node goes away, then
+     * comes back. */
+    ((node_t*) node)->using_as_guard = 1;
+    return NULL;
+  }

don't we want to return the guard here, to show success?

comment:3 in reply to:  2 Changed 7 years ago by andrea

Replying to arma:

+  if (entry_guard_get_by_id_digest(node->identity) != NULL) {
+    log_info(LD_CIRC, "I was about to add a duplicate entry guard.");
+    /* This can happen if we choose a guard, then the node goes away, then
+     * comes back. */
+    ((node_t*) node)->using_as_guard = 1;
+    return NULL;
+  }

don't we want to return the guard here, to show success?

I think we want to return NULL, since it looks like the things that call this mainly use the return value to know if the guard list changed. The comment for the add_an_entry_guard() function could perhaps do a better job of explaining this.

comment:4 Changed 7 years ago by nickm

My rationale there was that it was better to terminate the loop in any outside function than to let it go on. But I guess we could let it go on here, since if we got the same node the next time, the using_as_guard flag would keep us from yielding it then?

comment:5 in reply to:  4 Changed 7 years ago by andrea

Replying to nickm:

My rationale there was that it was better to terminate the loop in any outside function than to let it go on. But I guess we could let it go on here, since if we got the same node the next time, the using_as_guard flag would keep us from yielding it then?

That sounds fine to me; merge away.

comment:6 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great; merged! Thank you!

Note: See TracTickets for help on using tickets.