commit afb6ae7b0fb87550c12e09e5eb7c2a09e675909d Author: Chelsea H. Komlo chelsea.komlo@gmail.com Date: Fri Oct 14 07:00:35 2016 -0500

Refactor circuit_predict_and_launch_new --- changes/ticket18873 | 2 + src/or/circuitbuild.c | 6 +- src/or/circuitbuild.h | 5 +- src/or/circuituse.c | 166 +++++++++++++++++--------- src/or/circuituse.h | 20 ++++ src/or/nodelist.c | 4 +- src/or/nodelist.h | 3 +- src/test/include.am | 1 + src/test/test.c | 1 + src/test/test.h | 1 + src/test/test_circuituse.c | 284 +++++++++++++++++++++++++++++++++++++++++++++ 11 files changed, 431 insertions(+), 62 deletions(-)

diff --git a/changes/ticket18873 b/changes/ticket18873 index ada6798..df4cb1e 100644 --- a/changes/ticket18873 +++ b/changes/ticket18873 @@ -1,3 +1,5 @@ o Code simplification and refactoring: - Extracted dummy_origin_circuit_new so it can be used by other test functions. + - Refactor circuit_predict_and_launch_new for readability and testability. + - Added unit tests for extracted functions. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 0881f23..dee8ac0 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1508,9 +1508,9 @@ circuit_get_unhandled_ports(time_t now) * If we're returning 0, set need_uptime and need_capacity to * indicate any requirements that the unhandled ports have. */ -int -circuit_all_predicted_ports_handled(time_t now, int *need_uptime, - int *need_capacity) +MOCK_IMPL(int, +circuit_all_predicted_ports_handled, (time_t now, int *need_uptime, + int *need_capacity)) { int i, enough; uint16_t *port; diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 1244601..7a67589 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -40,8 +40,9 @@ int onionskin_answer(or_circuit_t *circ, const struct created_cell_t *created_cell, const char *keys, const uint8_t *rend_circ_nonce); -int circuit_all_predicted_ports_handled(time_t now, int *need_uptime, - int *need_capacity); +MOCK_DECL(int, circuit_all_predicted_ports_handled, (time_t now, + int *need_uptime, + int *need_capacity));

int circuit_append_new_exit(origin_circuit_t *circ, extend_info_t *info); int circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *info); diff --git a/src/or/circuituse.c b/src/or/circuituse.c index ba7b75f..682961d 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1022,8 +1022,95 @@ circuit_stream_is_being_handled(entry_connection_t *conn, /** Don't keep more than this many unused open circuits around. */ #define MAX_UNUSED_OPEN_CIRCUITS 14

-/** Figure out how many circuits we have open that are clean. Make - * sure it's enough for all the upcoming behaviors we predict we'll have. +/* Return true if a circuit is available for use, meaning that it is open, + * clean, usable for new multi-hop connections, and a general purpose origin + * circuit. + * Accept any kind of circuit, return false if the above conditions are not + * met. */ +STATIC int +circuit_is_available_for_use(const circuit_t *circ) +{ + const origin_circuit_t *origin_circ; + cpath_build_state_t *build_state; + + if (!CIRCUIT_IS_ORIGIN(circ)) + return 0; + if (circ->marked_for_close) + return 0; /* Don't mess with marked circs */ + if (circ->timestamp_dirty) + return 0; /* Only count clean circs */ + if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL) + return 0;/* Only pay attention to general + purpose circs */ + + origin_circ = CONST_TO_ORIGIN_CIRCUIT(circ); + if (origin_circ->unusable_for_new_conns) + return 0; + + build_state = origin_circ->build_state; + if (build_state->onehop_tunnel) + return 0; + + return 1; +} + +/* Return true if we need any more exit circuits. + * needs_uptime and needs_capacity are set only if we need more exit circuits. + * Check if we know of a port that's been requested recently and no circuit + * is currently available that can handle it. */ +STATIC int +needs_exit_circuits(time_t now, int *needs_uptime, int *needs_capacity) +{ + return (!circuit_all_predicted_ports_handled(now, needs_uptime, + needs_capacity) && + router_have_consensus_path() == CONSENSUS_PATH_EXIT); +} + +/* Return true if we need any more hidden service server circuits. + * HS servers only need an internal circuit. */ +STATIC int +needs_hs_server_circuits(int num_uptime_internal) +{ + return (num_rend_services() && num_uptime_internal < 3 && + router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN); +} + +/* Return true if we need any more hidden service client circuits. + * HS clients only need an internal circuit. */ +STATIC int +needs_hs_client_circuits(time_t now, int *needs_uptime, int *needs_capacity, + int num_internal, int num_uptime_internal) +{ + int used_internal_recently = rep_hist_get_predicted_internal(now, + needs_uptime, + needs_capacity); + return (used_internal_recently && + ((num_uptime_internal<2 && needs_uptime) || num_internal<3) && + router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN); +} + +/* Check to see if we still need more circuits to learn + * a good build timeout. But if we're close to our max number we + * want, don't do another -- we want to leave a few slots open so + * we can still build circuits preemptively as needed. + * XXXX make the assumption that build timeout streams should be + * created whenever we can build internal circuits. */ +STATIC int +needs_circuits_for_build(int num) +{ + if (router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) { + if (num < MAX_UNUSED_OPEN_CIRCUITS-2 && + ! circuit_build_times_disabled() && + circuit_build_times_needs_circuits_now(get_circuit_build_times())) + { + return 1; + } + } + return 0; +} + +/** Determine how many circuits we have open that are clean, + * Make sure it's enough for all the upcoming behaviors we predict we'll have. * But put an upper bound on the total number of circuits. */ static void @@ -1035,25 +1122,14 @@ circuit_predict_and_launch_new(void) time_t now = time(NULL); int flags = 0;

