Opened 3 years ago

Closed 3 years ago

#23989 closed defect (fixed)

entry_guards_update_all() will pretend to update primaries even if sampled set is empty

Reported by: asn Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: tor-guard, tor-bridge, tor-client, 031-backport
Cc: catalyst, isis, bmeson, mrphs Actual Points:
Parent ID: #21969 Points:
Reviewer: Sponsor:


entry_guards_update_all() is used to update all the various sets of the guard subsystem, and then make the list of primary guards.

The first list that needs to be made is the sampled set in sampled_guards_update_from_consensus(). However ,that function is a NOP if we are missing a live consensus.

The problem here is that entry_guards_update_all() will not notice that the sampled set was never initialized and will happily move forward into making the list of primary guards from a non-existent sampled set which will fail. It will also set gs->primary_guards_up_to_date and other parts of the subsystem will think that there is actually a primary guard list and will not initialize it (e.g. select_entry_guard_for_circuit()).

We should probably not allow the primary guard list etc. to be done if we failed to initialize our sampled set. Perhaps we could move the live_consensus_is_missing() check from sampled_guards_update_from_consensus() to entry_guards_update_all().

I don't think that this can cause serious issues because we will eventually regenerate our primary guard list when we finally fetch a live consensus.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by asn

Parent ID: #23862#21969

comment:2 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

I think this might be something we could fix just by documentation -- the primary_guards_up_to_date flag now means "are the primary guards up-to-date with respect to the consensus and status info that we have" -- not necessarily "are the primary guards usable". When primary_guards_up_to_date gets set with no consensus available, then calling update_primary_guards() again actually wouldn't be a good idea, since it wouldn't do anything.

comment:4 Changed 3 years ago by nickm

Keywords: 030-backport removed

Remove 030-backport from all open tickets that have it: 0.3.0 is now deprecated.

comment:5 Changed 3 years ago by asn

Status: acceptedneeds_review

Given the lack of consequences for this issue I think that a doc change is an acceptable fix here.

Please see the proposed doc change in my bug23989.

comment:6 Changed 3 years ago by nickm

lgtm; merged! No backport.

comment:7 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.