Opened 3 years ago

Closed 3 years ago

#21142 closed defect (fixed)

prop271: circuits_pending_other_guards not properly maintained

Reported by: asn Owned by: asn
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: tor-guard
Cc: Actual Points:
Parent ID: Points: 0.3
Reviewer: nickm Sponsor:

Description (last modified by asn)


I've been doing various tests to prop271 by using it with tor browser.

I started digging more into circuit_find_circuits_to_upgrade_from_guard_wait() and particularly the circuits_pending_other_guards smartlist to understand better how this feature works in little-t-tor.

While inspecting the elements of circuits_pending_other_guards I noticed that some of those circuits were zombies that were already freed, probably because they were closed but not removed from the smartlist.

The only time we change membership of that list is in circuit_set_state():

 if (circ->state == CIRCUIT_STATE_GUARD_WAIT) {
    smartlist_remove(circuits_pending_other_guards, circ);
  if (state == CIRCUIT_STATE_GUARD_WAIT) {
    smartlist_add(circuits_pending_other_guards, circ);

We should probably consider removing circuits from that list when they marked for close as well, so that the list does not stay permanently populated. Also see how membership in the similar smartlist circuits_pending_chans is maintained.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by asn

Description: modified (diff)

comment:2 Changed 3 years ago by asn

For now I'm testing a simple fix of adding:

+  if (circuits_pending_other_guards) {
+    smartlist_remove(circuits_pending_other_guards, circ);
+  }

in circuit_about_to_free(). Seems to work so far.

comment:3 Changed 3 years ago by nickm

Priority: MediumHigh

comment:4 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:5 Changed 3 years ago by asn

Status: acceptedneeds_review

Please see my branch in bug21142.

comment:6 Changed 3 years ago by nickm

Owner: changed from nickm to asn
Reviewer: nickm
Status: needs_reviewassigned

Fixing owner (and thanks!)

comment:7 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:8 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

ack; merged; thank you!

Note: See TracTickets for help on using tickets.