[tor-commits] [tor/master] In count_acceptable_nodes(), count direct and indirect nodes with node_has_preferred_descriptor()

[nickm at torproject.org]

commit 45b28167d7e2b1d5afb26db6f76ca2329a9bbc04
Author: Neel Chauhan <neel at neelc.org>
Date:   Wed Oct 17 21:43:59 2018 -0400

    In count_acceptable_nodes(), count direct and indirect nodes with node_has_preferred_descriptor()
---
 changes/bug25885             |  7 +++++++
 src/core/or/circuitbuild.c   | 21 ++++++++++++---------
 src/core/or/circuitbuild.h   |  3 ++-
 src/test/test_circuitbuild.c |  4 ++--
 4 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/changes/bug25885 b/changes/bug25885
new file mode 100644
index 000000000..1b89acfe0
--- /dev/null
+++ b/changes/bug25885
@@ -0,0 +1,7 @@
+  o Minor bugfixes (guards):
+    - In count_acceptable_nodes(), check if we have at least one bridge
+      or guard node, and two non-guard nodes for a circuit. Previously,
+      we have added up the sum of all nodes with a descriptor, but that
+      could cause us to build circuits that fail if we had either too
+      many bridges, or not enough guard nodes. Fixes bug 25885; bugfix
+      on 0.3.6.1-alpha. Patch by Neel Chauhan.
diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index a69457571..4f9f09bc8 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -1658,22 +1658,25 @@ route_len_for_purpose(uint8_t purpose, extend_info_t *exit_ei)
 STATIC int
 new_route_len(uint8_t purpose, extend_info_t *exit_ei, smartlist_t *nodes)
 {
-  int num_acceptable_routers;
   int routelen;
 
   tor_assert(nodes);
 
   routelen = route_len_for_purpose(purpose, exit_ei);
 
-  num_acceptable_routers = count_acceptable_nodes(nodes);
+  int num_acceptable_direct = count_acceptable_nodes(nodes, 1);
+  int num_acceptable_indirect = count_acceptable_nodes(nodes, 0);
 
-  log_debug(LD_CIRC,"Chosen route length %d (%d/%d routers suitable).",
-            routelen, num_acceptable_routers, smartlist_len(nodes));
+  log_debug(LD_CIRC,"Chosen route length %d (%d direct and %d indirect "
+             "routers suitable).", routelen, num_acceptable_direct,
+             num_acceptable_indirect);
 
-  if (num_acceptable_routers < routelen) {
+  if (num_acceptable_direct < 1 || num_acceptable_indirect < routelen - 1) {
     log_info(LD_CIRC,
-             "Not enough acceptable routers (%d/%d). Discarding this circuit.",
-             num_acceptable_routers, routelen);
+             "Not enough acceptable routers (%d/%d direct and %d/%d "
+             "indirect routers suitable). Discarding this circuit.",
+             num_acceptable_direct, routelen,
+             num_acceptable_indirect, routelen);
     return -1;
   }
 
@@ -2315,7 +2318,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))
+count_acceptable_nodes, (smartlist_t *nodes, int direct))
 {
   int num=0;
 
@@ -2329,7 +2332,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_any_descriptor(node))
+    if (! node_has_preferred_descriptor(node, direct))
       continue;
     /* The node has a descriptor, so we can just check the ntor key directly */
     if (!node_has_curve25519_onion_key(node))
diff --git a/src/core/or/circuitbuild.h b/src/core/or/circuitbuild.h
index cee71b297..93f903f06 100644
--- a/src/core/or/circuitbuild.h
+++ b/src/core/or/circuitbuild.h
@@ -84,7 +84,8 @@ void circuit_upgrade_circuits_from_guard_wait(void);
 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);
-MOCK_DECL(STATIC int, count_acceptable_nodes, (smartlist_t *nodes));
+MOCK_DECL(STATIC int, count_acceptable_nodes, (smartlist_t *nodes,
+                                               int direct));
 
 STATIC int onion_extend_cpath(origin_circuit_t *circ);
 
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
index 02eadecd9..dd47ad768 100644
--- a/src/test/test_circuitbuild.c
+++ b/src/test/test_circuitbuild.c
@@ -21,11 +21,11 @@ static smartlist_t dummy_nodes;
 static extend_info_t dummy_ei;
 
 static int
-mock_count_acceptable_nodes(smartlist_t *nodes)
+mock_count_acceptable_nodes(smartlist_t *nodes, int direct)
 {
   (void)nodes;
 
-  return DEFAULT_ROUTE_LEN + 1;
+  return direct ? 1 : DEFAULT_ROUTE_LEN + 1;
 }
 
 /* Test route lengths when the caller of new_route_len() doesn't