[tor-bugs] #24392 [Core Tor/Tor]: Ignore cached bridge descriptors until we check if they are running

[Tor Bug Tracker & Wiki]

#24392: Ignore cached bridge descriptors until we check if they are running
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  teor
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.2.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.0.1-alpha
 Severity:  Normal                               |     Resolution:
 Keywords:  030-backport, 031-backport,          |  Actual Points:  0.5
  regression, tor-bridge-client, s8-errors,      |
  review-group-27                                |
Parent ID:  #24367                               |         Points:  0.3
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:18 nickm]:
 > Hi! I have some questions, that are not necessarily blockers.
 >
 > In the new commit bca43d548ad7301fc3698e8ded0a94d43a41858d:
 > {{{
 > -    int first = num_bridges_usable() <= 1;
 > +    /* Retry directory downloads whenever we get a bridge descriptor:
 > +     * - when bootstrapping, and
 > +     * - when we aren't sure if any of our bridges are reachable.
 > +     * Keep on retrying until we have at least one reachable bridge. */
 > +    int first = num_bridges_usable(0) < 1;
 > }}}
 >
 > Do we know why this was `<=` before?  It looks like I introduced the
 code in bca43d548ad7301fc3698e8ded0a94d43a41858d, but I don't understand
 what my original rationale was, so I'm a little uncertain here.  I'll try
 to see if I can puzzle this out.

 I suspect that it was `num_bridges_usable() <= 1` because in the old code
 that meant:
 "keep on trying until we have more than one bridge that is maybe or
 definitely reachable".
 This isn't quite right, because two maybe-reachable bridges can still be
 useless to a client.

 But now `num_bridges_usable(0) < 1` means:
 "keep on trying until we have one bridge that is definitely reachable".
 I think that's what we actually want, and I added the comment for future
 reference.

 > Second question: In e9cb0cd7c8fe03dbd7e0fef91ab868cb56f280b0, when we do
 > {{{
 > -      } else if (!options->UseBridges || num_bridges_usable() > 0) {
 > +      } else {
 > }}}
 > can we add a `tor_nonfatal_assert()` there to make sure that the second
 case really and truly only happens when we think it does?

 Yes, I'll do this in the morning.

 > Last:
 > > I am no longer convinced that this bug is restricted to 0.3.2, we
 should consider backporting these changes to 0.3.0 as a precautionary
 measure.
 >
 > This is a pretty complex set of changes, and the rebase isn't clean.  Is
 there a minimal set of changes that, we could put into a branch based on
 0.3.0?

 I'll look into this in the morning.
 I think we only need the `int first = num_bridges_usable(0) < 1;` change.

 Because `} else if (!options->UseBridges || num_bridges_usable() > 0) {`
 is redundant, and the final change is on code that didn't exist before
 0.3.2.

 > And final question: how is the testing on this?

 It passes all of `make test-network-all`, and gk likes it better than any
 of the previous versions when testing bridge switches in Tor Browser. But
 there's still a bug in #23524 where an old cached bridge descriptor /
 state file entry that is no longer in the torrc still forms part of the
 set of guards.

 I was hoping someone else would have a look at that one, I'm not that
 familiar with the new guard code.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24392#comment:19>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online