commit 45b28167d7e2b1d5afb26db6f76ca2329a9bbc04 Author: Neel Chauhan neel@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