From 010779176bb959c8106a95806ede2c80b4397f60 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 11 May 2019 02:51:14 +0000 Subject: [PATCH 01/11] Bug 29085: Refactor non-padding accounting out of token removal. This commit moves the padding state limit checks and the padding rate limit checks out of the token removal codepath, and causes all three functions to get called from a single circpad_machine_count_nonpadding_sent() function. It does not change functionality. --- src/core/or/circuitpadding.c | 110 +++++++++++++++++++++++++---------- src/core/or/circuitpadding.h | 5 +- 2 files changed, 81 insertions(+), 34 deletions(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index 58e8e053c7..1386bd22c8 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -80,6 +80,9 @@ static void circpad_setup_machine_on_circ(circuit_t *on_circ, const circpad_machine_spec_t *machine); static double circpad_distribution_sample(circpad_distribution_t dist); +static inline void circpad_machine_update_state_length_for_nonpadding( + circpad_machine_runtime_t *mi); + /** Cached consensus params */ static uint8_t circpad_padding_disabled; static uint8_t circpad_padding_reduced; @@ -828,22 +831,15 @@ check_machine_token_supply(circpad_machine_runtime_t *mi) } /** - * Remove a token from the bin corresponding to the delta since - * last packet. If that bin is empty, choose a token based on - * the specified removal strategy in the state machine. + * Count a nonpadding packet as being sent. * - * This function also updates and checks rate limit and state - * limit counters. - * - * Returns 1 if we transition states, 0 otherwise. + * This function updates our overhead accounting variables, as well + * as decrements the state limit packet counter, if the latter was + * flagged as applying to non-padding as well. */ -STATIC circpad_decision_t -circpad_machine_remove_token(circpad_machine_runtime_t *mi) +static inline void +circpad_machine_count_nonpadding_sent(circpad_machine_runtime_t *mi) { - const circpad_state_t *state = NULL; - circpad_time_t current_time; - circpad_delay_t target_bin_usec; - /* Update non-padding counts for rate limiting: We scale at UINT16_MAX * because we only use this for a percentile limit of 2 sig figs, and * space is scare in the machineinfo struct. */ @@ -853,12 +849,67 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi) mi->nonpadding_sent /= 2; } + /* Update any state packet length limits that apply */ + circpad_machine_update_state_length_for_nonpadding(mi); + + /* Remove a token from the histrogram, if applicable */ + circpad_machine_remove_token(mi); +} + +/** + * Decrement the state length counter for a non-padding packet. + * + * Only updates the state length if we're using that feature, we + * have a state, and the machine wants to count non-padding packets + * towards the state length. + */ +static inline void +circpad_machine_update_state_length_for_nonpadding( + circpad_machine_runtime_t *mi) +{ + const circpad_state_t *state = NULL; + + if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE) + return; + + state = circpad_machine_current_state(mi); + + /* If we are not in a padding state (like start or end), we're done */ + if (!state) + return; + + /* If we're enforcing a state length on non-padding packets, + * decrement it */ + if (state->length_includes_nonpadding && + mi->state_length > 0) { + mi->state_length--; + } +} + +/** + * When a non-padding packet arrives, remove a token from the bin + * corresponding to the delta since last sent packet. If that bin + * is empty, choose a token based on the specified removal strategy + * in the state machine. + */ +STATIC void +circpad_machine_remove_token(circpad_machine_runtime_t *mi) +{ + const circpad_state_t *state = NULL; + circpad_time_t current_time; + circpad_delay_t target_bin_usec; + /* Dont remove any tokens if there was no padding scheduled */ if (!mi->padding_scheduled_at_usec) { - return CIRCPAD_STATE_UNCHANGED; + return; } state = circpad_machine_current_state(mi); + + /* Don't remove any tokens if we're not doing token removal */ + if (!state || state->token_removal == CIRCPAD_TOKEN_REMOVAL_NONE) + return; + current_time = monotime_absolute_usec(); /* If we have scheduled padding some time in the future, we want to see what @@ -877,20 +928,10 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi) /* If we are not in a padding state (like start or end), we're done */ if (!state) - return CIRCPAD_STATE_UNCHANGED; - - /* If we're enforcing a state length on non-padding packets, - * decrement it */ - if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE && - state->length_includes_nonpadding && - mi->state_length > 0) { - mi->state_length--; - } + return; /* Perform the specified token removal strategy */ switch (state->token_removal) { - case CIRCPAD_TOKEN_REMOVAL_NONE: - break; case CIRCPAD_TOKEN_REMOVAL_CLOSEST_USEC: circpad_machine_remove_closest_token(mi, target_bin_usec, 1); break; @@ -906,10 +947,13 @@ circpad_machine_remove_token(circpad_machine_runtime_t *mi) case CIRCPAD_TOKEN_REMOVAL_EXACT: circpad_machine_remove_exact(mi, target_bin_usec); break; + case CIRCPAD_TOKEN_REMOVAL_NONE: + default: + tor_assert_nonfatal_unreached(); + log_warn(LD_BUG, "Circpad: Unknown token removal strategy %d", + state->token_removal); + break; } - - /* Check our token and state length limits */ - return check_machine_token_supply(mi); } /** @@ -1541,9 +1585,13 @@ circpad_cell_event_nonpadding_sent(circuit_t *on_circ) /* First, update any RTT estimate */ circpad_estimate_circ_rtt_on_send(on_circ, on_circ->padding_info[i]); - /* Remove a token: this is the idea of adaptive padding, since we have an - * ideal distribution that we want our distribution to look like. */ - if (!circpad_machine_remove_token(on_circ->padding_info[i])) { + /* Then, do accounting */ + circpad_machine_count_nonpadding_sent(on_circ->padding_info[i]); + + /* Check to see if we've run out of tokens for this state already, + * and if not, check for other state transitions */ + if (check_machine_token_supply(on_circ->padding_info[i]) + == CIRCPAD_STATE_UNCHANGED) { /* If removing a token did not cause a transition, check if * non-padding sent event should */ circpad_machine_spec_transition(on_circ->padding_info[i], diff --git a/src/core/or/circuitpadding.h b/src/core/or/circuitpadding.h index f00369eb0a..7d0f8dacfa 100644 --- a/src/core/or/circuitpadding.h +++ b/src/core/or/circuitpadding.h @@ -712,9 +712,6 @@ circpad_machine_sample_delay(circpad_machine_runtime_t *mi); STATIC bool circpad_machine_reached_padding_limit(circpad_machine_runtime_t *mi); -STATIC -circpad_decision_t circpad_machine_remove_token(circpad_machine_runtime_t *mi); - STATIC circpad_delay_t circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi, circpad_hist_index_t bin); @@ -722,6 +719,8 @@ circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi, STATIC const circpad_state_t * circpad_machine_current_state(const circpad_machine_runtime_t *mi); +STATIC void circpad_machine_remove_token(circpad_machine_runtime_t *mi); + STATIC circpad_hist_index_t circpad_histogram_usec_to_bin( const circpad_machine_runtime_t *mi, circpad_delay_t us); From 1c46790e0d3d6c722a0f3e74f24d60d03650e7d8 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 11 May 2019 03:26:53 +0000 Subject: [PATCH 02/11] Bug 29085: Refactor padding sent accounting out of callback. This commit moves code that updates the state length and padding limit counts out from the callback to its own function, for clarity. It does not change functionality. --- src/core/or/circuitpadding.c | 73 ++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index 1386bd22c8..d6a79004f5 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -830,6 +830,50 @@ check_machine_token_supply(circpad_machine_runtime_t *mi) return CIRCPAD_STATE_UNCHANGED; } +/** + * Count that a padding packet was sent. + * + * This updates our state length count, our machine rate limit counts, + * and if token removal is used, decrements the histogram. + */ +static inline void +circpad_machine_count_padding_sent(circpad_machine_runtime_t *mi) +{ + /* If we have a valid state length bound, consider it */ + if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE && + !BUG(mi->state_length <= 0)) { + mi->state_length--; + } + + /* + * Update non-padding counts for rate limiting: We scale at UINT16_MAX + * because we only use this for a percentile limit of 2 sig figs, and + * space is scare in the machineinfo struct. + */ + mi->padding_sent++; + if (mi->padding_sent == UINT16_MAX) { + mi->padding_sent /= 2; + mi->nonpadding_sent /= 2; + } + + circpad_global_padding_sent++; + + /* If we have a mutable histogram, reduce the token count from + * the chosen padding bin (this assumes we always send padding + * when we intended to). */ + if (mi->histogram && mi->histogram_len) { + /* Ensure that we have a token removal strategy set */ + const circpad_state_t *state = circpad_machine_current_state(mi); + tor_assert_nonfatal(state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE); + + /* Basic sanity check on the histogram before removing anything */ + if (!BUG(mi->chosen_bin >= mi->histogram_len) && + !BUG(mi->histogram[mi->chosen_bin] == 0)) { + mi->histogram[mi->chosen_bin]--; + } + } +} + /** * Count a nonpadding packet as being sent. * @@ -1023,34 +1067,7 @@ circpad_send_padding_cell_for_callback(circpad_machine_runtime_t *mi) return CIRCPAD_STATE_CHANGED; } - /* If it's a histogram, reduce the token count */ - if (mi->histogram && mi->histogram_len) { - /* Basic sanity check on the histogram before removing anything */ - if (BUG(mi->chosen_bin >= mi->histogram_len) || - BUG(mi->histogram[mi->chosen_bin] == 0)) { - return CIRCPAD_STATE_CHANGED; - } - - mi->histogram[mi->chosen_bin]--; - } - - /* If we have a valid state length bound, consider it */ - if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE && - !BUG(mi->state_length <= 0)) { - mi->state_length--; - } - - /* - * Update non-padding counts for rate limiting: We scale at UINT16_MAX - * because we only use this for a percentile limit of 2 sig figs, and - * space is scare in the machineinfo struct. - */ - mi->padding_sent++; - if (mi->padding_sent == UINT16_MAX) { - mi->padding_sent /= 2; - mi->nonpadding_sent /= 2; - } - circpad_global_padding_sent++; + circpad_machine_count_padding_sent(mi); if (CIRCUIT_IS_ORIGIN(mi->on_circ)) { circpad_send_command_to_hop(TO_ORIGIN_CIRCUIT(mi->on_circ), From 57e5e940d39754ec5b3ca1c7055ff7ca9a47c238 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 7 May 2019 23:37:33 +0000 Subject: [PATCH 03/11] Bug 29085: Minor unit test updates for refactoring. Deliver nonpadding events instead of calling token removal functions. --- src/test/test_circuitpadding.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index db175fecee..b7dbf022aa 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -586,12 +586,12 @@ test_circuitpadding_token_removal_higher(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -610,7 +610,7 @@ test_circuitpadding_token_removal_higher(void *arg) CIRCPAD_STATE_BURST); circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100; mi->padding_scheduled_at_usec = current_time; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[0], OP_EQ, 1); done: @@ -683,12 +683,12 @@ test_circuitpadding_token_removal_lower(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -708,7 +708,7 @@ test_circuitpadding_token_removal_lower(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST]. histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -780,12 +780,12 @@ test_circuitpadding_closest_token_removal(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -807,14 +807,14 @@ test_circuitpadding_closest_token_removal(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[1], OP_EQ, 1); /* Test above the highest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); mi->padding_scheduled_at_usec = current_time - 29202; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -889,12 +889,12 @@ test_circuitpadding_closest_token_removal_usec(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -916,7 +916,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[1], OP_EQ, 1); /* Test above the highest bin, for coverage */ @@ -925,7 +925,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST]. histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -971,16 +971,16 @@ test_circuitpadding_token_removal_exact(void *arg) /* Ensure that we will clear out bin #4 with this usec */ mi->padding_scheduled_at_usec = current_time - 57; tt_int_op(mi->histogram[4], OP_EQ, 2); - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); mi->padding_scheduled_at_usec = current_time - 57; tt_int_op(mi->histogram[4], OP_EQ, 1); - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); tt_int_op(mi->histogram[4], OP_EQ, 0); /* Ensure that we will not remove any other tokens even tho we try to, since * this is what the exact strategy dictates */ mi->padding_scheduled_at_usec = current_time - 57; - circpad_machine_remove_token(mi); + circpad_cell_event_nonpadding_sent((circuit_t*)client_side); for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { if (i != 4) { tt_int_op(mi->histogram[i], OP_EQ, 2); From 5c2d2b5d11b72fac01c5958f18c83df252f5313d Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 11 May 2019 03:12:33 +0000 Subject: [PATCH 04/11] Make the relationship between mutable histograms and token removal explicit. --- src/core/or/circuitpadding.c | 65 +++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index d6a79004f5..eeedbc124f 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -330,6 +330,39 @@ circpad_histogram_usec_to_bin(const circpad_machine_runtime_t *mi, return CIRCPAD_INFINITY_BIN(state)-1; } +/** + * Return true if the machine supports token removal. + * + * Token removal is equivalent to having a mutable histogram in the + * circpad_machine_runtime_t mutable info. So while we're at it, + * let's assert that everything is consistent between the mutable + * runtime and the readonly machine spec. + */ +static inline int +circpad_is_token_removal_supported(circpad_machine_runtime_t *mi) +{ + /* No runtime histogram == no token removal */ + if (mi->histogram == NULL) { + /* Machines that don't want token removal are trying to avoid + * potentially expensive mallocs, extra memory accesses, and/or + * potentially expensive monotime calls. Let's minimize checks + * and keep this path fast. */ + tor_assert_nonfatal(mi->histogram_len == 0); + return 0; + } else { + /* Machines that do want token removal are less sensitive to performance. + * Let's spend some time to check that our state is consistent and sane */ + const circpad_state_t *state = circpad_machine_current_state(mi); + tor_assert_nonfatal(state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE); + tor_assert_nonfatal(state->histogram_len == mi->histogram_len); + tor_assert_nonfatal(mi->histogram_len != 0); + return 1; + } + + tor_assert_nonfatal_unreached(); + return 0; +} + /** * This function frees any token bins allocated from a previous state * @@ -441,13 +474,7 @@ circpad_machine_sample_delay(circpad_machine_runtime_t *mi) mi->rtt_estimate_usec + state->dist_added_shift_usec : state->dist_added_shift_usec; return circpad_distribution_sample_iat_delay(state, iat_delay_shift); - } else if (state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE) { - /* We have a mutable histogram. Do basic sanity check and apply: */ - if (BUG(!mi->histogram) || - BUG(mi->histogram_len != state->histogram_len)) { - return CIRCPAD_DELAY_INFINITE; - } - + } else if (circpad_is_token_removal_supported(mi)) { histogram = mi->histogram; for (circpad_hist_index_t b = 0; b < state->histogram_len; b++) histogram_total_tokens += histogram[b]; @@ -812,7 +839,7 @@ check_machine_token_supply(circpad_machine_runtime_t *mi) * * We also do not count infinity bin in histogram totals. */ - if (mi->histogram_len && mi->histogram) { + if (circpad_is_token_removal_supported(mi)) { for (circpad_hist_index_t b = 0; b < CIRCPAD_INFINITY_BIN(mi); b++) histogram_total_tokens += mi->histogram[b]; @@ -861,12 +888,8 @@ circpad_machine_count_padding_sent(circpad_machine_runtime_t *mi) /* If we have a mutable histogram, reduce the token count from * the chosen padding bin (this assumes we always send padding * when we intended to). */ - if (mi->histogram && mi->histogram_len) { - /* Ensure that we have a token removal strategy set */ - const circpad_state_t *state = circpad_machine_current_state(mi); - tor_assert_nonfatal(state->token_removal != CIRCPAD_TOKEN_REMOVAL_NONE); - - /* Basic sanity check on the histogram before removing anything */ + if (circpad_is_token_removal_supported(mi)) { + /* Check array bounds and token count before removing */ if (!BUG(mi->chosen_bin >= mi->histogram_len) && !BUG(mi->histogram[mi->chosen_bin] == 0)) { mi->histogram[mi->chosen_bin]--; @@ -896,7 +919,7 @@ circpad_machine_count_nonpadding_sent(circpad_machine_runtime_t *mi) /* Update any state packet length limits that apply */ circpad_machine_update_state_length_for_nonpadding(mi); - /* Remove a token from the histrogram, if applicable */ + /* Remove a token from the histogram, if applicable */ circpad_machine_remove_token(mi); } @@ -1301,7 +1324,17 @@ circpad_machine_schedule_padding,(circpad_machine_runtime_t *mi)) /* in_usec = in microseconds */ in_usec = circpad_machine_sample_delay(mi); - mi->padding_scheduled_at_usec = monotime_absolute_usec(); + /* If we're using token removal, we need to know when the padding + * was scheduled at, so we can remove the appropriate token if + * a non-padding cell is sent before the padding timer expires. + * + * However, since monotime is unpredictably expensive, let's avoid + * using it for machines that don't need token removal. */ + if (circpad_is_token_removal_supported(mi)) { + mi->padding_scheduled_at_usec = monotime_absolute_usec(); + } else { + mi->padding_scheduled_at_usec = 1; + } log_fn(LOG_INFO,LD_CIRC,"\tPadding in %u usec", in_usec); // Don't schedule if we have infinite delay. From 14ec8b89f83ea9cc4fbc7b8bd24194f5408cfea4 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 8 May 2019 00:40:49 +0000 Subject: [PATCH 05/11] Bug 29085: Avoid monotime usage for rtt estimates if it is not in use. --- src/core/or/circuitpadding.c | 25 +++++++++++++++++++++---- src/test/test_circuitpadding.c | 1 + 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index eeedbc124f..7674f3510c 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -1547,10 +1547,26 @@ circpad_estimate_circ_rtt_on_received(circuit_t *circ, ", %d) after two back to back packets. Current RTT: %d", circ->n_chan ? circ->n_chan->global_identifier : 0, circ->n_circ_id, mi->rtt_estimate_usec); - mi->stop_rtt_update = 1; + mi->stop_rtt_update = 1; + + if (!mi->rtt_estimate_usec) { + static ratelim_t rtt_lim = RATELIM_INIT(600); + log_fn_ratelim(&rtt_lim,LOG_NOTICE,LD_BUG, + "Circuit got two cells back to back before estimating RTT."); + } } } else { - mi->last_received_time_usec = monotime_absolute_usec(); + const circpad_state_t *state = circpad_machine_current_state(mi); + + /* Since monotime is unpredictably expensive, only update this field + * if rtt estimates are needed. Otherwise, stop the rtt update. */ + if (state->use_rtt_estimate) { + mi->last_received_time_usec = monotime_absolute_usec(); + } else { + /* Let's fast-path future decisions not to update rtt if the + * feature is not in use. */ + mi->stop_rtt_update = 1; + } } } @@ -1610,8 +1626,9 @@ circpad_estimate_circ_rtt_on_send(circuit_t *circ, mi->stop_rtt_update = 1; if (!mi->rtt_estimate_usec) { - log_fn(LOG_NOTICE, LD_CIRC, - "Got two cells back to back on a circuit before estimating RTT."); + static ratelim_t rtt_lim = RATELIM_INIT(600); + log_fn_ratelim(&rtt_lim,LOG_NOTICE,LD_BUG, + "Circuit sent two cells back to back before estimating RTT."); } } } diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index b7dbf022aa..13371df687 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -415,6 +415,7 @@ helper_create_basic_machine(void) circ_client_machine.states[CIRCPAD_STATE_START]. next_state[CIRCPAD_EVENT_NONPADDING_RECV] = CIRCPAD_STATE_BURST; + circ_client_machine.states[CIRCPAD_STATE_START].use_rtt_estimate = 1; circ_client_machine.states[CIRCPAD_STATE_BURST]. next_state[CIRCPAD_EVENT_PADDING_RECV] = CIRCPAD_STATE_BURST; From 5ef0d89d912c2b4cbf42510df5f609b3704ede73 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 8 May 2019 00:50:52 +0000 Subject: [PATCH 06/11] Bug 29085: Changes file. --- changes/bug29085 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug29085 diff --git a/changes/bug29085 b/changes/bug29085 new file mode 100644 index 0000000000..b17c06378f --- /dev/null +++ b/changes/bug29085 @@ -0,0 +1,4 @@ + o Code simplification and refactoring (circuit padding): + - Avoid calling monotime_absolute_usec() in circuit padding machines + that do not use token removal or circuit RTT estimation. Fixes bug + 29085; bugfix on 0.4.0.1-alpha. From aef9be6ace841e89c47c53a6c419fa9c1a223cd2 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 14 May 2019 22:25:15 +0000 Subject: [PATCH 07/11] Eliminate unneeded casts to circuit_t in circpad tests. --- src/test/test_circuitpadding.c | 114 ++++++++++++++++----------------- 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index 13371df687..b19bbcc94d 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -329,12 +329,12 @@ test_circuitpadding_rtt(void *arg) relay_side->padding_info[0] = circpad_circuit_machineinfo_new(client_side,0); /* Test 1: Test measuring RTT */ - circpad_cell_event_nonpadding_received((circuit_t*)relay_side); + circpad_cell_event_nonpadding_received(relay_side); tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_NE, 0); timers_advance_and_run(20); - circpad_cell_event_nonpadding_sent((circuit_t*)relay_side); + circpad_cell_event_nonpadding_sent(relay_side); tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_EQ, 0); tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_GE, 19000); @@ -345,12 +345,12 @@ test_circuitpadding_rtt(void *arg) circpad_machine_current_state( relay_side->padding_info[0])->histogram_edges[0]); - circpad_cell_event_nonpadding_received((circuit_t*)relay_side); - circpad_cell_event_nonpadding_received((circuit_t*)relay_side); + circpad_cell_event_nonpadding_received(relay_side); + circpad_cell_event_nonpadding_received(relay_side); tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_NE, 0); timers_advance_and_run(20); - circpad_cell_event_nonpadding_sent((circuit_t*)relay_side); - circpad_cell_event_nonpadding_sent((circuit_t*)relay_side); + circpad_cell_event_nonpadding_sent(relay_side); + circpad_cell_event_nonpadding_sent(relay_side); tt_u64_op(relay_side->padding_info[0]->last_received_time_usec, OP_EQ, 0); tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_GE, 20000); @@ -365,9 +365,9 @@ test_circuitpadding_rtt(void *arg) tt_int_op(relay_side->padding_info[0]->stop_rtt_update, OP_EQ, 1); rtt_estimate = relay_side->padding_info[0]->rtt_estimate_usec; - circpad_cell_event_nonpadding_received((circuit_t*)relay_side); + circpad_cell_event_nonpadding_received(relay_side); timers_advance_and_run(4); - circpad_cell_event_nonpadding_sent((circuit_t*)relay_side); + circpad_cell_event_nonpadding_sent(relay_side); tt_int_op(relay_side->padding_info[0]->rtt_estimate_usec, OP_EQ, rtt_estimate); @@ -380,11 +380,11 @@ test_circuitpadding_rtt(void *arg) relay_side->padding_info[0])->histogram_edges[0]); /* Test 3: Make sure client side machine properly ignores RTT */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_u64_op(client_side->padding_info[0]->last_received_time_usec, OP_EQ, 0); timers_advance_and_run(20); - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_u64_op(client_side->padding_info[0]->last_received_time_usec, OP_EQ, 0); tt_int_op(client_side->padding_info[0]->rtt_estimate_usec, OP_EQ, 0); @@ -527,7 +527,7 @@ test_circuitpadding_token_removal_higher(void *arg) MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); /* Setup test environment (time etc.) */ - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; monotime_enable_test_mocking(); @@ -538,7 +538,7 @@ test_circuitpadding_token_removal_higher(void *arg) circpad_circuit_machineinfo_new(client_side, 0); /* move the machine to the right state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); @@ -587,12 +587,12 @@ test_circuitpadding_token_removal_higher(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -611,7 +611,7 @@ test_circuitpadding_token_removal_higher(void *arg) CIRCPAD_STATE_BURST); circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[0] = 100; mi->padding_scheduled_at_usec = current_time; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[0], OP_EQ, 1); done: @@ -632,7 +632,7 @@ test_circuitpadding_token_removal_lower(void *arg) MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); /* Setup test environment (time etc.) */ - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; monotime_enable_test_mocking(); @@ -643,7 +643,7 @@ test_circuitpadding_token_removal_lower(void *arg) circpad_circuit_machineinfo_new(client_side, 0); /* move the machine to the right state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); @@ -684,12 +684,12 @@ test_circuitpadding_token_removal_lower(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -709,7 +709,7 @@ test_circuitpadding_token_removal_lower(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST]. histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -730,7 +730,7 @@ test_circuitpadding_closest_token_removal(void *arg) MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); /* Setup test environment (time etc.) */ - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; monotime_enable_test_mocking(); @@ -741,7 +741,7 @@ test_circuitpadding_closest_token_removal(void *arg) circpad_circuit_machineinfo_new(client_side, 0); /* move the machine to the right state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); @@ -781,12 +781,12 @@ test_circuitpadding_closest_token_removal(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -808,14 +808,14 @@ test_circuitpadding_closest_token_removal(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[1], OP_EQ, 1); /* Test above the highest bin, for coverage */ tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); mi->padding_scheduled_at_usec = current_time - 29202; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -836,7 +836,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); /* Setup test environment (time etc.) */ - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; monotime_enable_test_mocking(); @@ -847,7 +847,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) circpad_circuit_machineinfo_new(client_side, 0); /* move the machine to the right state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); @@ -890,12 +890,12 @@ test_circuitpadding_closest_token_removal_usec(void *arg) tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 2); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[bin_to_remove], OP_EQ, 1); mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); /* Test that we cleaned out this bin. Don't do this in the case of the last bin since the tokens will get refilled */ @@ -917,7 +917,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST].histogram_edges[2] = 120; mi->padding_scheduled_at_usec = current_time - 102; mi->histogram[0] = 0; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[1], OP_EQ, 1); /* Test above the highest bin, for coverage */ @@ -926,7 +926,7 @@ test_circuitpadding_closest_token_removal_usec(void *arg) circ_client_machine.states[CIRCPAD_STATE_BURST]. histogram_edges[BIG_HISTOGRAM_LEN-2] = 100; mi->padding_scheduled_at_usec = current_time - 29202; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[BIG_HISTOGRAM_LEN-2], OP_EQ, 1); done: @@ -947,7 +947,7 @@ test_circuitpadding_token_removal_exact(void *arg) MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); /* Setup test environment (time etc.) */ - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; monotime_enable_test_mocking(); @@ -958,7 +958,7 @@ test_circuitpadding_token_removal_exact(void *arg) circpad_circuit_machineinfo_new(client_side, 0); /* move the machine to the right state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); tt_int_op(client_side->padding_info[0]->current_state, OP_EQ, CIRCPAD_STATE_BURST); @@ -972,16 +972,16 @@ test_circuitpadding_token_removal_exact(void *arg) /* Ensure that we will clear out bin #4 with this usec */ mi->padding_scheduled_at_usec = current_time - 57; tt_int_op(mi->histogram[4], OP_EQ, 2); - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); mi->padding_scheduled_at_usec = current_time - 57; tt_int_op(mi->histogram[4], OP_EQ, 1); - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); tt_int_op(mi->histogram[4], OP_EQ, 0); /* Ensure that we will not remove any other tokens even tho we try to, since * this is what the exact strategy dictates */ mi->padding_scheduled_at_usec = current_time - 57; - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); for (int i = 0; i < BIG_HISTOGRAM_LEN ; i++) { if (i != 4) { tt_int_op(mi->histogram[i], OP_EQ, 2); @@ -1047,8 +1047,8 @@ test_circuitpadding_tokens(void *arg) mi = client_side->padding_info[0]; // Pretend a non-padding cell was sent - circpad_cell_event_nonpadding_received((circuit_t*)client_side); - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); + circpad_cell_event_nonpadding_sent(client_side); /* We have to save the infinity bin because one inf delay * could have been chosen when we transition to burst */ circpad_hist_token_t inf_bin = mi->histogram[4]; @@ -1157,11 +1157,11 @@ test_circuitpadding_tokens(void *arg) /* Drain the infinity bin and cause a refill */ while (inf_bin != 0) { tt_int_op(mi->histogram[4], OP_EQ, inf_bin); - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); inf_bin--; } - circpad_cell_event_nonpadding_sent((circuit_t*)client_side); + circpad_cell_event_nonpadding_sent(client_side); // We should have refilled here. tt_int_op(mi->histogram[4], OP_EQ, 2); @@ -1285,10 +1285,10 @@ test_circuitpadding_wronghop(void *arg) * padding that gets sent by scheduled timers. */ MOCK(circpad_machine_schedule_padding,circpad_machine_schedule_padding_mock); - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); dummy_channel.cmux = circuitmux_alloc(); - relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel, - &dummy_channel); + relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, + &dummy_channel)); orig_client = TO_ORIGIN_CIRCUIT(client_side); relay_side->purpose = CIRCUIT_PURPOSE_OR; @@ -1406,9 +1406,9 @@ test_circuitpadding_wronghop(void *arg) free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side)); free_fake_orcirc(relay_side); - client_side = (circuit_t *)origin_circuit_new(); - relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel, - &dummy_channel); + client_side = TO_CIRCUIT(origin_circuit_new()); + relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, + &dummy_channel)); relay_side->purpose = CIRCUIT_PURPOSE_OR; client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; @@ -1602,10 +1602,10 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay, tor_addr_t addr; // Pretend a non-padding cell was sent - circpad_cell_event_nonpadding_sent((circuit_t*)client); + circpad_cell_event_nonpadding_sent(client); // Receive extend cell at middle - circpad_cell_event_nonpadding_received((circuit_t*)mid_relay); + circpad_cell_event_nonpadding_received(mid_relay); // Advance time a tiny bit so we can calculate an RTT curr_mocked_time += 10 * TOR_NSEC_PER_MSEC; @@ -1613,10 +1613,10 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay, monotime_set_mock_time_nsec(curr_mocked_time); // Receive extended cell at middle - circpad_cell_event_nonpadding_sent((circuit_t*)mid_relay); + circpad_cell_event_nonpadding_sent(mid_relay); // Receive extended cell at first hop - circpad_cell_event_nonpadding_received((circuit_t*)client); + circpad_cell_event_nonpadding_received(client); // Add a hop to cpath crypt_path_t *hop = tor_malloc_zero(sizeof(crypt_path_t)); @@ -1762,9 +1762,9 @@ test_circuitpadding_conditions(void *arg) nodes_init(); dummy_channel.cmux = circuitmux_alloc(); - relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel, - &dummy_channel); - client_side = (circuit_t *)origin_circuit_new(); + relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, + &dummy_channel)); + client_side = TO_CIRCUIT(origin_circuit_new()); relay_side->purpose = CIRCUIT_PURPOSE_OR; client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; @@ -2218,7 +2218,7 @@ test_circuitpadding_sample_distribution(void *arg) } /* send a non-padding cell to move to the next machine state */ - circpad_cell_event_nonpadding_received((circuit_t*)client_side); + circpad_cell_event_nonpadding_received(client_side); } done: @@ -2330,12 +2330,12 @@ test_circuitpadding_global_rate_limiting(void *arg) curr_mocked_time = actual_mocked_monotime_start; timers_initialize(); - client_side = (circuit_t *)origin_circuit_new(); + client_side = TO_CIRCUIT(origin_circuit_new()); client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; dummy_channel.cmux = circuitmux_alloc(); /* Setup machine and circuits */ - relay_side = (circuit_t *)new_fake_orcirc(&dummy_channel, &dummy_channel); + relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, &dummy_channel)); relay_side->purpose = CIRCUIT_PURPOSE_OR; helper_create_basic_machine(); relay_side->padding_machine[0] = &circ_client_machine; From e8a1f241783878a242ebb153119af06631fe7cdb Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 03:45:03 +0000 Subject: [PATCH 08/11] Add test to explicitly check state lengths and token counts. Our other tests tested state lengths against padding packets, and token counts against non-padding packets. This test checks state lengths against non-padding packets (and also padding packets too), and checks token counts against padding packets (and also non-padding packets too). The next three commits are needed to make this test pass (it found 3 bugs). Yay? --- src/test/test_circuitpadding.c | 180 +++++++++++++++++++++++++++++++++ 1 file changed, 180 insertions(+) diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index b19bbcc94d..b2ae659a60 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -55,6 +55,7 @@ void test_circuitpadding_conditions(void *arg); void test_circuitpadding_serialize(void *arg); void test_circuitpadding_rtt(void *arg); void test_circuitpadding_tokens(void *arg); +void test_circuitpadding_state_length(void *arg); static void simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay, @@ -1643,6 +1644,55 @@ simulate_single_hop_extend(circuit_t *client, circuit_t *mid_relay, circpad_machine_event_circ_added_hop(TO_ORIGIN_CIRCUIT(client)); } +static circpad_machine_spec_t * +helper_create_length_machine(void) +{ + circpad_machine_spec_t *ret = + tor_malloc_zero(sizeof(circpad_machine_spec_t)); + + /* Start, burst */ + circpad_machine_states_init(ret, 2); + + ret->states[CIRCPAD_STATE_START]. + next_state[CIRCPAD_EVENT_PADDING_SENT] = CIRCPAD_STATE_BURST; + + ret->states[CIRCPAD_STATE_BURST]. + next_state[CIRCPAD_EVENT_PADDING_SENT] = CIRCPAD_STATE_BURST; + + ret->states[CIRCPAD_STATE_BURST]. + next_state[CIRCPAD_EVENT_LENGTH_COUNT] = CIRCPAD_STATE_END; + + ret->states[CIRCPAD_STATE_BURST]. + next_state[CIRCPAD_EVENT_BINS_EMPTY] = CIRCPAD_STATE_END; + + /* No token removal.. end via state_length only */ + ret->states[CIRCPAD_STATE_BURST].token_removal = + CIRCPAD_TOKEN_REMOVAL_NONE; + + /* Let's have this one end after 12 packets */ + ret->states[CIRCPAD_STATE_BURST].length_dist.type = CIRCPAD_DIST_UNIFORM; + ret->states[CIRCPAD_STATE_BURST].length_dist.param1 = 12; + ret->states[CIRCPAD_STATE_BURST].length_dist.param2 = 13; + ret->states[CIRCPAD_STATE_BURST].max_length = 12; + + ret->states[CIRCPAD_STATE_BURST].histogram_len = 4; + + ret->states[CIRCPAD_STATE_BURST].histogram_edges[0] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram_edges[1] = 1; + ret->states[CIRCPAD_STATE_BURST].histogram_edges[2] = 1000000; + ret->states[CIRCPAD_STATE_BURST].histogram_edges[3] = 10000000; + + ret->states[CIRCPAD_STATE_BURST].histogram[0] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram[1] = 0; + ret->states[CIRCPAD_STATE_BURST].histogram[2] = 6; + + ret->states[CIRCPAD_STATE_BURST].histogram_total_tokens = 6; + ret->states[CIRCPAD_STATE_BURST].use_rtt_estimate = 0; + ret->states[CIRCPAD_STATE_BURST].length_includes_nonpadding = 0; + + return ret; +} + static circpad_machine_spec_t * helper_create_conditional_machine(void) { @@ -1738,6 +1788,135 @@ helper_create_conditional_machines(void) register_padding_machine(add, relay_padding_machines); } +void +test_circuitpadding_state_length(void *arg) +{ + /** + * Test plan: + * * Explicitly test that with no token removal enabled, we hit + * the state length limit due to either padding, or non-padding. + * * Repeat test with an arbitrary token removal strategy, and + * verify that if we run out of tokens due to padding before we + * hit the state length, we still go to state end (all our + * token removal tests only test nonpadding token removal). + */ + int64_t actual_mocked_monotime_start; + (void)arg; + MOCK(circuitmux_attach_circuit, circuitmux_attach_circuit_mock); + MOCK(circpad_send_command_to_hop, circpad_send_command_to_hop_mock); + + nodes_init(); + dummy_channel.cmux = circuitmux_alloc(); + relay_side = TO_CIRCUIT(new_fake_orcirc(&dummy_channel, + &dummy_channel)); + client_side = TO_CIRCUIT(origin_circuit_new()); + relay_side->purpose = CIRCUIT_PURPOSE_OR; + client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; + + monotime_init(); + monotime_enable_test_mocking(); + actual_mocked_monotime_start = MONOTIME_MOCK_START; + monotime_set_mock_time_nsec(actual_mocked_monotime_start); + monotime_coarse_set_mock_time_nsec(actual_mocked_monotime_start); + curr_mocked_time = actual_mocked_monotime_start; + + /* This is needed so that we are not considered to be dormant */ + note_user_activity(20); + + timers_initialize(); + circpad_machine_spec_t *client_machine = + helper_create_length_machine(); + + MOCK(circuit_package_relay_cell, + circuit_package_relay_cell_mock); + MOCK(node_get_by_id, + node_get_by_id_mock); + + client_side->padding_machine[0] = client_machine; + client_side->padding_info[0] = + circpad_circuit_machineinfo_new(client_side, 0); + circpad_machine_runtime_t *mi = client_side->padding_info[0]; + + circpad_cell_event_padding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, 12); + tt_ptr_op(mi->histogram, OP_EQ, NULL); + + /* Verify that non-padding does not change our state length */ + circpad_cell_event_nonpadding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, 12); + + /* verify that sending padding changes our state length */ + for (uint64_t i = mi->state_length-1; i > 0; i--) { + circpad_send_padding_cell_for_callback(mi); + tt_int_op(mi->state_length, OP_EQ, i); + } + circpad_send_padding_cell_for_callback(mi); + + tt_int_op(mi->state_length, OP_EQ, -1); + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END); + + /* Restart machine */ + mi->current_state = CIRCPAD_STATE_START; + + /* Now, count nonpadding as part of the state length */ + client_machine->states[CIRCPAD_STATE_BURST].length_includes_nonpadding = 1; + + circpad_cell_event_padding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, 12); + + /* Verify that non-padding does change our state length now */ + for (uint64_t i = mi->state_length-1; i > 0; i--) { + circpad_cell_event_nonpadding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, i); + } + + circpad_cell_event_nonpadding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, -1); + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END); + + /* Now, just test token removal when we send padding */ + client_machine->states[CIRCPAD_STATE_BURST].token_removal = + CIRCPAD_TOKEN_REMOVAL_EXACT; + + /* Restart machine */ + mi->current_state = CIRCPAD_STATE_START; + circpad_cell_event_padding_sent(client_side); + tt_int_op(mi->state_length, OP_EQ, 12); + tt_ptr_op(mi->histogram, OP_NE, NULL); + tt_int_op(mi->chosen_bin, OP_EQ, 2); + + /* verify that sending padding changes our state length and + * our histogram now */ + for (uint32_t i = mi->histogram[2]-1; i > 0; i--) { + circpad_send_padding_cell_for_callback(mi); + tt_int_op(mi->chosen_bin, OP_EQ, 2); + tt_int_op(mi->histogram[2], OP_EQ, i); + } + + tt_int_op(mi->state_length, OP_EQ, 7); + tt_int_op(mi->histogram[2], OP_EQ, 1); + + circpad_send_padding_cell_for_callback(mi); + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END); + + done: + tor_free(client_machine->states); + tor_free(client_machine); + + free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side)); + free_fake_orcirc(relay_side); + + circuitmux_detach_all_circuits(dummy_channel.cmux, NULL); + circuitmux_free(dummy_channel.cmux); + timers_shutdown(); + monotime_disable_test_mocking(); + UNMOCK(circuit_package_relay_cell); + UNMOCK(circuitmux_attach_circuit); + UNMOCK(node_get_by_id); + + return; +} + void test_circuitpadding_conditions(void *arg) { @@ -2556,6 +2735,7 @@ test_circuitpadding_reduce_disable(void *arg) struct testcase_t circuitpadding_tests[] = { TEST_CIRCUITPADDING(circuitpadding_tokens, TT_FORK), + TEST_CIRCUITPADDING(circuitpadding_state_length, TT_FORK), TEST_CIRCUITPADDING(circuitpadding_negotiation, TT_FORK), TEST_CIRCUITPADDING(circuitpadding_wronghop, TT_FORK), /** Disabled unstable test until #29298 is implemented (see #29122) */ From 148c2d5bab051eb60e802fc6569c84a875c5f663 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 03:33:53 +0000 Subject: [PATCH 09/11] Fix two typo bugs found by new state length test. --- src/core/or/circuitpadding.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index 7674f3510c..3124e74033 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -253,7 +253,7 @@ circpad_histogram_bin_to_usec(const circpad_machine_runtime_t *mi, /* The infinity bin has an upper bound of infinity, so make sure we return * that if they ask for it. */ - if (bin > CIRCPAD_INFINITY_BIN(mi)) { + if (bin > CIRCPAD_INFINITY_BIN(state)) { return CIRCPAD_DELAY_INFINITE; } @@ -936,7 +936,7 @@ circpad_machine_update_state_length_for_nonpadding( { const circpad_state_t *state = NULL; - if (mi->state_length != CIRCPAD_STATE_LENGTH_INFINITE) + if (mi->state_length == CIRCPAD_STATE_LENGTH_INFINITE) return; state = circpad_machine_current_state(mi); From 5638d65f792393a85703342bfb14cc1a60550c18 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 03:43:57 +0000 Subject: [PATCH 10/11] Check the token supply when we received a padding event, too. We need to check here because otherwise we can try to schedule padding with no tokens left upon the receipt of a padding event when our bins just became empty. --- src/core/or/circuitpadding.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index 3124e74033..cd58a191d6 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -1699,8 +1699,16 @@ void circpad_cell_event_padding_sent(circuit_t *on_circ) { FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) { - circpad_machine_spec_transition(on_circ->padding_info[i], + /* Check to see if we've run out of tokens for this state already, + * and if not, check for other state transitions */ + if (check_machine_token_supply(on_circ->padding_info[i]) + == CIRCPAD_STATE_UNCHANGED) { + /* If removing a token did not cause a transition, check if + * non-padding sent event should */ + + circpad_machine_spec_transition(on_circ->padding_info[i], CIRCPAD_EVENT_PADDING_SENT); + } } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END; } From 5f47d582d50134d9504ec8e7a912887829904c8a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 04:58:21 +0000 Subject: [PATCH 11/11] Practracker beatings are even more fun when they get caused mid-PR due to a github auto-rebase of a PR Because github PRs choose the most recent origin/master at the time of the PR (and for any fixups pushed to a PR later to send to CI), there are tons of conflicts and unexpected practracker issues. This means CI can suddenly fail after fixups to a branch that pass locally. Then CI fails and we have to close and re-open the PR. --- scripts/maint/practracker/exceptions.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 24452b6158..b380d507b1 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -289,3 +289,4 @@ problem function-size /src/tools/tor-gencert.c:parse_commandline() 111 problem function-size /src/tools/tor-resolve.c:build_socks5_resolve_request() 104 problem function-size /src/tools/tor-resolve.c:do_resolve() 175 problem function-size /src/tools/tor-resolve.c:main() 112 +problem function-size /src/core/or/circuitpadding.c:circpad_machine_schedule_padding() 107