From 935f84bd40f51dc7d4b6477ade37d72ecda7e407 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 3 Jul 2017 16:14:48 -0400 Subject: [PATCH 1/6] Split circuit_send_next_onion_skin() into its three main cases. This commit is designed to have a very small diff. Therefore, the indentation is wrong. The next commit will fix that. --- src/or/circuitbuild.c | 51 +++++++++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 833c0b98b7..21ed038664 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -74,6 +74,10 @@ static int onion_pick_cpath_exit(origin_circuit_t *circ, extend_info_t *exit); static crypt_path_t *onion_next_hop_in_cpath(crypt_path_t *cpath); static int onion_extend_cpath(origin_circuit_t *circ); static int onion_append_hop(crypt_path_t **head_ptr, extend_info_t *choice); +static int circuit_send_first_onion_skin(origin_circuit_t *circ); +static int circuit_build_no_more_hops(origin_circuit_t *circ); +static int circuit_send_intermediate_onion_skin(origin_circuit_t *circ, + crypt_path_t *hop); /** This function tries to get a channel to the specified endpoint, * and then calls command_setup_channel() to give it the right @@ -920,12 +924,28 @@ circuit_purpose_may_omit_guard(int purpose) int circuit_send_next_onion_skin(origin_circuit_t *circ) { - crypt_path_t *hop; - const node_t *node; tor_assert(circ); if (circ->cpath->state == CPATH_STATE_CLOSED) { + return circuit_send_first_onion_skin(circ); + } else { + tor_assert(circ->cpath->state == CPATH_STATE_OPEN); + tor_assert(circ->base_.state == CIRCUIT_STATE_BUILDING); + log_debug(LD_CIRC,"starting to send subsequent skin."); + crypt_path_t *hop = onion_next_hop_in_cpath(circ->cpath); + if (!hop) { + return circuit_build_no_more_hops(circ); + } else { + return circuit_send_intermediate_onion_skin(circ, hop); + } + } +} + +static int +circuit_send_first_onion_skin(origin_circuit_t *circ) +{ + const node_t *node; /* This is the first hop. */ create_cell_t cc; int fast; @@ -980,15 +1000,12 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) log_info(LD_CIRC,"First hop: finished sending %s cell to '%s'", fast ? "CREATE_FAST" : "CREATE", node ? node_describe(node) : ""); - } else { - extend_cell_t ec; - int len; - tor_assert(circ->cpath->state == CPATH_STATE_OPEN); - tor_assert(circ->base_.state == CIRCUIT_STATE_BUILDING); - log_debug(LD_CIRC,"starting to send subsequent skin."); - hop = onion_next_hop_in_cpath(circ->cpath); - memset(&ec, 0, sizeof(ec)); - if (!hop) { + return 0; +} + +static int +circuit_build_no_more_hops(origin_circuit_t *circ) +{ /* done building the circuit. whew. */ guard_usable_t r; if (! circ->guard_state) { @@ -1090,8 +1107,15 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_FINISHED); } return 0; - } +} +static int +circuit_send_intermediate_onion_skin(origin_circuit_t *circ, + crypt_path_t *hop) +{ + extend_cell_t ec; + int len; + memset(&ec, 0, sizeof(ec)); if (tor_addr_family(&hop->extend_info->addr) != AF_INET) { log_warn(LD_BUG, "Trying to extend to a non-IPv4 address."); return - END_CIRC_REASON_INTERNAL; @@ -1139,8 +1163,7 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) return 0; /* circuit is closed */ } hop->state = CPATH_STATE_AWAITING_KEYS; - } - return 0; + return 0; } /** Our clock just jumped by seconds_elapsed. Assume From 2814b86875964aedd1fb62a13510073c23f05cd5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 3 Jul 2017 16:16:26 -0400 Subject: [PATCH 2/6] Reindent the functions split from circuit_send_next_onion_skin(). This is a whitespace change only. --- src/or/circuitbuild.c | 386 +++++++++++++++++++++--------------------- 1 file changed, 193 insertions(+), 193 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 21ed038664..ee33361840 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -946,224 +946,224 @@ static int circuit_send_first_onion_skin(origin_circuit_t *circ) { const node_t *node; - /* This is the first hop. */ - create_cell_t cc; - int fast; - int len; - log_debug(LD_CIRC,"First skin; sending create cell."); - memset(&cc, 0, sizeof(cc)); - if (circ->build_state->onehop_tunnel) - control_event_bootstrap(BOOTSTRAP_STATUS_ONEHOP_CREATE, 0); - else { - control_event_bootstrap(BOOTSTRAP_STATUS_CIRCUIT_CREATE, 0); + /* This is the first hop. */ + create_cell_t cc; + int fast; + int len; + log_debug(LD_CIRC,"First skin; sending create cell."); + memset(&cc, 0, sizeof(cc)); + if (circ->build_state->onehop_tunnel) + control_event_bootstrap(BOOTSTRAP_STATUS_ONEHOP_CREATE, 0); + else { + control_event_bootstrap(BOOTSTRAP_STATUS_CIRCUIT_CREATE, 0); - /* If this is not a one-hop tunnel, the channel is being used - * for traffic that wants anonymity and protection from traffic - * analysis (such as netflow record retention). That means we want - * to pad it. - */ - if (circ->base_.n_chan->channel_usage < CHANNEL_USED_FOR_FULL_CIRCS) - circ->base_.n_chan->channel_usage = CHANNEL_USED_FOR_FULL_CIRCS; - } + /* If this is not a one-hop tunnel, the channel is being used + * for traffic that wants anonymity and protection from traffic + * analysis (such as netflow record retention). That means we want + * to pad it. + */ + if (circ->base_.n_chan->channel_usage < CHANNEL_USED_FOR_FULL_CIRCS) + circ->base_.n_chan->channel_usage = CHANNEL_USED_FOR_FULL_CIRCS; + } - node = node_get_by_id(circ->base_.n_chan->identity_digest); - fast = should_use_create_fast_for_circuit(circ); - if (!fast) { - /* We are an OR and we know the right onion key: we should - * send a create cell. - */ - circuit_pick_create_handshake(&cc.cell_type, &cc.handshake_type, - circ->cpath->extend_info); - } else { - /* We are not an OR, and we're building the first hop of a circuit to a - * new OR: we can be speedy and use CREATE_FAST to save an RSA operation - * and a DH operation. */ - cc.cell_type = CELL_CREATE_FAST; - cc.handshake_type = ONION_HANDSHAKE_TYPE_FAST; - } + node = node_get_by_id(circ->base_.n_chan->identity_digest); + fast = should_use_create_fast_for_circuit(circ); + if (!fast) { + /* We are an OR and we know the right onion key: we should + * send a create cell. + */ + circuit_pick_create_handshake(&cc.cell_type, &cc.handshake_type, + circ->cpath->extend_info); + } else { + /* We are not an OR, and we're building the first hop of a circuit to a + * new OR: we can be speedy and use CREATE_FAST to save an RSA operation + * and a DH operation. */ + cc.cell_type = CELL_CREATE_FAST; + cc.handshake_type = ONION_HANDSHAKE_TYPE_FAST; + } - len = onion_skin_create(cc.handshake_type, - circ->cpath->extend_info, - &circ->cpath->handshake_state, - cc.onionskin); - if (len < 0) { - log_warn(LD_CIRC,"onion_skin_create (first hop) failed."); - return - END_CIRC_REASON_INTERNAL; - } - cc.handshake_len = len; + len = onion_skin_create(cc.handshake_type, + circ->cpath->extend_info, + &circ->cpath->handshake_state, + cc.onionskin); + if (len < 0) { + log_warn(LD_CIRC,"onion_skin_create (first hop) failed."); + return - END_CIRC_REASON_INTERNAL; + } + cc.handshake_len = len; - if (circuit_deliver_create_cell(TO_CIRCUIT(circ), &cc, 0) < 0) - return - END_CIRC_REASON_RESOURCELIMIT; + if (circuit_deliver_create_cell(TO_CIRCUIT(circ), &cc, 0) < 0) + return - END_CIRC_REASON_RESOURCELIMIT; - circ->cpath->state = CPATH_STATE_AWAITING_KEYS; - circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_BUILDING); - log_info(LD_CIRC,"First hop: finished sending %s cell to '%s'", - fast ? "CREATE_FAST" : "CREATE", - node ? node_describe(node) : ""); - return 0; + circ->cpath->state = CPATH_STATE_AWAITING_KEYS; + circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_BUILDING); + log_info(LD_CIRC,"First hop: finished sending %s cell to '%s'", + fast ? "CREATE_FAST" : "CREATE", + node ? node_describe(node) : ""); + return 0; } static int circuit_build_no_more_hops(origin_circuit_t *circ) { - /* done building the circuit. whew. */ - guard_usable_t r; - if (! circ->guard_state) { - if (circuit_get_cpath_len(circ) != 1 && - ! circuit_purpose_may_omit_guard(circ->base_.purpose) && - get_options()->UseEntryGuards) { - log_warn(LD_BUG, "%d-hop circuit %p with purpose %d has no " - "guard state", - circuit_get_cpath_len(circ), circ, circ->base_.purpose); - } - r = GUARD_USABLE_NOW; - } else { - r = entry_guard_succeeded(&circ->guard_state); - } - const int is_usable_for_streams = (r == GUARD_USABLE_NOW); - if (r == GUARD_USABLE_NOW) { - circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_OPEN); - } else if (r == GUARD_MAYBE_USABLE_LATER) { - // Wait till either a better guard succeeds, or till - // all better guards fail. - circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_GUARD_WAIT); - } else { - tor_assert_nonfatal(r == GUARD_USABLE_NEVER); - return - END_CIRC_REASON_INTERNAL; + /* done building the circuit. whew. */ + guard_usable_t r; + if (! circ->guard_state) { + if (circuit_get_cpath_len(circ) != 1 && + ! circuit_purpose_may_omit_guard(circ->base_.purpose) && + get_options()->UseEntryGuards) { + log_warn(LD_BUG, "%d-hop circuit %p with purpose %d has no " + "guard state", + circuit_get_cpath_len(circ), circ, circ->base_.purpose); + } + r = GUARD_USABLE_NOW; + } else { + r = entry_guard_succeeded(&circ->guard_state); + } + const int is_usable_for_streams = (r == GUARD_USABLE_NOW); + if (r == GUARD_USABLE_NOW) { + circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_OPEN); + } else if (r == GUARD_MAYBE_USABLE_LATER) { + // Wait till either a better guard succeeds, or till + // all better guards fail. + circuit_set_state(TO_CIRCUIT(circ), CIRCUIT_STATE_GUARD_WAIT); + } else { + tor_assert_nonfatal(r == GUARD_USABLE_NEVER); + return - END_CIRC_REASON_INTERNAL; + } + + /* XXXX #21422 -- the rest of this branch needs careful thought! + * Some of the things here need to happen when a circuit becomes + * mechanically open; some need to happen when it is actually usable. + * I think I got them right, but more checking would be wise. -NM + */ + + if (circuit_timeout_want_to_count_circ(circ)) { + struct timeval end; + long timediff; + tor_gettimeofday(&end); + timediff = tv_mdiff(&circ->base_.timestamp_began, &end); + + /* + * If the circuit build time is much greater than we would have cut + * it off at, we probably had a suspend event along this codepath, + * and we should discard the value. + */ + if (timediff < 0 || + timediff > 2*get_circuit_build_close_time_ms()+1000) { + log_notice(LD_CIRC, "Strange value for circuit build time: %ldmsec. " + "Assuming clock jump. Purpose %d (%s)", timediff, + circ->base_.purpose, + circuit_purpose_to_string(circ->base_.purpose)); + } else if (!circuit_build_times_disabled(get_options())) { + /* Only count circuit times if the network is live */ + if (circuit_build_times_network_check_live( + get_circuit_build_times())) { + circuit_build_times_add_time(get_circuit_build_times_mutable(), + (build_time_t)timediff); + circuit_build_times_set_timeout(get_circuit_build_times_mutable()); } - /* XXXX #21422 -- the rest of this branch needs careful thought! - * Some of the things here need to happen when a circuit becomes - * mechanically open; some need to happen when it is actually usable. - * I think I got them right, but more checking would be wise. -NM - */ - - if (circuit_timeout_want_to_count_circ(circ)) { - struct timeval end; - long timediff; - tor_gettimeofday(&end); - timediff = tv_mdiff(&circ->base_.timestamp_began, &end); - - /* - * If the circuit build time is much greater than we would have cut - * it off at, we probably had a suspend event along this codepath, - * and we should discard the value. - */ - if (timediff < 0 || - timediff > 2*get_circuit_build_close_time_ms()+1000) { - log_notice(LD_CIRC, "Strange value for circuit build time: %ldmsec. " - "Assuming clock jump. Purpose %d (%s)", timediff, - circ->base_.purpose, - circuit_purpose_to_string(circ->base_.purpose)); - } else if (!circuit_build_times_disabled(get_options())) { - /* Only count circuit times if the network is live */ - if (circuit_build_times_network_check_live( - get_circuit_build_times())) { - circuit_build_times_add_time(get_circuit_build_times_mutable(), - (build_time_t)timediff); - circuit_build_times_set_timeout(get_circuit_build_times_mutable()); - } - - if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { - circuit_build_times_network_circ_success( - get_circuit_build_times_mutable()); - } - } + if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { + circuit_build_times_network_circ_success( + get_circuit_build_times_mutable()); } - log_info(LD_CIRC,"circuit built!"); - circuit_reset_failure_count(0); + } + } + log_info(LD_CIRC,"circuit built!"); + circuit_reset_failure_count(0); - if (circ->build_state->onehop_tunnel || circ->has_opened) { - control_event_bootstrap(BOOTSTRAP_STATUS_REQUESTING_STATUS, 0); - } + if (circ->build_state->onehop_tunnel || circ->has_opened) { + control_event_bootstrap(BOOTSTRAP_STATUS_REQUESTING_STATUS, 0); + } - pathbias_count_build_success(circ); - circuit_rep_hist_note_result(circ); - if (is_usable_for_streams) - circuit_has_opened(circ); /* do other actions as necessary */ + pathbias_count_build_success(circ); + circuit_rep_hist_note_result(circ); + if (is_usable_for_streams) + circuit_has_opened(circ); /* do other actions as necessary */ - if (!have_completed_a_circuit() && !circ->build_state->onehop_tunnel) { - const or_options_t *options = get_options(); - note_that_we_completed_a_circuit(); - /* FFFF Log a count of known routers here */ - log_notice(LD_GENERAL, - "Tor has successfully opened a circuit. " - "Looks like client functionality is working."); - if (control_event_bootstrap(BOOTSTRAP_STATUS_DONE, 0) == 0) { - log_notice(LD_GENERAL, - "Tor has successfully opened a circuit. " - "Looks like client functionality is working."); - } - control_event_client_status(LOG_NOTICE, "CIRCUIT_ESTABLISHED"); - clear_broken_connection_map(1); - if (server_mode(options) && !check_whether_orport_reachable(options)) { - inform_testing_reachability(); - consider_testing_reachability(1, 1); - } - } + if (!have_completed_a_circuit() && !circ->build_state->onehop_tunnel) { + const or_options_t *options = get_options(); + note_that_we_completed_a_circuit(); + /* FFFF Log a count of known routers here */ + log_notice(LD_GENERAL, + "Tor has successfully opened a circuit. " + "Looks like client functionality is working."); + if (control_event_bootstrap(BOOTSTRAP_STATUS_DONE, 0) == 0) { + log_notice(LD_GENERAL, + "Tor has successfully opened a circuit. " + "Looks like client functionality is working."); + } + control_event_client_status(LOG_NOTICE, "CIRCUIT_ESTABLISHED"); + clear_broken_connection_map(1); + if (server_mode(options) && !check_whether_orport_reachable(options)) { + inform_testing_reachability(); + consider_testing_reachability(1, 1); + } + } - /* We're done with measurement circuits here. Just close them */ - if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { - circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_FINISHED); - } - return 0; + /* We're done with measurement circuits here. Just close them */ + if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { + circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_FINISHED); + } + return 0; } static int circuit_send_intermediate_onion_skin(origin_circuit_t *circ, crypt_path_t *hop) { - extend_cell_t ec; - int len; - memset(&ec, 0, sizeof(ec)); - if (tor_addr_family(&hop->extend_info->addr) != AF_INET) { - log_warn(LD_BUG, "Trying to extend to a non-IPv4 address."); - return - END_CIRC_REASON_INTERNAL; + extend_cell_t ec; + int len; + memset(&ec, 0, sizeof(ec)); + if (tor_addr_family(&hop->extend_info->addr) != AF_INET) { + log_warn(LD_BUG, "Trying to extend to a non-IPv4 address."); + return - END_CIRC_REASON_INTERNAL; + } + + circuit_pick_extend_handshake(&ec.cell_type, + &ec.create_cell.cell_type, + &ec.create_cell.handshake_type, + hop->extend_info); + + tor_addr_copy(&ec.orport_ipv4.addr, &hop->extend_info->addr); + ec.orport_ipv4.port = hop->extend_info->port; + tor_addr_make_unspec(&ec.orport_ipv6.addr); + memcpy(ec.node_id, hop->extend_info->identity_digest, DIGEST_LEN); + /* Set the ED25519 identity too -- it will only get included + * in the extend2 cell if we're configured to use it, though. */ + ed25519_pubkey_copy(&ec.ed_pubkey, &hop->extend_info->ed_identity); + + len = onion_skin_create(ec.create_cell.handshake_type, + hop->extend_info, + &hop->handshake_state, + ec.create_cell.onionskin); + if (len < 0) { + log_warn(LD_CIRC,"onion_skin_create failed."); + return - END_CIRC_REASON_INTERNAL; + } + ec.create_cell.handshake_len = len; + + log_info(LD_CIRC,"Sending extend relay cell."); + { + uint8_t command = 0; + uint16_t payload_len=0; + uint8_t payload[RELAY_PAYLOAD_SIZE]; + if (extend_cell_format(&command, &payload_len, payload, &ec)<0) { + log_warn(LD_CIRC,"Couldn't format extend cell"); + return -END_CIRC_REASON_INTERNAL; } - circuit_pick_extend_handshake(&ec.cell_type, - &ec.create_cell.cell_type, - &ec.create_cell.handshake_type, - hop->extend_info); - - tor_addr_copy(&ec.orport_ipv4.addr, &hop->extend_info->addr); - ec.orport_ipv4.port = hop->extend_info->port; - tor_addr_make_unspec(&ec.orport_ipv6.addr); - memcpy(ec.node_id, hop->extend_info->identity_digest, DIGEST_LEN); - /* Set the ED25519 identity too -- it will only get included - * in the extend2 cell if we're configured to use it, though. */ - ed25519_pubkey_copy(&ec.ed_pubkey, &hop->extend_info->ed_identity); - - len = onion_skin_create(ec.create_cell.handshake_type, - hop->extend_info, - &hop->handshake_state, - ec.create_cell.onionskin); - if (len < 0) { - log_warn(LD_CIRC,"onion_skin_create failed."); - return - END_CIRC_REASON_INTERNAL; - } - ec.create_cell.handshake_len = len; - - log_info(LD_CIRC,"Sending extend relay cell."); - { - uint8_t command = 0; - uint16_t payload_len=0; - uint8_t payload[RELAY_PAYLOAD_SIZE]; - if (extend_cell_format(&command, &payload_len, payload, &ec)<0) { - log_warn(LD_CIRC,"Couldn't format extend cell"); - return -END_CIRC_REASON_INTERNAL; - } - - /* send it to hop->prev, because it will transfer - * it to a create cell and then send to hop */ - if (relay_send_command_from_edge(0, TO_CIRCUIT(circ), - command, - (char*)payload, payload_len, - hop->prev) < 0) - return 0; /* circuit is closed */ - } - hop->state = CPATH_STATE_AWAITING_KEYS; - return 0; + /* send it to hop->prev, because it will transfer + * it to a create cell and then send to hop */ + if (relay_send_command_from_edge(0, TO_CIRCUIT(circ), + command, + (char*)payload, payload_len, + hop->prev) < 0) + return 0; /* circuit is closed */ + } + hop->state = CPATH_STATE_AWAITING_KEYS; + return 0; } /** Our clock just jumped by seconds_elapsed. Assume From 9b44e2e50e8209f856dd1f1e7c66618acf419be6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 3 Jul 2017 16:23:23 -0400 Subject: [PATCH 3/6] Document the new functions from the refactor --- src/or/circuitbuild.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index ee33361840..bf14d495bf 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -942,6 +942,12 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) } } +/** + * Called from circuit_send_next_onion_skin() when we find ourselves connected + * to the first hop in circ: Send a CREATE or CREATE2 or CREATE_FAST + * cell to that hop. Return 0 on success; -reason on failure (if the circuit + * should be torn down). + */ static int circuit_send_first_onion_skin(origin_circuit_t *circ) { @@ -1003,6 +1009,12 @@ circuit_send_first_onion_skin(origin_circuit_t *circ) return 0; } +/** + * Called from circuit_send_next_onion_skin() when we find that we have no + * more hops: mark the circuit as finished, and perform the necessary + * bookkeeping. Return 0 on success; -reason on failure (if the circuit + * should be torn down). + */ static int circuit_build_no_more_hops(origin_circuit_t *circ) { @@ -1109,6 +1121,12 @@ circuit_build_no_more_hops(origin_circuit_t *circ) return 0; } +/** + * Called from circuit_send_next_onion_skin() when we find that we have a hop + * other than the first that we need to extend to: use hop's + * information to extend the circuit another step. Return 0 on success; + * -reason on failure (if the circuit should be torn down). + */ static int circuit_send_intermediate_onion_skin(origin_circuit_t *circ, crypt_path_t *hop) From 69fba1f2cd0dae3408d8ebe2aee4c4ec426278db Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 3 Jul 2017 17:13:08 -0400 Subject: [PATCH 4/6] better comments and mild refactoring --- src/or/circuitbuild.c | 52 +++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index bf14d495bf..5fd199ca1f 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -916,30 +916,35 @@ circuit_purpose_may_omit_guard(int purpose) * If circ's first hop is closed, then we need to build a create * cell and send it forward. * - * Otherwise, we need to build a relay extend cell and send it - * forward. + * Otherwise, if circ's cpath still has any non-open hops, we need to + * build a relay extend cell and send it forward to the next non-open hop. + * + * If all hops on the cpath are open, we're done building the circuit + * and we should do housekeeping for the newly opened circuit. * * Return -reason if we want to tear down circ, else return 0. */ int circuit_send_next_onion_skin(origin_circuit_t *circ) { - tor_assert(circ); if (circ->cpath->state == CPATH_STATE_CLOSED) { + /* Case one: we're on the first hop. */ return circuit_send_first_onion_skin(circ); - } else { - tor_assert(circ->cpath->state == CPATH_STATE_OPEN); - tor_assert(circ->base_.state == CIRCUIT_STATE_BUILDING); - log_debug(LD_CIRC,"starting to send subsequent skin."); - crypt_path_t *hop = onion_next_hop_in_cpath(circ->cpath); - if (!hop) { - return circuit_build_no_more_hops(circ); - } else { - return circuit_send_intermediate_onion_skin(circ, hop); - } } + + tor_assert(circ->cpath->state == CPATH_STATE_OPEN); + tor_assert(circ->base_.state == CIRCUIT_STATE_BUILDING); + crypt_path_t *hop = onion_next_hop_in_cpath(circ->cpath); + + if (hop) { + /* Case two: we're on a hop after the first. */ + return circuit_send_intermediate_onion_skin(circ, hop); + } + + /* Case three: the circuit is finished. Do housekeeping tasks on it. */ + return circuit_build_no_more_hops(circ); } /** @@ -951,16 +956,17 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) static int circuit_send_first_onion_skin(origin_circuit_t *circ) { - const node_t *node; - /* This is the first hop. */ - create_cell_t cc; int fast; int len; - log_debug(LD_CIRC,"First skin; sending create cell."); + const node_t *node; + create_cell_t cc; memset(&cc, 0, sizeof(cc)); - if (circ->build_state->onehop_tunnel) + + log_debug(LD_CIRC,"First skin; sending create cell."); + + if (circ->build_state->onehop_tunnel) { control_event_bootstrap(BOOTSTRAP_STATUS_ONEHOP_CREATE, 0); - else { + } else { control_event_bootstrap(BOOTSTRAP_STATUS_CIRCUIT_CREATE, 0); /* If this is not a one-hop tunnel, the channel is being used @@ -1018,7 +1024,6 @@ circuit_send_first_onion_skin(origin_circuit_t *circ) static int circuit_build_no_more_hops(origin_circuit_t *circ) { - /* done building the circuit. whew. */ guard_usable_t r; if (! circ->guard_state) { if (circuit_get_cpath_len(circ) != 1 && @@ -1131,9 +1136,12 @@ static int circuit_send_intermediate_onion_skin(origin_circuit_t *circ, crypt_path_t *hop) { - extend_cell_t ec; int len; + extend_cell_t ec; memset(&ec, 0, sizeof(ec)); + + log_debug(LD_CIRC,"starting to send subsequent skin."); + if (tor_addr_family(&hop->extend_info->addr) != AF_INET) { log_warn(LD_BUG, "Trying to extend to a non-IPv4 address."); return - END_CIRC_REASON_INTERNAL; @@ -1172,7 +1180,7 @@ circuit_send_intermediate_onion_skin(origin_circuit_t *circ, return -END_CIRC_REASON_INTERNAL; } - /* send it to hop->prev, because it will transfer + /* send it to hop->prev, because that relay will transfer * it to a create cell and then send to hop */ if (relay_send_command_from_edge(0, TO_CIRCUIT(circ), command, From 943d284752c7c0d7075e7d7a21d5384e8335e95b Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 3 Jul 2017 17:16:26 -0400 Subject: [PATCH 5/6] CREATE_FAST is for when you don't know the onion key it isn't (anymore) for when you think you can get away with saving some crypto operations. --- src/or/circuitbuild.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 5fd199ca1f..8bb7c94d74 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -981,15 +981,11 @@ circuit_send_first_onion_skin(origin_circuit_t *circ) node = node_get_by_id(circ->base_.n_chan->identity_digest); fast = should_use_create_fast_for_circuit(circ); if (!fast) { - /* We are an OR and we know the right onion key: we should - * send a create cell. - */ + /* We know the right onion key: we should send a create cell. */ circuit_pick_create_handshake(&cc.cell_type, &cc.handshake_type, circ->cpath->extend_info); } else { - /* We are not an OR, and we're building the first hop of a circuit to a - * new OR: we can be speedy and use CREATE_FAST to save an RSA operation - * and a DH operation. */ + /* We don't know an onion key, so we need to fall back to CREATE_FAST. */ cc.cell_type = CELL_CREATE_FAST; cc.handshake_type = ONION_HANDSHAKE_TYPE_FAST; } From ef56f073c1fba23772e16f11d599d66a7f128ef8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 5 Jul 2017 10:01:40 -0400 Subject: [PATCH 6/6] changes file for 22804 --- changes/ticket22804 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket22804 diff --git a/changes/ticket22804 b/changes/ticket22804 new file mode 100644 index 0000000000..a5d71c5120 --- /dev/null +++ b/changes/ticket22804 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + + - Split the enormous circuit_send_next_onion_skin() function into + multiple subfunctions. Closes ticket 22804.