Merge branch 'bug25691_033_again_squashed'

changes/bug25691_again

@@ -0,0 +1,6 @@
+  o Minor bugfixes (path selection):
+    - Only select relays when they have the descriptors we prefer to
+      use for them. This change fixes a bug where we could select
+      a relay because it had _some_ descriptor, but reject it later with
+      a nonfatal assertion error because it didn't have the exact one we
+      wanted. Fixes bugs 25691 and 25692; bugfix on 0.3.3.4-alpha.

src/or/circuitbuild.c

@@ -418,7 +418,7 @@ onion_populate_cpath(origin_circuit_t *circ)
                                     circ->cpath->extend_info->identity_digest);
     /* If we don't know the node and its descriptor, we must be bootstrapping.
      */
-    if (!node || !node_has_descriptor(node)) {
+    if (!node || !node_has_preferred_descriptor(node, 1)) {
       return 0;
     }
   }
@@ -1758,7 +1758,7 @@ ap_stream_wants_exit_attention(connection_t *conn)
  * Return NULL if we can't find any suitable routers.
  */
 static const node_t *
-choose_good_exit_server_general(int need_uptime, int need_capacity)
+choose_good_exit_server_general(router_crn_flags_t flags)
 {
   int *n_supported;
   int n_pending_connections = 0;
@@ -1768,6 +1768,9 @@ choose_good_exit_server_general(int need_uptime, int need_capacity)
   const or_options_t *options = get_options();
   const smartlist_t *the_nodes;
   const node_t *selected_node=NULL;
+  const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
+  const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
+  const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
 
   connections = get_connection_array();
 
@@ -1800,7 +1803,7 @@ choose_good_exit_server_general(int need_uptime, int need_capacity)
        */
       continue;
     }
-    if (!node_has_descriptor(node)) {
+    if (!node_has_preferred_descriptor(node, direct_conn)) {
       n_supported[i] = -1;
       continue;
     }
@@ -1913,7 +1916,8 @@ choose_good_exit_server_general(int need_uptime, int need_capacity)
                  need_capacity?", fast":"",
                  need_uptime?", stable":"");
         tor_free(n_supported);
