[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #24392 [Core Tor/Tor]: Ignore cached bridge descriptors until we check if they are running
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs