Opened 7 months ago

Closed 5 months ago

#23862 closed defect (fixed)

Tor only updates guard state after a consensus if it has enough directory info

Reported by: teor Owned by: asn
Priority: Very High Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.1-alpha
Severity: Normal Keywords: tor-guard, tor-bridge, tor-client, 030-backport, 031-backport
Cc: catalyst, isis, bmeson, mrphs Actual Points:
Parent ID: #21969 Points: 1
Reviewer: Sponsor: SponsorV

Description

Steps to reproduce:

  1. Launch tor in a fresh data directory
    export DATADIR=`mktemp -d`
    src/or/tor DataDirectory "$DATADIR" StrictNodes 1 EntryNodes 1390DFDB5603AB5A16564505D7AE8647B1818A3C
    
  2. Delete its microdescriptors
    rm -v "$DATADIR"/cached-microdescs*
    
  3. Repeat step 1

Expected behaviour:

Tor uses the selected EntryNode

Actual Behaviour:

Tor uses a fallback directory mirror

This happens because tor should update guard state after every consensus, but it only updates guard state when it has enough directory info.

It is pathological when combined with #23817, when tor gets in a state when it doesn't have enough directory info because its guards are broken, and never updates its guard state, so it can't get enough directory info.

Child Tickets

Change History (20)

comment:1 Changed 7 months ago by teor

Status: newneeds_review

Please see my draft branch bug23862.
Someone else will need to write the changes file, and any unit tests, and maybe edit the commit message. (I don't have time.)

comment:2 Changed 7 months ago by teor

Version: Tor: 0.3.0.6Tor: 0.3.0.1-alpha

comment:3 Changed 7 months ago by nickm

Priority: MediumHigh

comment:4 Changed 7 months ago by nickm

Priority: HighVery High
Status: needs_reviewneeds_revision

This approach looks good, but we're going to need to backport onto maint-0.3.0 and do the other work teor notes above ("write the changes file, and any unit tests, and maybe edit the commit message").

comment:5 Changed 7 months ago by asn

Owner: set to asn
Status: needs_revisionassigned

I'll try to get to this tomorrow or the day after, due to its #21969 significance.

comment:6 Changed 7 months ago by nickm

Sponsor: SponsorV

comment:7 Changed 7 months ago by asn

Worked many hours today making a nice unittest here. Almost there but not quite there yet. Will try to have something here tomorrow.

comment:8 Changed 7 months ago by asn

Status: assignedneeds_review

Please see branch bug23862_v2 in my repo that features the following changes from teor's branch:

  • Rebased to latest master
  • Added unittest that reproduces the failure and makes sure that the patch fixes it.
  • Add changes file
  • Add some more info on the commit msgs.

The unittest was not simple to right at all due to the hairiness of the subsystems involved (mds and guards)!!! It took me about 6 hours to write and I encountered another bug while writing it (#23989). Let me know if you have any questions.

FWIW, I don't think this bug is a very important bug wrt #21969. It's quite edge-casey and it won't cause a total client disable like #21969. Onwards!

comment:9 Changed 7 months ago by nickm

So, dfd3ed5bdc252ad2c3afed922e5dcd955eff1d68 and 10c5eae3fa4f26027c6335cab607a5de550458db are the commits with the actual fix here -- those are the ones to backport if we decide to do so. Also the changes file from 1a6814c096b0fa6fbfd2a1688b29dbc8143d6568.

This needs to be based on maint-0.3.2 to merge it into 0.3.2 as planned.

comment:10 Changed 7 months ago by asn

Pushed bug23862_032 which is based on maint-0.3.2!

comment:11 Changed 7 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final
Status: needs_reviewmerge_ready

Thanks! I've merged that to maint-0.3.2, and am merging the the tests to master! Marking for possible backport in case this helps more than we expect.

comment:12 Changed 7 months ago by teor

Do we want to backport this to 0.3.0 as well?
This bug was introduced in 0.3.0.1-alpha.

comment:13 Changed 7 months ago by nickm

Yes, possibly. It's currently marked as 030-backport and 031-backport.

comment:14 Changed 6 months ago by nickm

I'm okay with backporting this if somebody prepares a branch.

comment:15 in reply to:  14 Changed 6 months ago by asn

Replying to nickm:

I'm okay with backporting this if somebody prepares a branch.

Hello. Pushed bug23862_031 and bug23862_030 for backports.

comment:16 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay; those are merged now too!

comment:17 Changed 5 months ago by teor

Oops, there is a typo in the name of the changes file, it does not pass make check-changes.

Please see my branch bug23862-changes-030.

comment:18 Changed 5 months ago by teor

Resolution: fixed
Status: closedreopened

comment:19 Changed 5 months ago by teor

Status: reopenedmerge_ready

comment:20 Changed 5 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

whoops; sorry -- found this independently, and fixed it to make jenkins happy.

Note: See TracTickets for help on using tickets.