[tor-bugs] #18929 [Core Tor/Tor]: Fix selection of directories with non-preferred address families

[Tor Bug Tracker & Wiki]

#18929: Fix selection of directories with non-preferred address families
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_review
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  must-fix-before-028-rc,              |        Version:  Tor:
  TorCoreTeam201605                              |  0.2.8.1-alpha
Parent ID:  #18483                               |     Resolution:
 Reviewer:  isis                                 |  Actual Points:  2 hours
                                                 |         Points:  small
                                                 |        Sponsor:
-------------------------------------------------+-------------------------

Comment (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.
    * 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.
    * 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?
    * 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.

 All other patches on this branch look fine to me.

 One last question:

    * 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. :)

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