Merge branch 'tor-github/pr/983'

changes/ticket30307

@@ -0,0 +1,4 @@
+  o Major features (performance):
+    - Update our node selection algorithm to exclude nodes in linear time.
+      Previously, the algorithm was quadratic, which could slow down heavily
+      used onion services.  Closes ticket 30307.

changes/ticket30308

@@ -0,0 +1,5 @@
+  o Minor bugfixes (performance):
+    - When checking a node for bridge status, use a fast check to make sure
+      that its identity is set. Previously, we used a constant-time check,
+      which is not necessary when verifying a BUG() condition that causes
+      a stack trace. Fixes bug 30308; bugfix on 0.3.5.1-alpha.

scripts/maint/practracker/exceptions.txt

@@ -78,7 +78,7 @@ problem function-size /src/core/or/channeltls.c:channel_tls_process_versions_cel
 problem function-size /src/core/or/channeltls.c:channel_tls_process_netinfo_cell() 214
 problem function-size /src/core/or/channeltls.c:channel_tls_process_certs_cell() 246
 problem function-size /src/core/or/channeltls.c:channel_tls_process_authenticate_cell() 202
-problem file-size /src/core/or/circuitbuild.c 3060
+problem file-size /src/core/or/circuitbuild.c 3061
 problem include-count /src/core/or/circuitbuild.c 53
 problem function-size /src/core/or/circuitbuild.c:get_unique_circ_id_by_chan() 128
 problem function-size /src/core/or/circuitbuild.c:circuit_extend() 147

src/core/or/circuitbuild.c

@@ -1683,7 +1683,8 @@ route_len_for_purpose(uint8_t purpose, extend_info_t *exit_ei)
  * to handle the desired path length, return -1.
  */
 STATIC int
