Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#18929 closed defect (fixed)

Fix selection of directories with non-preferred address families

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: must-fix-before-028-rc, TorCoreTeam201605
Cc: Actual Points: 2 hours
Parent ID: #18483 Points: 1
Reviewer: isis,nickm Sponsor:

Description (last modified by teor)

In #17840 in 0.2.8.1-alpha, we sometimes fail to fall back to directories with addresses in non-preferred IP families:

  • we didn't identify relays that we could fall back to correctly;
  • we incorrectly assumed that every node would have an IPv4 address - this doesn't apply to bridges;
  • we counted relays when we had already fallen back to non-preferred addresses.

This likely affected bridge clients with IPv4 bridges, and clients in small networks.

Child Tickets

Change History (14)

comment:1 Changed 4 years ago by teor

See commit
e6f859c Make directory node selection for IPv4 bridge clients more reliable
in my branch feature18483 on ​https://github.com/teor2345/tor.git

comment:2 Changed 4 years ago by isis

Reviewer: isis
Status: newneeds_review

comment:3 Changed 4 years ago by isis

This ticket is just for fixing the counting problem, right?

comment:4 in reply to:  1 ; Changed 4 years ago by isis

Status: needs_reviewneeds_revision

Replying to teor:

See commit
e6f859c Make directory node selection for IPv4 bridge clients more reliable
in my branch feature18483 on ​https://github.com/teor2345/tor.git


router_has_non_preferred_address() will return true if the routerstatus_t has both an IPv4 and an IPv6 address… regardless of whether IPv4 or IPv6 is preferred. This might still mess up the n_non_preferred count?

If that's the right behaviour, then there's a unittest for you, for router_has_non_preferred_address(), in my bug18929 branch (based on your feature18483). Otherwise we should make the unittest do the intended thing and then revise.

comment:5 in reply to:  4 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Replying to isis:

Replying to teor:

See commit
e6f859c Make directory node selection for IPv4 bridge clients more reliable
in my branch feature18483 on ​https://github.com/teor2345/tor.git


router_has_non_preferred_address() will return true if the routerstatus_t has both an IPv4 and an IPv6 address… regardless of whether IPv4 or IPv6 is preferred. This might still mess up the n_non_preferred count?

That's the intended behaviour - we want to know if there's any point in trying the non-preferred IP family. So we count the number of relays we might choose if we don't get any reachable relays from our preferred IP family.
To do that, we look for a valid address from the non-preferred family.
If it has an IPv4 and IPv6 address, it definitely has an address from the non-preferred family.

But there's a bug, it's not checking node-specific preferences, which matter for bridge clients:
f48abb6 Use bridge client node-specific IPv6 preferences where available

If that's the right behaviour, then there's a unittest for you, for router_has_non_preferred_address(), in my bug18929 branch (based on your feature18483). Otherwise we should make the unittest do the intended thing and then revise.

Thanks for the unit test.

Cherry picked your bug18929 branch as:
2695ace Add unittest for router_has_non_preferred_address().

I changed the function name in f48abb6, so I needed to change it in the unit test as well:
c198259 Tweak unit test so it tests router_has_non_preferred_address_rs

That should be it!

comment:6 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:7 Changed 4 years ago by teor

I split #18483, #18921, and #18929 into three separate branches for 0.2.8, and a branch for 0.2.9.
Please see bug18929-v2 on https://github.com/teor2345/tor.git for this issue. It has no dependencies.

comment:8 Changed 4 years ago by bugzilla

Keywords: CoreTorTeam201605 removed

comment:9 Changed 4 years ago by nickm

okay, trying this again!

First, I'm reviewing the first two commits (aaa6ac0a1a94f2cf058c3bb45879c1919889f6df and cbb834587b8eed1148125a382891c7f6003ebf60) as one, since they're supposed to get squashed eventually.

  • NM1 This is cosmetic, but I don't understand why router_has_non_preferred_address takes prefer_ipv6_or as an argument, but derives perfer_ipv6_dir for iteslf.
  • NM2 For router_has_non_preferred_address_node -- an unlisted bridge has a node_t, but does not have a routerstatus. Can this function ever get called on a bridge? Do we care?
  • NM3 Maybe you now handle this in #18483 or someplace else -- I haven't reviewed that yet -- but I'm a little confused about how router_has_non_preferred_address* looks at both the ORPort and the DirPort, but it doesn't actually take into account whether we're picking a directory for a connection where we would absolutely not use the ORPort or absolutely not use the DirPort. [Edit: I think that must_have_or is probably meant to take care of this?]

All other patches on this branch look fine to me.

One last question:

  • (NM4) Would it be even simpler to just omit most of this patch, remove n_not_preferred, and just do this?
     #define RETRY_ALTERNATE_IP_VERSION(retry_label)                               \
       STMT_BEGIN                                                                  \
         if (result == NULL && try_ip_pref && options->ClientUseIPv4               \
             && fascist_firewall_use_ipv6(options) && !server_mode(options)        \
    -        && n_not_preferred && !n_busy) {                                      \
    +        && !n_busy) {                                                         \
           n_excluded = 0;                                                         \
           n_busy = 0;                                                             \
           try_ip_pref = 0;                                                        \
    -      n_not_preferred = 0;                                                    \
           goto retry_label;                                                       \
         }                                                                         \
       STMT_END                                                                    \
    

Sure, we would sometimes make two passes needlessly, but not in a common case. What do you think?

Now I'm going to get more tea and read #18483; we'll see how much of this I strikethrough once I'm done. :)

Last edited 4 years ago by nickm (previous) (diff)

comment:10 Changed 4 years ago by nickm

Okay, on reading #18483 I see some of my questions answered, but I'd still like to know about the other ones above. Also, please make sure I was right to strikethrough the strikethroughed item.

comment:11 Changed 4 years ago by nickm

Reviewer: isisisis,nickm

comment:12 Changed 4 years ago by teor

Description: modified (diff)

Wow, you're right, let's do it the simple way - delete the bad check:
bug18929-v3

(I thought I was saving client speed at the cost of complexity. For most clients, this would have imposed more checks on every directory for no benefit, and for clients where it mattered, I didn't have actual performance figures.)

I will need to rebuild a simpler #18483 branch now there's no dependency between them.

For the record:

NM2 For router_has_non_preferred_address_node -- an unlisted bridge has a node_t, but does not have a routerstatus. Can this function ever get called on a bridge? Do we care?

This function never gets called on a bridge, as directory_get_from_dirserver handles bridges.
So this bug never affected bridges or bridge clients, sorry, isis!

comment:13 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging!

comment:14 Changed 3 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.