- /* First, count how many of each type of circuit we have already. */ + /* Count how many of each type of circuit we currently have. */ SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) { - cpath_build_state_t *build_state; - origin_circuit_t *origin_circ; - if (!CIRCUIT_IS_ORIGIN(circ)) - continue; - if (circ->marked_for_close) - continue; /* don't mess with marked circs */ - if (circ->timestamp_dirty) - continue; /* only count clean circs */ - if (circ->purpose != CIRCUIT_PURPOSE_C_GENERAL) - continue; /* only pay attention to general-purpose circs */ - origin_circ = TO_ORIGIN_CIRCUIT(circ); - if (origin_circ->unusable_for_new_conns) - continue; - build_state = origin_circ->build_state; - if (build_state->onehop_tunnel) + if (!circuit_is_available_for_use(circ)) continue; + num++; + + cpath_build_state_t *build_state = TO_ORIGIN_CIRCUIT(circ)->build_state; if (build_state->is_internal) num_internal++; if (build_state->need_uptime && build_state->is_internal) @@ -1063,19 +1139,14 @@ circuit_predict_and_launch_new(void)

/* If that's enough, then stop now. */ if (num >= MAX_UNUSED_OPEN_CIRCUITS) - return; /* we already have many, making more probably will hurt */ - - /* Second, see if we need any more exit circuits. */ - /* check if we know of a port that's been requested recently - * and no circuit is currently available that can handle it. - * Exits (obviously) require an exit circuit. */ - if (!circuit_all_predicted_ports_handled(now, &port_needs_uptime, - &port_needs_capacity) - && router_have_consensus_path() == CONSENSUS_PATH_EXIT) { + return; + + if (needs_exit_circuits(now, &port_needs_uptime, &port_needs_capacity)) { if (port_needs_uptime) flags |= CIRCLAUNCH_NEED_UPTIME; if (port_needs_capacity) flags |= CIRCLAUNCH_NEED_CAPACITY; + log_info(LD_CIRC, "Have %d clean circs (%d internal), need another exit circ.", num, num_internal); @@ -1083,12 +1154,10 @@ circuit_predict_and_launch_new(void) return; }

- /* Third, see if we need any more hidden service (server) circuits. - * HS servers only need an internal circuit. */ - if (num_rend_services() && num_uptime_internal < 3 - && router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) { + if (needs_hs_server_circuits(num_uptime_internal)) { flags = (CIRCLAUNCH_NEED_CAPACITY | CIRCLAUNCH_NEED_UPTIME | CIRCLAUNCH_IS_INTERNAL); + log_info(LD_CIRC, "Have %d clean circs (%d internal), need another internal " "circ for my hidden service.", @@ -1097,18 +1166,16 @@ circuit_predict_and_launch_new(void) return; }

- /* Fourth, see if we need any more hidden service (client) circuits. - * HS clients only need an internal circuit. */ - if (rep_hist_get_predicted_internal(now, &hidserv_needs_uptime, - &hidserv_needs_capacity) && - ((num_uptime_internal<2 && hidserv_needs_uptime) || - num_internal<3) - && router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) { + if (needs_hs_client_circuits(now, &hidserv_needs_uptime, + &hidserv_needs_capacity, + num_internal, num_uptime_internal)) + { if (hidserv_needs_uptime) flags |= CIRCLAUNCH_NEED_UPTIME; if (hidserv_needs_capacity) flags |= CIRCLAUNCH_NEED_CAPACITY; flags |= CIRCLAUNCH_IS_INTERNAL; + log_info(LD_CIRC, "Have %d clean circs (%d uptime-internal, %d internal), need" " another hidden service circ.", @@ -1117,26 +1184,17 @@ circuit_predict_and_launch_new(void) return; }

- /* Finally, check to see if we still need more circuits to learn - * a good build timeout. But if we're close to our max number we - * want, don't do another -- we want to leave a few slots open so - * we can still build circuits preemptively as needed. - * XXXX make the assumption that build timeout streams should be - * created whenever we can build internal circuits. */ - if (router_have_consensus_path() != CONSENSUS_PATH_UNKNOWN) { - if (num < MAX_UNUSED_OPEN_CIRCUITS-2 && - ! circuit_build_times_disabled() && - circuit_build_times_needs_circuits_now(get_circuit_build_times())) { - flags = CIRCLAUNCH_NEED_CAPACITY; - /* if there are no exits in the consensus, make timeout - * circuits internal */ - if (router_have_consensus_path() == CONSENSUS_PATH_INTERNAL) - flags |= CIRCLAUNCH_IS_INTERNAL; + if (needs_circuits_for_build(num)) { + flags = CIRCLAUNCH_NEED_CAPACITY; + /* if there are no exits in the consensus, make timeout + * circuits internal */ + if (router_have_consensus_path() == CONSENSUS_PATH_INTERNAL) + flags |= CIRCLAUNCH_IS_INTERNAL; + log_info(LD_CIRC, "Have %d clean circs need another buildtime test circ.", num); circuit_launch(CIRCUIT_PURPOSE_C_GENERAL, flags); return; - } } }

diff --git a/src/or/circuituse.h b/src/or/circuituse.h index 5973978..d484be1 100644 --- a/src/or/circuituse.h +++ b/src/or/circuituse.h @@ -59,5 +59,25 @@ int hostname_in_track_host_exits(const or_options_t *options, const char *address); void mark_circuit_unusable_for_new_conns(origin_circuit_t *circ);

+#ifdef TOR_UNIT_TESTS +/* Used only by circuituse.c and test_circuituse.c */ + +STATIC int circuit_is_available_for_use(const circuit_t *circ); + +STATIC int needs_exit_circuits(time_t now, + int *port_needs_uptime, + int *port_needs_capacity); +STATIC int needs_hs_server_circuits(int num_uptime_internal); + +STATIC int needs_hs_client_circuits(time_t now, + int *needs_uptime, + int *needs_capacity, + int num_internal, + int num_uptime_internal); + +STATIC int needs_circuits_for_build(int num); + +#endif + #endif

diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 2802d5b..4d180dc 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -1569,8 +1569,8 @@ router_have_minimum_dir_info(void) * this can cause router_have_consensus_path() to be set to * CONSENSUS_PATH_EXIT, even if there are no nodes with accept exit policies. */ -consensus_path_type_t -router_have_consensus_path(void) +MOCK_IMPL(consensus_path_type_t, +router_have_consensus_path, (void)) { return have_consensus_path; } diff --git a/src/or/nodelist.h b/src/or/nodelist.h index 71a91e1..bfee935 100644 --- a/src/or/nodelist.h +++ b/src/or/nodelist.h @@ -118,7 +118,8 @@ typedef enum { * create exit and internal paths, circuits, streams, ... */ CONSENSUS_PATH_EXIT = 1 } consensus_path_type_t; -consensus_path_type_t router_have_consensus_path(void); + +MOCK_DECL(consensus_path_type_t, router_have_consensus_path, (void));

void router_dir_info_changed(void); const char *get_dir_info_status_string(void); diff --git a/src/test/include.am b/src/test/include.am index f6e4314..8d0fc2f 100644 --- a/src/test/include.am +++ b/src/test/include.am @@ -80,6 +80,7 @@ src_test_test_SOURCES = \ src/test/test_checkdir.c \ src/test/test_circuitlist.c \ src/test/test_circuitmux.c \ + src/test/test_circuituse.c \ src/test/test_compat_libevent.c \ src/test/test_config.c \ src/test/test_connection.c \ diff --git a/src/test/test.c b/src/test/test.c index b02ecb9..750d8b0 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -1190,6 +1190,7 @@ struct testgroup_t testgroups[] = { { "checkdir/", checkdir_tests }, { "circuitlist/", circuitlist_tests }, { "circuitmux/", circuitmux_tests }, + { "circuituse/", circuituse_tests }, { "compat/libevent/", compat_libevent_tests }, { "config/", config_tests }, { "connection/", connection_tests }, diff --git a/src/test/test.h b/src/test/test.h index 49b4499..2fa7359 100644 --- a/src/test/test.h +++ b/src/test/test.h @@ -185,6 +185,7 @@ extern struct testcase_t channeltls_tests[]; extern struct testcase_t checkdir_tests[]; extern struct testcase_t circuitlist_tests[]; extern struct testcase_t circuitmux_tests[]; +extern struct testcase_t circuituse_tests[]; extern struct testcase_t compat_libevent_tests[]; extern struct testcase_t config_tests[]; extern struct testcase_t connection_tests[]; diff --git a/src/test/test_circuituse.c b/src/test/test_circuituse.c new file mode 100644 index 0000000..23f1f9c --- /dev/null +++ b/src/test/test_circuituse.c @@ -0,0 +1,284 @@ +/* Copyright (c) 2001-2004, Roger Dingledine. + * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson. + * Copyright (c) 2007-2016, The Tor Project, Inc. */ +/* See LICENSE for licensing information */ + +#include "or.h" +#include "test.h" +#include "test_helpers.h" +#include "config.h" +#include "circuitlist.h" +#include "circuituse.h" +#include "circuitbuild.h" +#include "nodelist.h" + +static void +test_circuit_is_available_for_use_ret_false_when_marked_for_close(void *arg) +{ + (void)arg; + + circuit_t *circ = tor_malloc(sizeof(circuit_t)); + circ->marked_for_close = 1; + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_ret_false_when_timestamp_dirty(void *arg) +{ + (void)arg; + + circuit_t *circ = tor_malloc(sizeof(circuit_t)); + circ->timestamp_dirty = 1; + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_ret_false_for_non_general_purpose(void *arg) +{ + (void)arg; + + circuit_t *circ = tor_malloc(sizeof(circuit_t)); + circ->purpose = CIRCUIT_PURPOSE_OR; + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_ret_false_for_non_origin_purpose(void *arg) +{ + (void)arg; + + circuit_t *circ = tor_malloc(sizeof(circuit_t)); + circ->purpose = CIRCUIT_PURPOSE_OR; + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_ret_false_unusable_for_new_conns(void *arg) +{ + (void)arg; + + circuit_t *circ = dummy_origin_circuit_new(30); + mark_circuit_unusable_for_new_conns(TO_ORIGIN_CIRCUIT(circ)); + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_returns_false_for_onehop_tunnel(void *arg) +{ + (void)arg; + + circuit_t *circ = dummy_origin_circuit_new(30); + origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ); + oc->build_state = tor_malloc(sizeof(cpath_build_state_t)); + oc->build_state->onehop_tunnel = 1; + + tt_int_op(0, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static void +test_circuit_is_available_for_use_returns_true_for_clean_circuit(void *arg) +{ + (void)arg; + + circuit_t *circ = dummy_origin_circuit_new(30); + origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ); + oc->build_state = tor_malloc(sizeof(cpath_build_state_t)); + oc->build_state->onehop_tunnel = 0; + + tt_int_op(1, ==, circuit_is_available_for_use(circ)); + + done: + tor_free(circ); +} + +static int +mock_circuit_all_predicted_ports_handled(time_t now, + int *need_uptime, + int *need_capacity) +{ + (void)now; + + if (need_uptime && need_capacity) + return 0; + return 1; +} + +static consensus_path_type_t +mock_router_have_unknown_consensus_path(void) +{ + return CONSENSUS_PATH_UNKNOWN; +} + +static consensus_path_type_t +mock_router_have_exit_consensus_path(void) +{ + return CONSENSUS_PATH_EXIT; +} + +static void +test_needs_exit_circuits_ret_false_for_predicted_ports_and_path(void *arg) +{ + (void)arg; + + MOCK(circuit_all_predicted_ports_handled, + mock_circuit_all_predicted_ports_handled); + int needs_uptime = 1; + int needs_capacity = 0; + + time_t now = time(NULL); + tt_int_op(0, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity)); + + done: + UNMOCK(circuit_all_predicted_ports_handled); +} + +static void +test_needs_exit_circuits_ret_false_for_non_exit_consensus_path(void *arg) +{ + (void)arg; + + MOCK(circuit_all_predicted_ports_handled, + mock_circuit_all_predicted_ports_handled); + int needs_uptime = 1; + int needs_capacity = 1; + MOCK(router_have_consensus_path, mock_router_have_unknown_consensus_path); + + time_t now = time(NULL); + tt_int_op(0, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity)); + + done: + UNMOCK(circuit_all_predicted_ports_handled); + UNMOCK(router_have_consensus_path); +} + +static void +test_needs_exit_circuits_ret_true_for_predicted_ports_and_path(void *arg) +{ + (void)arg; + + MOCK(circuit_all_predicted_ports_handled, + mock_circuit_all_predicted_ports_handled); + int needs_uptime = 1; + int needs_capacity = 1; + MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path); + + time_t now = time(NULL); + tt_int_op(1, ==, needs_exit_circuits(now, &needs_uptime, &needs_capacity)); + + done: + UNMOCK(circuit_all_predicted_ports_handled); + UNMOCK(router_have_consensus_path); +} + +static void +test_needs_circuits_for_build_ret_false_consensus_path_unknown(void *arg) +{ + (void)arg; + MOCK(router_have_consensus_path, mock_router_have_unknown_consensus_path); + tt_int_op(0, ==, needs_circuits_for_build(0)); + done: ; +} + +static void +test_needs_circuits_for_build_ret_false_if_num_less_than_max(void *arg) +{ + (void)arg; + MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path); + tt_int_op(0, ==, needs_circuits_for_build(13)); + done: + UNMOCK(router_have_consensus_path); +} + +static void +test_needs_circuits_for_build_returns_true_when_more_are_needed(void *arg) +{ + (void)arg; + MOCK(router_have_consensus_path, mock_router_have_exit_consensus_path); + tt_int_op(1, ==, needs_circuits_for_build(0)); + done: + UNMOCK(router_have_consensus_path); +} + +struct testcase_t circuituse_tests[] = { + { "marked", + test_circuit_is_available_for_use_ret_false_when_marked_for_close, + TT_FORK, NULL, NULL + }, + { "timestamp", + test_circuit_is_available_for_use_ret_false_when_timestamp_dirty, + TT_FORK, NULL, NULL + }, + { "non_general", + test_circuit_is_available_for_use_ret_false_for_non_general_purpose, + TT_FORK, NULL, NULL + }, + { "origin", + test_circuit_is_available_for_use_ret_false_for_non_origin_purpose, + TT_FORK, NULL, NULL + }, + { "clean", + test_circuit_is_available_for_use_ret_false_unusable_for_new_conns, + TT_FORK, NULL, NULL + }, + { "onehop", + test_circuit_is_available_for_use_returns_false_for_onehop_tunnel, + TT_FORK, NULL, NULL + }, + { "clean_circ", + test_circuit_is_available_for_use_returns_true_for_clean_circuit, + TT_FORK, NULL, NULL + }, + { "exit_f", + test_needs_exit_circuits_ret_false_for_predicted_ports_and_path, + TT_FORK, NULL, NULL + }, + { "exit_t", + test_needs_exit_circuits_ret_true_for_predicted_ports_and_path, + TT_FORK, NULL, NULL + }, + { "non_exit", + test_needs_exit_circuits_ret_false_for_non_exit_consensus_path, + TT_FORK, NULL, NULL + }, + { "true", + test_needs_exit_circuits_ret_true_for_predicted_ports_and_path, + TT_FORK, NULL, NULL + }, + { "consensus_path_unknown", + test_needs_circuits_for_build_ret_false_consensus_path_unknown, + TT_FORK, NULL, NULL + }, + { "less_than_max", + test_needs_circuits_for_build_ret_false_if_num_less_than_max, + TT_FORK, NULL, NULL + }, + { "more_needed", + test_needs_circuits_for_build_returns_true_when_more_are_needed, + TT_FORK, NULL, NULL + }, + END_OF_TESTCASES +}; +