-new_route_len(uint8_t purpose, extend_info_t *exit_ei, smartlist_t *nodes)
+new_route_len(uint8_t purpose, extend_info_t *exit_ei,
+              const smartlist_t *nodes)
 {
   int routelen;
 
@@ -2345,7 +2346,7 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit_ei)
  * particular router. See bug #25885.)
  */
 MOCK_IMPL(STATIC int,
-count_acceptable_nodes, (smartlist_t *nodes, int direct))
+count_acceptable_nodes, (const smartlist_t *nodes, int direct))
 {
   int num=0;
 

src/core/or/circuitbuild.h

@@ -83,8 +83,8 @@ void circuit_upgrade_circuits_from_guard_wait(void);
 #ifdef CIRCUITBUILD_PRIVATE
 STATIC circid_t get_unique_circ_id_by_chan(channel_t *chan);
 STATIC int new_route_len(uint8_t purpose, extend_info_t *exit_ei,
-                         smartlist_t *nodes);
+                         const smartlist_t *nodes);
-MOCK_DECL(STATIC int, count_acceptable_nodes, (smartlist_t *nodes,
+MOCK_DECL(STATIC int, count_acceptable_nodes, (const smartlist_t *nodes,
                                                int direct));
 
 STATIC int onion_extend_cpath(origin_circuit_t *circ);

src/feature/client/bridges.c

@@ -348,7 +348,7 @@ int
 node_is_a_configured_bridge(const node_t *node)
 {
   /* First, let's try searching for a bridge with matching identity. */
-  if (BUG(tor_digest_is_zero(node->identity)))
+  if (BUG(tor_mem_is_zero(node->identity, DIGEST_LEN)))
     return 0;
 
   if (find_bridge_by_digest(node->identity) != NULL)

src/feature/dirauth/voteflags.c

@@ -239,7 +239,7 @@ dirserv_compute_performance_thresholds(digestmap_t *omit_as_sybil)
   uint32_t *uptimes, *bandwidths_kb, *bandwidths_excluding_exits_kb;
   long *tks;
   double *mtbfs, *wfus;
-  smartlist_t *nodelist;
+  const smartlist_t *nodelist;
   time_t now = time(NULL);
   const or_options_t *options = get_options();
 

src/feature/nodelist/node_select.c

@@ -30,6 +30,7 @@
 #include "feature/nodelist/routerset.h"
 #include "feature/relay/router.h"
 #include "feature/relay/routermode.h"
+#include "lib/container/bitarray.h"
 #include "lib/crypt_ops/crypto_rand.h"
 #include "lib/math/fp.h"
 
@@ -826,6 +827,58 @@ routerlist_add_node_and_family(smartlist_t *sl, const routerinfo_t *router)
   nodelist_add_node_and_family(sl, node);
 }
 
+/**
+ * Remove every node_t that appears in <b>excluded</b> from <b>sl</b>.
+ *
+ * Behaves like smartlist_subtract, but uses nodelist_idx values to deliver
+ * linear performance when smartlist_subtract would be quadratic.
+ **/
+static void
+nodelist_subtract(smartlist_t *sl, const smartlist_t *excluded)
+{
+  const smartlist_t *nodelist = nodelist_get_list();
+  const int nodelist_len = smartlist_len(nodelist);
+  bitarray_t *excluded_idx = bitarray_init_zero(nodelist_len);
+
+  /* We haven't used nodelist_idx in this way previously, so I'm going to be
+   * paranoid in this code, and check that nodelist_idx is correct for every
+   * node before we use it.  If we fail, we fall back to smartlist_subtract().
+   */
+
+  /* Set the excluded_idx bit corresponding to every excluded node...
+   */
+  SMARTLIST_FOREACH_BEGIN(excluded, const node_t *, node) {
+    const int idx = node->nodelist_idx;
+    if (BUG(idx < 0) || BUG(idx >= nodelist_len) ||
+        BUG(node != smartlist_get(nodelist, idx))) {
+      goto internal_error;
+    }
+    bitarray_set(excluded_idx, idx);
+  } SMARTLIST_FOREACH_END(node);
+
+  /* Then remove them from sl.
+   */
+  SMARTLIST_FOREACH_BEGIN(sl, const node_t *, node) {
+    const int idx = node->nodelist_idx;
+    if (BUG(idx < 0) || BUG(idx >= nodelist_len) ||
+        BUG(node != smartlist_get(nodelist, idx))) {
+      goto internal_error;
+    }
+    if (bitarray_is_set(excluded_idx, idx)) {
+      SMARTLIST_DEL_CURRENT(sl, node);
+    }
+  } SMARTLIST_FOREACH_END(node);
+
+  bitarray_free(excluded_idx);
+  return;
+
+ internal_error:
+  log_warn(LD_BUG, "Internal error prevented us from using the fast method "
+           "for subtracting nodelists. Falling back to the quadratic way.");
+  smartlist_subtract(sl, excluded);
+  bitarray_free(excluded_idx);
+}
+
 /** Return a random running node from the nodelist. Never
  * pick a node that is in
  * <b>excludedsmartlist</b>, or which matches <b>excludedset</b>,
@@ -860,6 +913,7 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
   const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
 
+  const smartlist_t *node_list = nodelist_get_list();
   smartlist_t *sl=smartlist_new(),
     *excludednodes=smartlist_new();
   const node_t *choice = NULL;
@@ -870,17 +924,17 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   rule = weight_for_exit ? WEIGHT_FOR_EXIT :
     (need_guard ? WEIGHT_FOR_GUARD : WEIGHT_FOR_MID);
 
-  SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), node_t *, node) {
+  SMARTLIST_FOREACH_BEGIN(node_list, const node_t *, node) {
     if (node_allows_single_hop_exits(node)) {
       /* Exclude relays that allow single hop exit circuits. This is an
        * obsolete option since 0.2.9.2-alpha and done by default in
        * 0.3.1.0-alpha. */
-      smartlist_add(excludednodes, node);
+      smartlist_add(excludednodes, (node_t*)node);
     } else if (rendezvous_v3 &&
                !node_supports_v3_rendezvous_point(node)) {
       /* Exclude relays that do not support to rendezvous for a hidden service
        * version 3. */
-      smartlist_add(excludednodes, node);
+      smartlist_add(excludednodes, (node_t*)node);
     }
   } SMARTLIST_FOREACH_END(node);
 
@@ -897,19 +951,11 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
            "We found %d running nodes.",
             smartlist_len(sl));
 
-  smartlist_subtract(sl,excludednodes);
-  log_debug(LD_CIRC,
-            "We removed %d excludednodes, leaving %d nodes.",
-            smartlist_len(excludednodes),
-            smartlist_len(sl));
-
   if (excludedsmartlist) {
-    smartlist_subtract(sl,excludedsmartlist);
+    smartlist_add_all(excludednodes, excludedsmartlist);
-    log_debug(LD_CIRC,
-              "We removed %d excludedsmartlist, leaving %d nodes.",
-              smartlist_len(excludedsmartlist),
-              smartlist_len(sl));
   }