-        return choose_good_exit_server_general(0, 0);
+        flags &= ~(CRN_NEED_UPTIME|CRN_NEED_CAPACITY);
+        return choose_good_exit_server_general(flags);
       }
       log_notice(LD_CIRC, "All routers are down or won't exit%s -- "
                  "choosing a doomed exit at random.",
@@ -2160,17 +2164,11 @@ pick_restricted_middle_node(router_crn_flags_t flags,
  * toward the preferences in 'options'.
  */
 static const node_t *
-choose_good_exit_server(origin_circuit_t *circ, int need_uptime,
-                        int need_capacity, int is_internal, int need_hs_v3)
+choose_good_exit_server(origin_circuit_t *circ,
+                        router_crn_flags_t flags, int is_internal)
 {
   const or_options_t *options = get_options();
-  router_crn_flags_t flags = CRN_NEED_DESC;
-  if (need_uptime)
-    flags |= CRN_NEED_UPTIME;
-  if (need_capacity)
-    flags |= CRN_NEED_CAPACITY;
-  if (need_hs_v3)
-    flags |= CRN_RENDEZVOUS_V3;
+  flags |= CRN_NEED_DESC;
 
   switch (TO_CIRCUIT(circ)->purpose) {
     case CIRCUIT_PURPOSE_C_HSDIR_GET:
@@ -2184,7 +2182,7 @@ choose_good_exit_server(origin_circuit_t *circ, int need_uptime,
       if (is_internal) /* pick it like a middle hop */
         return router_choose_random_node(NULL, options->ExcludeNodes, flags);
       else
-        return choose_good_exit_server_general(need_uptime,need_capacity);
+        return choose_good_exit_server_general(flags);
     case CIRCUIT_PURPOSE_C_ESTABLISH_REND:
       {
         /* Pick a new RP */
@@ -2309,15 +2307,22 @@ onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit_ei,
              extend_info_describe(exit_ei));
     exit_ei = extend_info_dup(exit_ei);
   } else { /* we have to decide one */
+    router_crn_flags_t flags = CRN_NEED_DESC;
+    if (state->need_uptime)
+      flags |= CRN_NEED_UPTIME;
+    if (state->need_capacity)
+      flags |= CRN_NEED_CAPACITY;
+    if (is_hs_v3_rp_circuit)
+      flags |= CRN_RENDEZVOUS_V3;
+    if (state->onehop_tunnel)
+      flags |= CRN_DIRECT_CONN;
     const node_t *node =
-      choose_good_exit_server(circ, state->need_uptime,
-                              state->need_capacity, state->is_internal,
-                              is_hs_v3_rp_circuit);
+      choose_good_exit_server(circ, flags, state->is_internal);
     if (!node) {
       log_warn(LD_CIRC,"Failed to choose an exit server");
       return -1;
     }
-    exit_ei = extend_info_from_node(node, 0);
+    exit_ei = extend_info_from_node(node, state->onehop_tunnel);
     if (BUG(exit_ei == NULL))
       return -1;
   }
@@ -2374,6 +2379,10 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit_ei)
 
 /** Return the number of routers in <b>routers</b> that are currently up
  * and available for building circuits through.
+ *
+ * (Note that this function may overcount or undercount, if we have
+ * descriptors that are not the type we would prefer to use for some
+ * particular router. See bug #25885.)
  */
 MOCK_IMPL(STATIC int,
 count_acceptable_nodes, (smartlist_t *nodes))
@@ -2390,7 +2399,7 @@ count_acceptable_nodes, (smartlist_t *nodes))
     if (! node->is_valid)
 //      log_debug(LD_CIRC,"Nope, the directory says %d is not valid.",i);
       continue;
-    if (! node_has_descriptor(node))
+    if (! node_has_any_descriptor(node))
       continue;
     /* The node has a descriptor, so we can just check the ntor key directly */
     if (!node_has_curve25519_onion_key(node))
@@ -2778,9 +2787,10 @@ extend_info_new(const char *nickname,
  * of the node (i.e. its IPv4 address) unless
  * <b>for_direct_connect</b> is true, in which case the preferred
  * address is used instead. May return NULL if there is not enough
- * info about <b>node</b> to extend to it--for example, if there is no
- * routerinfo_t or microdesc_t, or if for_direct_connect is true and none of
- * the node's addresses are allowed by tor's firewall and IP version config.
+ * info about <b>node</b> to extend to it--for example, if the preferred
+ * routerinfo_t or microdesc_t is missing, or if for_direct_connect is
+ * true and none of the node's addresses is allowed by tor's firewall
+ * and IP version config.
  **/
 extend_info_t *
 extend_info_from_node(const node_t *node, int for_direct_connect)
@@ -2788,17 +2798,8 @@ extend_info_from_node(const node_t *node, int for_direct_connect)
   tor_addr_port_t ap;
   int valid_addr = 0;
 
-  const int is_bridge = node_is_a_configured_bridge(node);
-  const int we_use_mds = we_use_microdescriptors_for_circuits(get_options());
-
-  if ((is_bridge && for_direct_connect) || !we_use_mds) {
-    /* We need an ri in this case. */
-    if (!node->ri)
-      return NULL;
-  } else {
-    /* Otherwise we need an md. */
-    if (node->rs == NULL || node->md == NULL)
-      return NULL;
+  if (!node_has_preferred_descriptor(node, for_direct_connect)) {
+    return NULL;
   }
 
   /* Choose a preferred address first, but fall back to an allowed address.

src/or/circuituse.c

@@ -2383,7 +2383,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
         const node_t *r;
         int opt = conn->chosen_exit_optional;
         r = node_get_by_nickname(conn->chosen_exit_name, 0);
-        if (r && node_has_descriptor(r)) {
+        if (r && node_has_preferred_descriptor(r, conn->want_onehop ? 1 : 0)) {
           /* We might want to connect to an IPv6 bridge for loading
              descriptors so we use the preferred address rather than
              the primary. */
@@ -2393,7 +2393,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
                      "Discarding this circuit.", conn->chosen_exit_name);
             return -1;
           }
-        } else  { /* ! (r && node_has_descriptor(r)) */
+        } else  { /* ! (r && node_has_preferred_descriptor(...)) */
           log_debug(LD_DIR, "considering %d, %s",
                     want_onehop, conn->chosen_exit_name);
           if (want_onehop && conn->chosen_exit_name[0] == '$') {

src/or/control.c

@@ -3487,17 +3487,19 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len,
   smartlist_free(args);
 
   nodes = smartlist_new();
+  int first_node = zero_circ;
   SMARTLIST_FOREACH_BEGIN(router_nicknames, const char *, n) {
     const node_t *node = node_get_by_nickname(n, 0);
     if (!node) {
       connection_printf_to_buf(conn, "552 No such router \"%s\"\r\n", n);
       goto done;
     }
-    if (!node_has_descriptor(node)) {
+    if (!node_has_preferred_descriptor(node, first_node)) {
       connection_printf_to_buf(conn, "552 No descriptor for \"%s\"\r\n", n);
       goto done;
     }
     smartlist_add(nodes, (void*)node);
+    first_node = 0;
   } SMARTLIST_FOREACH_END(n);
   if (!smartlist_len(nodes)) {
     connection_write_str_to_buf("512 No router names provided\r\n", conn);
@@ -3510,14 +3512,15 @@ handle_control_extendcircuit(control_connection_t *conn, uint32_t len,
   }
 
   /* now circ refers to something that is ready to be extended */
-  int first_node = zero_circ;
+  first_node = zero_circ;
   SMARTLIST_FOREACH(nodes, const node_t *, node,
   {
     extend_info_t *info = extend_info_from_node(node, first_node);
     if (!info) {
       tor_assert_nonfatal(first_node);
       log_warn(LD_CONTROL,
-               "controller tried to connect to a node that doesn't have any "
+               "controller tried to connect to a node that lacks a suitable "
+               "descriptor, or which doesn't have any "
                "addresses that are allowed by the firewall configuration; "
                "circuit marked for closing.");
       circuit_mark_for_close(TO_CIRCUIT(circ), -END_CIRC_REASON_CONNECTFAILED);

src/or/entrynodes.c

@@ -185,14 +185,14 @@ should_apply_guardfraction(const networkstatus_t *ns)
   return options->UseGuardFraction;
 }
 
-/** Return true iff we know a descriptor for <b>guard</b> */
+/** Return true iff we know a preferred descriptor for <b>guard</b> */
 static int
 guard_has_descriptor(const entry_guard_t *guard)
 {
   const node_t *node = node_get_by_id(guard->identity);
   if (!node)
     return 0;
-  return node_has_descriptor(node);
+  return node_has_preferred_descriptor(node, 1);
 }
 
 /**
@@ -2269,7 +2269,8 @@ entry_guard_pick_for_circuit(guard_selection_t *gs,
   // XXXX #20827 check Ed ID.
   if (! node)
     goto fail;
-  if (BUG(usage != GUARD_USAGE_DIRGUARD && !node_has_descriptor(node)))
+  if (BUG(usage != GUARD_USAGE_DIRGUARD &&
+          !node_has_preferred_descriptor(node, 1)))
     goto fail;
 
   *chosen_node_out = node;

src/or/hs_common.c

@@ -1279,8 +1279,10 @@ node_has_hsdir_index(const node_t *node)
   tor_assert(node_supports_v3_hsdir(node));
 
   /* A node can't have an HSDir index without a descriptor since we need desc
-   * to get its ed25519 key */
-  if (!node_has_descriptor(node)) {
+   * to get its ed25519 key.  for_direct_connect should be zero, since we
+   * always use the consensus-indexed node's keys to build the hash ring, even
+   * if some of the consensus-indexed nodes are also bridges. */
+  if (!node_has_preferred_descriptor(node, 0)) {
     return 0;
   }
 
@@ -1611,12 +1613,17 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
   hs_clean_last_hid_serv_requests(now);
 
   /* Only select those hidden service directories to which we did not send a
-   * request recently and for which we have a router descriptor here. */
+   * request recently and for which we have a router descriptor here.
+   *
+   * Use for_direct_connect==0 even if we will be connecting to the node
+   * directly, since we always use the key information in the
+   * consensus-indexed node descriptors for building the index.
+   **/
   SMARTLIST_FOREACH_BEGIN(responsible_dirs, routerstatus_t *, dir) {
     time_t last = hs_lookup_last_hid_serv_request(dir, req_key_str, 0, 0);
     const node_t *node = node_get_by_id(dir->identity_digest);
     if (last + hs_hsdir_requery_period(options) >= now ||
-        !node || !node_has_descriptor(node)) {
+        !node || !node_has_preferred_descriptor(node, 0)) {
       SMARTLIST_DEL_CURRENT(responsible_dirs, dir);
       continue;
     }

src/or/nodelist.c

@@ -43,6 +43,7 @@
 #include "or.h"
 #include "address.h"
 #include "address_set.h"
+#include "bridges.h"
 #include "config.h"
 #include "control.h"
 #include "dirserv.h"
@@ -1130,15 +1131,44 @@ node_is_dir(const node_t *node)
   }
 }
 
-/** Return true iff <b>node</b> has either kind of usable descriptor -- that
- * is, a routerdescriptor or a microdescriptor. */
+/** Return true iff <b>node</b> has either kind of descriptor -- that
+ * is, a routerdescriptor or a microdescriptor.
+ *
+ * You should probably use node_has_preferred_descriptor() instead.
+ **/
 int
-node_has_descriptor(const node_t *node)
+node_has_any_descriptor(const node_t *node)
 {
   return (node->ri ||
           (node->rs && node->md));
 }
 
+/** Return true iff <b>node</b> has the kind of descriptor we would prefer to
+ * use for it, given our configuration and how we intend to use the node.
+ *
+ * If <b>for_direct_connect</b> is true, we intend to connect to the node
+ * directly, as the first hop of a circuit; otherwise, we intend to connect to
+ * it indirectly, or use it as if we were connecting to it indirectly. */
+int
+node_has_preferred_descriptor(const node_t *node,
+                              int for_direct_connect)
+{
+  const int is_bridge = node_is_a_configured_bridge(node);
+  const int we_use_mds = we_use_microdescriptors_for_circuits(get_options());
+
+  if ((is_bridge && for_direct_connect) || !we_use_mds) {
+    /* We need an ri in this case. */
+    if (!node->ri)
+      return 0;
+  } else {
+    /* Otherwise we need an rs and an md. */
+    if (node->rs == NULL || node->md == NULL)
+      return 0;
+  }
+
+  return 1;
+}
+
 /** Return the router_purpose of <b>node</b>. */
 int
 node_get_purpose(const node_t *node)
@@ -2223,7 +2253,8 @@ compute_frac_paths_available(const networkstatus_t *consensus,
               nu);
 
     SMARTLIST_FOREACH_BEGIN(myexits_unflagged, const node_t *, node) {
-      if (node_has_descriptor(node) && node_exit_policy_rejects_all(node)) {
+      if (node_has_preferred_descriptor(node, 0) &&
+          node_exit_policy_rejects_all(node)) {
         SMARTLIST_DEL_CURRENT(myexits_unflagged, node);
         /* this node is not actually an exit */
         np--;

src/or/nodelist.h

@@ -46,7 +46,9 @@ void node_get_verbose_nickname(const node_t *node,
 void node_get_verbose_nickname_by_id(const char *id_digest,
                                 char *verbose_name_out);
 int node_is_dir(const node_t *node);
-int node_has_descriptor(const node_t *node);
+int node_has_any_descriptor(const node_t *node);
+int node_has_preferred_descriptor(const node_t *node,
+                                  int for_direct_connect);
 int node_get_purpose(const node_t *node);
 #define node_is_bridge(node) \
   (node_get_purpose((node)) == ROUTER_PURPOSE_BRIDGE)

src/or/rendservice.c

@@ -3596,7 +3596,7 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc,
         /* Don't upload descriptor if we succeeded in doing so last time. */
         continue;
       node = node_get_by_id(hs_dir->identity_digest);
-      if (!node || !node_has_descriptor(node)) {
+      if (!node || !node_has_preferred_descriptor(node,0)) {
         log_info(LD_REND, "Not launching upload for for v2 descriptor to "
                           "hidden service directory %s; we don't have its "
                           "router descriptor. Queuing for later upload.",

src/or/routerlist.c

@@ -2335,7 +2335,7 @@ router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
   SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), const node_t *, node) {
     if (!node->is_running || !node->is_valid)
       continue;
-    if (need_desc && !(node->ri || (node->rs && node->md)))
+    if (need_desc && !node_has_preferred_descriptor(node, direct_conn))
       continue;
     if (node->ri && node->ri->purpose != ROUTER_PURPOSE_GENERAL)
       continue;
@@ -2758,7 +2758,7 @@ frac_nodes_with_descriptors(const smartlist_t *sl,
       total <= 0.0) {
     int n_with_descs = 0;
     SMARTLIST_FOREACH(sl, const node_t *, node, {
-      if (node_has_descriptor(node))
+      if (node_has_any_descriptor(node))
         n_with_descs++;
     });
     return ((double)n_with_descs) / (double)smartlist_len(sl);
@@ -2766,7 +2766,7 @@ frac_nodes_with_descriptors(const smartlist_t *sl,
 
   present = 0.0;
   SMARTLIST_FOREACH_BEGIN(sl, const node_t *, node) {
-    if (node_has_descriptor(node))
+    if (node_has_any_descriptor(node))
       present += bandwidths[node_sl_idx];
   } SMARTLIST_FOREACH_END(node);
 

src/test/test_hs.c

@@ -361,6 +361,7 @@ test_pick_tor2web_rendezvous_node(void *arg)
 
   /* Parse Tor2webRendezvousPoints as a routerset. */
   options->Tor2webRendezvousPoints = routerset_new();
+  options->UseMicrodescriptors = 0;
   retval = routerset_parse(options->Tor2webRendezvousPoints,
                            tor2web_rendezvous_str,
                            "test_tor2web_rp");

src/test/test_hs_common.c

@@ -305,7 +305,8 @@ helper_add_hsdir_to_networkstatus(networkstatus_t *ns,
   node_t *node = node_get_mutable_by_id(ri->cache_info.identity_digest);
   tt_assert(node);
   node->rs = rs;
-  /* We need this to exist for node_has_descriptor() to return true. */
+  /* We need this to exist for node_has_preferred_descriptor() to return
+   * true. */
   node->md = tor_malloc_zero(sizeof(microdesc_t));
   /* Do this now the nodelist_set_routerinfo() function needs a "rs" to set
    * the indexes which it doesn't have when it is called. */