Choose directory servers by IPv4/IPv6 preferences

Add unit tests, refactor pick_directory functions.

src/or/connection.c

@@ -4252,10 +4252,10 @@ connection_write_to_buf_impl_,(const char *string, size_t len,
 /** Return a connection with given type, address, port, and purpose;
  * or NULL if no such connection exists (or if all such connections are marked
  * for close). */
-connection_t *
-connection_get_by_type_addr_port_purpose(int type,
+MOCK_IMPL(connection_t *,
+connection_get_by_type_addr_port_purpose,(int type,
                                          const tor_addr_t *addr, uint16_t port,
-                                         int purpose)
+                                         int purpose))
 {
   CONN_GET_TEMPLATE(conn,
        (conn->type == type &&

src/or/connection.h

@@ -186,9 +186,9 @@ connection_get_outbuf_len(connection_t *conn)
 connection_t *connection_get_by_global_id(uint64_t id);
 
 connection_t *connection_get_by_type(int type);
-connection_t *connection_get_by_type_addr_port_purpose(int type,
-                                                   const tor_addr_t *addr,
-                                                   uint16_t port, int purpose);
+MOCK_DECL(connection_t *,connection_get_by_type_addr_port_purpose,(int type,
+                                                  const tor_addr_t *addr,
+                                                  uint16_t port, int purpose));
 connection_t *connection_get_by_type_state(int type, int state);
 connection_t *connection_get_by_type_state_rendquery(int type, int state,
                                                      const char *rendquery);

src/or/directory.c

@@ -82,18 +82,18 @@ static void dir_microdesc_download_failed(smartlist_t *failed,
 static void note_client_request(int purpose, int compressed, size_t bytes);
 static int client_likes_consensus(networkstatus_t *v, const char *want_url);
 
-static void directory_initiate_command_rend(const tor_addr_t *addr,
-                                            uint16_t or_port,
-                                            uint16_t dir_port,
-                                            const char *digest,
-                                            uint8_t dir_purpose,
-                                            uint8_t router_purpose,
-                                            dir_indirection_t indirection,
-                                            const char *resource,
-                                            const char *payload,
-                                            size_t payload_len,
-                                            time_t if_modified_since,
-                                            const rend_data_t *rend_query);
+static void directory_initiate_command_rend(
+                                          const tor_addr_port_t *or_addr_port,
+                                          const tor_addr_port_t *dir_addr_port,
+                                          const char *digest,
+                                          uint8_t dir_purpose,
+                                          uint8_t router_purpose,
+                                          dir_indirection_t indirection,
+                                          const char *resource,
+                                          const char *payload,
+                                          size_t payload_len,
+                                          time_t if_modified_since,
+                                          const rend_data_t *rend_query);
 
 /********* START VARIABLES **********/
 
@@ -624,8 +624,10 @@ directory_initiate_command_routerstatus_rend(const routerstatus_t *status,
 {
   const or_options_t *options = get_options();
   const node_t *node;
-  tor_addr_t addr;
+  tor_addr_port_t use_or_ap, use_dir_ap;
   const int anonymized_connection = dirind_is_anon(indirection);
+  int have_or = 0, have_dir = 0;
+
   node = node_get_by_id(status->identity_digest);
 
   if (!node && anonymized_connection) {
@@ -634,7 +636,6 @@ directory_initiate_command_routerstatus_rend(const routerstatus_t *status,
              routerstatus_describe(status));
     return;
   }
-  tor_addr_from_ipv4h(&addr, status->addr);
 
   if (options->ExcludeNodes && options->StrictNodes &&
       routerset_contains_routerstatus(options->ExcludeNodes, status, -1)) {
@@ -646,13 +647,68 @@ directory_initiate_command_routerstatus_rend(const routerstatus_t *status,
     return;
   }
 
-  directory_initiate_command_rend(&addr,
-                             status->or_port, status->dir_port,
-                             status->identity_digest,
-                             dir_purpose, router_purpose,
-                             indirection, resource,
-                             payload, payload_len, if_modified_since,
-                             rend_query);
+  /* At this point, if we are a clients making a direct connection to a
+   * directory server, we have selected a server that has at least one address
+   * allowed by ClientUseIPv4/6 and Reachable{"",OR,Dir}Addresses. This
+   * selection uses the preference in ClientPreferIPv6{OR,Dir}Port, if
+   * possible. (If UseBridges is set, clients ignore all these settings.)
+   *
+   * Now we use a similar process to select an address for the relay,
+   * but simply use the other address if the one we want isn't allowed by
+   * the firewall.
+   *
+   * (When Tor uploads and downloads a hidden service descriptor, it uses
+   * DIRIND_ANONYMOUS, except for Tor2Web, which uses DIRIND_ONEHOP.
+   * So this code will only modify the address for Tor2Web's HS descriptor
+   * fetches. Even Single Onion Servers (NYI) use DIRIND_ANONYMOUS, to avoid
+   * HSDirs denying service by rejecting descriptors.)
+   */
+
+  /* Initialise the OR / Dir addresses */
+  tor_addr_make_null(&use_or_ap.addr, AF_UNSPEC);
+  use_or_ap.port = 0;
+  tor_addr_make_null(&use_dir_ap.addr, AF_UNSPEC);
+  use_dir_ap.port = 0;
+
+  if (anonymized_connection) {
+    /* Use the primary (IPv4) OR address if we're making an indirect
+     * connection. */
+    tor_addr_from_ipv4h(&use_or_ap.addr, status->addr);
+    use_or_ap.port = status->or_port;
+    have_or = 1;
+  } else {
+    /* We use an IPv6 address if we have one and we prefer it.
+     * Use the preferred address and port if they are reachable, otherwise,
+     * use the alternate address and port (if any).
+     */
+    have_or = fascist_firewall_choose_address_rs(status,
+                                                 FIREWALL_OR_CONNECTION, 0,
+                                                 &use_or_ap);
+  }
+
+  have_dir = fascist_firewall_choose_address_rs(status,
+                                                FIREWALL_DIR_CONNECTION, 0,
+                                                &use_dir_ap);
+
+  /* We rejected both addresses. This isn't great. */
+  if (!have_or && !have_dir) {
+    log_info(LD_DIR, "Rejected both the OR and Dir address when launching a "
+             "directory connection to: IPv4 %s OR %d Dir %d IPv6 %s OR %d "
+             "Dir %d", fmt_addr32(status->addr), status->or_port,
+             status->dir_port, fmt_addr(&status->ipv6_addr),
+             status->ipv6_orport, status->dir_port);
+    return;
+  }
+
+  /* XX/teor - we don't retry the alternate OR/Dir address if this one fails.
+   * (See #6772.) Instead, we'll retry another directory on failure. */
+
+  directory_initiate_command_rend(&use_or_ap, &use_dir_ap,
+                                  status->identity_digest,
+                                  dir_purpose, router_purpose,
+                                  indirection, resource,
+                                  payload, payload_len, if_modified_since,
+                                  rend_query);
 }
 
 /** Launch a new connection to the directory server <b>status</b> to
@@ -874,17 +930,21 @@ directory_command_should_use_begindir(const or_options_t *options,
   if (indirection == DIRIND_DIRECT_CONN || indirection == DIRIND_ANON_DIRPORT)
     return 0;
   if (indirection == DIRIND_ONEHOP)
-    if (!fascist_firewall_allows_address_or(addr, or_port) ||
+    if (!fascist_firewall_allows_address_addr(addr, or_port,
+                                              FIREWALL_OR_CONNECTION, 0) ||
         directory_fetches_from_authorities(options))
       return 0; /* We're firewalled or are acting like a relay -- also no. */
   return 1;
 }
 
-/** Helper for directory_initiate_command_routerstatus: send the
- * command to a server whose address is <b>address</b>, whose IP is
- * <b>addr</b>, whose directory port is <b>dir_port</b>, whose tor version
- * <b>supports_begindir</b>, and whose identity key digest is
- * <b>digest</b>. */
+/** Helper for directory_initiate_command_rend: send the
+ * command to a server whose address is <b>_addr</b>, whose OR port is
+ * <b>or_port</b>, whose directory port is <b>dir_port</b>, whose identity key
+ * digest is <b>digest</b>, with purposes <b>dir_purpose</b> and
+ * <b>router_purpose</b>, making an (in)direct connection as specified in
+ * <b>indirection</b>, with command <b>resource</b>, <b>payload</b> of
+ * <b>payload_len</b>, and asking for a result only <b>if_modified_since</b>.
+ */
 void
 directory_initiate_command(const tor_addr_t *_addr,
                            uint16_t or_port, uint16_t dir_port,
@@ -894,7 +954,27 @@ directory_initiate_command(const tor_addr_t *_addr,
                            const char *payload, size_t payload_len,
                            time_t if_modified_since)
 {
-  directory_initiate_command_rend(_addr, or_port, dir_port,
+  /* Assume _addr applies to both the ORPort and the DirPort, but use the
+   * null tor_addr if ORPort or DirPort are 0. */
+  tor_addr_port_t or_ap, dir_ap;
+
+  if (or_port) {
+    tor_addr_copy(&or_ap.addr, _addr);
+  } else {
+    /* the family doesn't matter here, so make it the same as _addr */
+    tor_addr_make_null(&or_ap.addr, tor_addr_family(_addr));
+  }
+  or_ap.port = or_port;
+
+  if (dir_port) {
+    tor_addr_copy(&dir_ap.addr, _addr);
+  } else {
+    /* the family doesn't matter here, so make it the same as _addr */
+    tor_addr_make_null(&dir_ap.addr, tor_addr_family(_addr));
+  }
+  dir_ap.port = dir_port;
+
+  directory_initiate_command_rend(&or_ap, &dir_ap,
                              digest, dir_purpose,
                              router_purpose, indirection,
                              resource, payload, payload_len,
@@ -914,10 +994,11 @@ is_sensitive_dir_purpose(uint8_t dir_purpose)
 }
 
 /** Same as directory_initiate_command(), but accepts rendezvous data to
- * fetch a hidden service descriptor. */
+ * fetch a hidden service descriptor, and takes its address & port arguments
+ * as tor_addr_port_t. */
 static void
-directory_initiate_command_rend(const tor_addr_t *_addr,
-                                uint16_t or_port, uint16_t dir_port,
+directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
+                                const tor_addr_port_t *dir_addr_port,
                                 const char *digest,
                                 uint8_t dir_purpose, uint8_t router_purpose,
                                 dir_indirection_t indirection,
@@ -926,19 +1007,24 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
                                 time_t if_modified_since,
                                 const rend_data_t *rend_query)
 {
+  tor_assert(or_addr_port);
+  tor_assert(dir_addr_port);
+  tor_assert(or_addr_port->port || dir_addr_port->port);
+  tor_assert(digest);
+
   dir_connection_t *conn;
   const or_options_t *options = get_options();
   int socket_error = 0;
-  int use_begindir = directory_command_should_use_begindir(options, _addr,
-                                     or_port, router_purpose, indirection);
+  const int use_begindir = directory_command_should_use_begindir(options,
+                                     &or_addr_port->addr, or_addr_port->port,
+                                     router_purpose, indirection);
   const int anonymized_connection = dirind_is_anon(indirection);
+  const int or_connection = use_begindir || anonymized_connection;
+
   tor_addr_t addr;
-
-  tor_assert(_addr);
-  tor_assert(or_port || dir_port);
-  tor_assert(digest);
-
-  tor_addr_copy(&addr, _addr);
+  tor_addr_copy(&addr, &(or_connection ? or_addr_port : dir_addr_port)->addr);
+  uint16_t port = (or_connection ? or_addr_port : dir_addr_port)->port;
+  uint16_t dir_port = dir_addr_port->port;
 
   log_debug(LD_DIR, "anonymized %d, use_begindir %d.",
             anonymized_connection, use_begindir);
@@ -954,13 +1040,23 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
 
   /* ensure that we don't make direct connections when a SOCKS server is
    * configured. */
-  if (!anonymized_connection && !use_begindir && !options->HTTPProxy &&
+  if (!or_connection && !options->HTTPProxy &&
       (options->Socks4Proxy || options->Socks5Proxy)) {
     log_warn(LD_DIR, "Cannot connect to a directory server through a "
                      "SOCKS proxy!");
     return;
   }
 
+  if (or_connection && (!or_addr_port->port
+                        || tor_addr_is_null(&or_addr_port->addr))) {
+    log_warn(LD_DIR, "Cannot make an OR connection without an OR port.");
+    return;
+  } else if (!or_connection && (!dir_addr_port->port
+                                || tor_addr_is_null(&dir_addr_port->addr))) {
+    log_warn(LD_DIR, "Cannot make a Dir connection without a Dir port.");
+    return;
+  }
+
   /* ensure we don't make excess connections when we're already downloading
    * a consensus during bootstrap */
   if (connection_dir_avoid_extra_connection_for_purpose(dir_purpose)) {
@@ -971,7 +1067,7 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
 
   /* set up conn so it's got all the data we need to remember */
   tor_addr_copy(&conn->base_.addr, &addr);
-  conn->base_.port = use_begindir ? or_port : dir_port;
+  conn->base_.port = port;
   conn->base_.address = tor_dup_addr(&addr);
   memcpy(conn->identity_digest, digest, DIGEST_LEN);
 
@@ -989,7 +1085,7 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
   if (rend_query)
     conn->rend_data = rend_data_dup(rend_query);
 
-  if (!anonymized_connection && !use_begindir) {
+  if (!or_connection) {
     /* then we want to connect to dirport directly */
 
     if (options->HTTPProxy) {

src/or/routerlist.c

@@ -1462,9 +1462,122 @@ router_pick_dirserver_generic(smartlist_t *sourcelist,
   return router_pick_trusteddirserver_impl(sourcelist, type, flags, NULL);
 }
 
+/* Check if we already have a directory fetch from ap, for serverdesc
+ * (including extrainfo) or microdesc documents.
+ * If so, return 1, if not, return 0.
+ * Also returns 0 if addr is NULL, tor_addr_is_null(addr), or dir_port is 0.
+ */
+STATIC int
+router_is_already_dir_fetching(const tor_addr_port_t *ap, int serverdesc,
+                               int microdesc)
+{
+  if (!ap || tor_addr_is_null(&ap->addr) || !ap->port) {
+    return 0;
+  }
+
+  /* XX/teor - we're not checking tunnel connections here, see #17848
+   */
+  if (serverdesc && (
+     connection_get_by_type_addr_port_purpose(
+       CONN_TYPE_DIR, &ap->addr, ap->port, DIR_PURPOSE_FETCH_SERVERDESC)
+  || connection_get_by_type_addr_port_purpose(
+       CONN_TYPE_DIR, &ap->addr, ap->port, DIR_PURPOSE_FETCH_EXTRAINFO))) {
+    return 1;
+  }
+
+  if (microdesc && (
+     connection_get_by_type_addr_port_purpose(
+       CONN_TYPE_DIR, &ap->addr, ap->port, DIR_PURPOSE_FETCH_MICRODESC))) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* Check if we already have a directory fetch from ds, for serverdesc
+ * (including extrainfo) or microdesc documents.
+ * If so, return 1, if not, return 0.
+ */
+static int
+router_is_already_dir_fetching_ds(const dir_server_t *ds,
+                                  int serverdesc,
+                                  int microdesc)
+{
+  tor_addr_port_t ipv4_dir_ap, ipv6_dir_ap;
+
+  /* Assume IPv6 DirPort is the same as IPv4 DirPort */
+  tor_addr_from_ipv4h(&ipv4_dir_ap.addr, ds->addr);
+  ipv4_dir_ap.port = ds->dir_port;
+  tor_addr_copy(&ipv6_dir_ap.addr, &ds->ipv6_addr);
+  ipv6_dir_ap.port = ds->dir_port;
+
+  return (router_is_already_dir_fetching(&ipv4_dir_ap, serverdesc, microdesc)
+       || router_is_already_dir_fetching(&ipv6_dir_ap, serverdesc, microdesc));
+}
+
+/* Check if we already have a directory fetch from rs, for serverdesc
+ * (including extrainfo) or microdesc documents.
+ * If so, return 1, if not, return 0.
+ */
+static int
+router_is_already_dir_fetching_rs(const routerstatus_t *rs,
+                                  int serverdesc,
+                                  int microdesc)
+{
+  tor_addr_port_t ipv4_dir_ap, ipv6_dir_ap;
+
+  /* Assume IPv6 DirPort is the same as IPv4 DirPort */
+  tor_addr_from_ipv4h(&ipv4_dir_ap.addr, rs->addr);
+  ipv4_dir_ap.port = rs->dir_port;
+  tor_addr_copy(&ipv6_dir_ap.addr, &rs->ipv6_addr);
+  ipv6_dir_ap.port = rs->dir_port;
+
+  return (router_is_already_dir_fetching(&ipv4_dir_ap, serverdesc, microdesc)
+       || router_is_already_dir_fetching(&ipv6_dir_ap, serverdesc, microdesc));
+}
+
 /** How long do we avoid using a directory server after it's given us a 503? */
 #define DIR_503_TIMEOUT (60*60)
 
+/* Common retry code for router_pick_directory_server_impl and
+ * router_pick_trusteddirserver_impl. Retry with the non-preferred IP version.
+ * Must be called before RETRY_WITHOUT_EXCLUDE().
+ *
+ * If we got no result, and we are applying IP preferences, and we are a
+ * client that could use an alternate IP version, try again with the
+ * opposite preferences. */
+#define RETRY_ALTERNATE_IP_VERSION(retry_label)                               \
+  STMT_BEGIN                                                                  \
+    if (result == NULL && try_ip_pref && options->ClientUseIPv4               \
+        && options->ClientUseIPv6 && !server_mode(options) && n_not_preferred \
+        && !n_busy) {                                                         \
+      n_excluded = 0;                                                         \
+      n_busy = 0;                                                             \
+      try_ip_pref = 0;                                                        \
+      n_not_preferred = 0;                                                    \
+      goto retry_label;                                                       \
+    }                                                                         \
+  STMT_END                                                                    \
+
+/* Common retry code for router_pick_directory_server_impl and
+ * router_pick_trusteddirserver_impl. Retry without excluding nodes, but with
+ * the preferred IP version. Must be called after RETRY_ALTERNATE_IP_VERSION().
+ *
+ * If we got no result, and we are excluding nodes, and StrictNodes is
+ * not set, try again without excluding nodes. */
+#define RETRY_WITHOUT_EXCLUDE(retry_label)                                    \
+  STMT_BEGIN                                                                  \
+    if (result == NULL && try_excluding && !options->StrictNodes              \
+        && n_excluded && !n_busy) {                                           \
+      try_excluding = 0;                                                      \
+      n_excluded = 0;                                                         \
+      n_busy = 0;                                                             \
+      try_ip_pref = 1;                                                        \
+      n_not_preferred = 0;                                                    \
+      goto retry_label;                                                       \
+    }                                                                         \
+  STMT_END
+
 /** Pick a random running valid directory server/mirror from our
  * routerlist.  Arguments are as for router_pick_directory_server(), except:
  *
@@ -1489,11 +1602,12 @@ router_pick_directory_server_impl(dirinfo_type_t type, int flags,
   const int no_microdesc_fetching = (flags & PDS_NO_EXISTING_MICRODESC_FETCH);
   const int for_guard = (flags & PDS_FOR_GUARD);
   int try_excluding = 1, n_excluded = 0, n_busy = 0;
+  int try_ip_pref = 1, n_not_preferred = 0;
 
   if (!consensus)
     return NULL;
 
- retry_without_exclude:
+ retry_search:
 
   direct = smartlist_new();
   tunnel = smartlist_new();
@@ -1506,7 +1620,6 @@ router_pick_directory_server_impl(dirinfo_type_t type, int flags,
   SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), const node_t *, node) {
     int is_trusted, is_trusted_extrainfo;
     int is_overloaded;
-    tor_addr_t addr;
     const routerstatus_t *status = node->rs;
     const country_t country = node->country;
     if (!status)
@@ -1537,36 +1650,32 @@ router_pick_directory_server_impl(dirinfo_type_t type, int flags,
       continue;
     }
 
-    /* XXXX IP6 proposal 118 */
-    tor_addr_from_ipv4h(&addr, status->addr);
-
-    if (no_serverdesc_fetching && (
-       connection_get_by_type_addr_port_purpose(
-         CONN_TYPE_DIR, &addr, status->dir_port, DIR_PURPOSE_FETCH_SERVERDESC)
-    || connection_get_by_type_addr_port_purpose(
-         CONN_TYPE_DIR, &addr, status->dir_port, DIR_PURPOSE_FETCH_EXTRAINFO)
-    )) {
-      ++n_busy;
-      continue;
-    }
-
-    if (no_microdesc_fetching && connection_get_by_type_addr_port_purpose(
-      CONN_TYPE_DIR, &addr, status->dir_port, DIR_PURPOSE_FETCH_MICRODESC)
-    ) {
+    if (router_is_already_dir_fetching_rs(status,
+                                          no_serverdesc_fetching,
+                                          no_microdesc_fetching)) {
       ++n_busy;
       continue;
     }
 
     is_overloaded = status->last_dir_503_at + DIR_503_TIMEOUT > now;
 
-    if ((!fascistfirewall ||
-         fascist_firewall_allows_address_or(&addr, status->or_port)))
+    /* We use an IPv6 address if we have one and we prefer it.
+     * Add the router if its preferred address and port are reachable.
+     * If we don't get any routers, we'll try again with the non-preferred
+     * address for each router (if any). (To ensure correct load-balancing
+     * we try routers that only have one address both times.)
+     */
+    if (!fascistfirewall ||
+        fascist_firewall_allows_rs(status, FIREWALL_OR_CONNECTION,
+                                   try_ip_pref))
       smartlist_add(is_trusted ? trusted_tunnel :
                     is_overloaded ? overloaded_tunnel : tunnel, (void*)node);
-    else if (!fascistfirewall ||
-             fascist_firewall_allows_address_dir(&addr, status->dir_port))
+    else if (fascist_firewall_allows_rs(status, FIREWALL_DIR_CONNECTION,
+                                        try_ip_pref))
       smartlist_add(is_trusted ? trusted_direct :
                     is_overloaded ? overloaded_direct : direct, (void*)node);
+    else if (!tor_addr_is_null(&status->ipv6_addr))
+      ++n_not_preferred;
   } SMARTLIST_FOREACH_END(node);
 
   if (smartlist_len(tunnel)) {
@@ -1595,15 +1704,9 @@ router_pick_directory_server_impl(dirinfo_type_t type, int flags,
   smartlist_free(overloaded_direct);
   smartlist_free(overloaded_tunnel);
 
-  if (result == NULL && try_excluding && !options->StrictNodes && n_excluded
-      && !n_busy) {
-    /* If we got no result, and we are excluding nodes, and StrictNodes is
-     * not set, try again without excluding nodes. */
-    try_excluding = 0;
-    n_excluded = 0;
-    n_busy = 0;
-    goto retry_without_exclude;
-  }
+  RETRY_ALTERNATE_IP_VERSION(retry_search);
+
+  RETRY_WITHOUT_EXCLUDE(retry_search);
 
   if (n_busy_out)
     *n_busy_out = n_busy;
@@ -1658,11 +1761,12 @@ router_pick_trusteddirserver_impl(const smartlist_t *sourcelist,
   smartlist_t *pick_from;
   int n_busy = 0;
   int try_excluding = 1, n_excluded = 0;
+  int try_ip_pref = 1, n_not_preferred = 0;
 
   if (!sourcelist)
     return NULL;
 
- retry_without_exclude:
+ retry_search:
 
   direct = smartlist_new();
   tunnel = smartlist_new();
@@ -1673,7 +1777,6 @@ router_pick_trusteddirserver_impl(const smartlist_t *sourcelist,
     {
       int is_overloaded =
           d->fake_status.last_dir_503_at + DIR_503_TIMEOUT > now;
-      tor_addr_t addr;
       if (!d->is_running) continue;
       if ((type & d->type) == 0)
         continue;
@@ -1689,35 +1792,27 @@ router_pick_trusteddirserver_impl(const smartlist_t *sourcelist,
         continue;
       }
 
-      /* XXXX IP6 proposal 118 */
-      tor_addr_from_ipv4h(&addr, d->addr);
-
-      if (no_serverdesc_fetching) {
-        if (connection_get_by_type_addr_port_purpose(
-            CONN_TYPE_DIR, &addr, d->dir_port, DIR_PURPOSE_FETCH_SERVERDESC)
-         || connection_get_by_type_addr_port_purpose(
-             CONN_TYPE_DIR, &addr, d->dir_port, DIR_PURPOSE_FETCH_EXTRAINFO)) {
-          //log_debug(LD_DIR, "We have an existing connection to fetch "
-          //           "descriptor from %s; delaying",d->description);
-          ++n_busy;
-          continue;
-        }
-      }
-      if (no_microdesc_fetching) {
-        if (connection_get_by_type_addr_port_purpose(
-             CONN_TYPE_DIR, &addr, d->dir_port, DIR_PURPOSE_FETCH_MICRODESC)) {
-          ++n_busy;
-          continue;
-        }
+      if (router_is_already_dir_fetching_ds(d, no_serverdesc_fetching,
+                                            no_microdesc_fetching)) {
+        ++n_busy;
+        continue;
       }
 
-      if (d->or_port &&
-          (!fascistfirewall ||
-           fascist_firewall_allows_address_or(&addr, d->or_port)))
+      /* We use an IPv6 address if we have one and we prefer it.
+       * Add the router if its preferred address and port are reachable.
+       * If we don't get any routers, we'll try again with the non-preferred
+       * address for each router (if any). (To ensure correct load-balancing
+       * we try routers that only have one address both times.)
+       */
+      if (!fascistfirewall ||
+          fascist_firewall_allows_dir_server(d, FIREWALL_OR_CONNECTION,
+                                             try_ip_pref))
         smartlist_add(is_overloaded ? overloaded_tunnel : tunnel, (void*)d);
-      else if (!fascistfirewall ||
-               fascist_firewall_allows_address_dir(&addr, d->dir_port))
+      else if (fascist_firewall_allows_dir_server(d, FIREWALL_DIR_CONNECTION,
+                                                  try_ip_pref))
         smartlist_add(is_overloaded ? overloaded_direct : direct, (void*)d);
+      else if (!tor_addr_is_null(&d->ipv6_addr))
+        ++n_not_preferred;
     }
   SMARTLIST_FOREACH_END(d);
 
@@ -1744,19 +1839,12 @@ router_pick_trusteddirserver_impl(const smartlist_t *sourcelist,
   smartlist_free(overloaded_direct);
   smartlist_free(overloaded_tunnel);
 
-  if (result == NULL && try_excluding && !options->StrictNodes && n_excluded
-      && !n_busy) {
-    /* If we got no result, and we are excluding nodes, and StrictNodes is
-     * not set, try again without excluding nodes. */
-    try_excluding = 0;
-    n_excluded = 0;
-    n_busy = 0;
-    goto retry_without_exclude;
-  }
+  RETRY_ALTERNATE_IP_VERSION(retry_search);
+
+  RETRY_WITHOUT_EXCLUDE(retry_search);
 
   if (n_busy_out)
     *n_busy_out = n_busy;
-
   return result;
 }
 

src/or/routerlist.h

@@ -243,6 +243,8 @@ MOCK_DECL(STATIC was_router_added_t, extrainfo_insert,
 MOCK_DECL(STATIC void, initiate_descriptor_downloads,
           (const routerstatus_t *source, int purpose, smartlist_t *digests,
            int lo, int hi, int pds_flags));
+STATIC int router_is_already_dir_fetching(const tor_addr_port_t *ap,
+                                          int serverdesc, int microdesc);
 
 #endif
 

src/test/test_routerlist.c

@@ -5,6 +5,7 @@
 #include "or.h"
 #include "routerlist.h"
 #include "directory.h"
+#include "connection.h"
 #include "test.h"
 
 /* 4 digests + 3 sep + pre + post + NULL */
@@ -94,12 +95,84 @@ test_routerlist_launch_descriptor_downloads(void *arg)
   smartlist_free(downloadable);
 }
 
+connection_t *mocked_connection = NULL;
+
+/* Mock connection_get_by_type_addr_port_purpose by returning
+ * mocked_connection. */
+static connection_t *
+mock_connection_get_by_type_addr_port_purpose(int type,
+                                              const tor_addr_t *addr,
+                                              uint16_t port, int purpose)
+{
+  (void)type;
+  (void)addr;
+  (void)port;
+  (void)purpose;
+
+  return mocked_connection;
+}
+
+#define TEST_ADDR_STR "127.0.0.1"
+#define TEST_DIR_PORT 12345
+
+static void
+test_routerlist_router_is_already_dir_fetching(void *arg)
+{
+  (void)arg;
+  tor_addr_port_t test_ap, null_addr_ap, zero_port_ap;
+
+  /* Setup */
+  tor_addr_parse(&test_ap.addr, TEST_ADDR_STR);
+  test_ap.port = TEST_DIR_PORT;
+  tor_addr_make_null(&null_addr_ap.addr, AF_INET6);
+  null_addr_ap.port = TEST_DIR_PORT;
+  tor_addr_parse(&zero_port_ap.addr, TEST_ADDR_STR);
+  zero_port_ap.port = 0;
+  MOCK(connection_get_by_type_addr_port_purpose,
+       mock_connection_get_by_type_addr_port_purpose);
+
+  /* Test that we never get 1 from a NULL connection */
+  mocked_connection = NULL;
+  tt_assert(router_is_already_dir_fetching(&test_ap, 1, 1) == 0);
+  tt_assert(router_is_already_dir_fetching(&test_ap, 1, 0) == 0);
+  tt_assert(router_is_already_dir_fetching(&test_ap, 0, 1) == 0);
+  /* We always expect 0 in these cases */
+  tt_assert(router_is_already_dir_fetching(&test_ap, 0, 0) == 0);
+  tt_assert(router_is_already_dir_fetching(NULL, 1, 1) == 0);
+  tt_assert(router_is_already_dir_fetching(&null_addr_ap, 1, 1) == 0);
+  tt_assert(router_is_already_dir_fetching(&zero_port_ap, 1, 1) == 0);
+
+  /* Test that we get 1 with a connection in the appropriate circumstances */
+  mocked_connection = connection_new(CONN_TYPE_DIR, AF_INET);
+  tt_assert(router_is_already_dir_fetching(&test_ap, 1, 1) == 1);
+  tt_assert(router_is_already_dir_fetching(&test_ap, 1, 0) == 1);
+  tt_assert(router_is_already_dir_fetching(&test_ap, 0, 1) == 1);
+
+  /* Test that we get 0 even with a connection in the appropriate
+   * circumstances */
+  tt_assert(router_is_already_dir_fetching(&test_ap, 0, 0) == 0);
+  tt_assert(router_is_already_dir_fetching(NULL, 1, 1) == 0);
+  tt_assert(router_is_already_dir_fetching(&null_addr_ap, 1, 1) == 0);
+  tt_assert(router_is_already_dir_fetching(&zero_port_ap, 1, 1) == 0);
+
+ done:
+  /* If a connection is never set up, connection_free chokes on it. */
+  buf_free(mocked_connection->inbuf);
+  buf_free(mocked_connection->outbuf);
+  tor_free(mocked_connection);
+  UNMOCK(connection_get_by_type_addr_port_purpose);
+}
+
+#undef TEST_ADDR_STR
+#undef TEST_DIR_PORT
+
 #define NODE(name, flags) \
   { #name, test_routerlist_##name, (flags), NULL, NULL }
 
 struct testcase_t routerlist_tests[] = {
   NODE(initiate_descriptor_downloads, 0),
   NODE(launch_descriptor_downloads, 0),
+  NODE(router_is_already_dir_fetching, TT_FORK),
   END_OF_TESTCASES
 };