+  nodelist_subtract(sl, excludednodes);
+
   if (excludedset) {
     routerset_subtract_nodes(sl,excludedset);
     log_debug(LD_CIRC,

src/feature/nodelist/nodelist.c

@@ -944,7 +944,7 @@ nodelist_ensure_freshness(networkstatus_t *ns)
 /** Return a list of a node_t * for every node we know about.  The caller
  * MUST NOT modify the list. (You can set and clear flags in the nodes if
  * you must, but you must not add or remove nodes.) */
-MOCK_IMPL(smartlist_t *,
+MOCK_IMPL(const smartlist_t *,
 nodelist_get_list,(void))
 {
   init_nodelist();
@@ -1939,7 +1939,7 @@ node_set_country(node_t *node)
 void
 nodelist_refresh_countries(void)
 {
-  smartlist_t *nodes = nodelist_get_list();
+  const smartlist_t *nodes = nodelist_get_list();
   SMARTLIST_FOREACH(nodes, node_t *, node,
                     node_set_country(node));
 }

src/feature/nodelist/nodelist.h

@@ -101,7 +101,7 @@ const struct curve25519_public_key_t *node_get_curve25519_onion_key(
                                   const node_t *node);
 crypto_pk_t *node_get_rsa_onion_key(const node_t *node);
 
-MOCK_DECL(smartlist_t *, nodelist_get_list, (void));
+MOCK_DECL(const smartlist_t *, nodelist_get_list, (void));
 
 /* Temporary during transition to multiple addresses.  */
 void node_get_addr(const node_t *node, tor_addr_t *addr_out);

src/feature/nodelist/routerset.c

@@ -378,7 +378,7 @@ routerset_get_all_nodes(smartlist_t *out, const routerset_t *routerset,
   } else {
     /* We need to iterate over the routerlist to get all the ones of the
      * right kind. */
-    smartlist_t *nodes = nodelist_get_list();
+    const smartlist_t *nodes = nodelist_get_list();
     SMARTLIST_FOREACH(nodes, const node_t *, node, {
         if (running_only && !node->is_running)
           continue;

src/test/test_circuitbuild.c

@@ -21,7 +21,7 @@ static smartlist_t dummy_nodes;
 static extend_info_t dummy_ei;
 
 static int
-mock_count_acceptable_nodes(smartlist_t *nodes, int direct)
+mock_count_acceptable_nodes(const smartlist_t *nodes, int direct)
 {
   (void)nodes;
 

src/test/test_controller.c

@@ -1723,7 +1723,7 @@ test_current_time(void *arg)
 static size_t n_nodelist_get_list = 0;
 static smartlist_t *nodes = NULL;
 
-static smartlist_t *
+static const smartlist_t *
 mock_nodelist_get_list(void)
 {
   n_nodelist_get_list++;

src/test/test_entrynodes.c

@@ -67,7 +67,7 @@ static networkstatus_t *dummy_consensus = NULL;
 
 static smartlist_t *big_fake_net_nodes = NULL;
 
-static smartlist_t *
+static const smartlist_t *
 bfn_mock_nodelist_get_list(void)
 {
   return big_fake_net_nodes;
@@ -197,6 +197,7 @@ big_fake_network_setup(const struct testcase_t *testcase)
       n->md->exit_policy = parse_short_policy("accept 443");
     }
 
+    n->nodelist_idx = smartlist_len(big_fake_net_nodes);
     smartlist_add(big_fake_net_nodes, n);
   }
 

src/test/test_helpers.c

@@ -78,7 +78,7 @@ helper_setup_fake_routerlist(void)
 {
   int retval;
   routerlist_t *our_routerlist = NULL;
-  smartlist_t *our_nodelist = NULL;
+  const smartlist_t *our_nodelist = NULL;
 
   /* Read the file that contains our test descriptors. */
 

src/test/test_hs_common.c

@@ -275,7 +275,7 @@ test_start_time_of_next_time_period(void *arg)
 static void
 cleanup_nodelist(void)
 {
-  smartlist_t *nodelist = nodelist_get_list();
+  const smartlist_t *nodelist = nodelist_get_list();
   SMARTLIST_FOREACH_BEGIN(nodelist, node_t *, node) {
     tor_free(node->md);
     node->md = NULL;

src/test/test_routerset.c

@@ -1765,7 +1765,7 @@ NS(node_get_by_nickname)(const char *nickname, unsigned flags)
  * Structural test for routerset_get_all_nodes, when the nodelist has no nodes.
  */
 
-NS_DECL(smartlist_t *, nodelist_get_list, (void));
+NS_DECL(const smartlist_t *, nodelist_get_list, (void));
 
 static smartlist_t *NS(mock_smartlist);
 
@@ -1795,7 +1795,7 @@ NS(test_main)(void *arg)
     ;
 }
 
-smartlist_t *
+const smartlist_t *
 NS(nodelist_get_list)(void)
 {
   CALLED(nodelist_get_list)++;
@@ -1811,7 +1811,7 @@ NS(nodelist_get_list)(void)
  * the running_only flag is set, but the nodes are not running.
  */
 
-NS_DECL(smartlist_t *, nodelist_get_list, (void));
+NS_DECL(const smartlist_t *, nodelist_get_list, (void));
 
 static smartlist_t *NS(mock_smartlist);
 static node_t NS(mock_node);
@@ -1844,7 +1844,7 @@ NS(test_main)(void *arg)
     ;
 }
 
-smartlist_t *
+const smartlist_t *
 NS(nodelist_get_list)(void)
 {
   CALLED(nodelist_get_list)++;