From d44e3e57b0904259041fa19a2fe14639ce476135 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 04:42:28 +0000 Subject: [PATCH 1/7] Bug 28780: Add purpose for keeping padding circuits open --- src/core/or/circuitlist.c | 6 ++++++ src/core/or/circuitlist.h | 28 +++++++++++++++------------- src/core/or/circuituse.c | 1 + 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index cd2259c98d..91a02ce56f 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -823,6 +823,8 @@ circuit_purpose_to_controller_string(uint8_t purpose) return "PATH_BIAS_TESTING"; case CIRCUIT_PURPOSE_HS_VANGUARDS: return "HS_VANGUARDS"; + case CIRCUIT_PURPOSE_C_CIRCUIT_PADDING: + return "CIRCUIT_PADDING"; default: tor_snprintf(buf, sizeof(buf), "UNKNOWN_%d", (int)purpose); @@ -852,6 +854,7 @@ circuit_purpose_to_controller_hs_state_string(uint8_t purpose) case CIRCUIT_PURPOSE_CONTROLLER: case CIRCUIT_PURPOSE_PATH_BIAS_TESTING: case CIRCUIT_PURPOSE_HS_VANGUARDS: + case CIRCUIT_PURPOSE_C_CIRCUIT_PADDING: return NULL; case CIRCUIT_PURPOSE_INTRO_POINT: @@ -952,6 +955,9 @@ circuit_purpose_to_string(uint8_t purpose) case CIRCUIT_PURPOSE_HS_VANGUARDS: return "Hidden service: Pre-built vanguard circuit"; + case CIRCUIT_PURPOSE_C_CIRCUIT_PADDING: + return "Circuit kept open for padding"; + default: tor_snprintf(buf, sizeof(buf), "UNKNOWN_%d", (int)purpose); return buf; diff --git a/src/core/or/circuitlist.h b/src/core/or/circuitlist.h index a50e23716a..6f5fce4875 100644 --- a/src/core/or/circuitlist.h +++ b/src/core/or/circuitlist.h @@ -92,31 +92,33 @@ #define CIRCUIT_PURPOSE_C_HS_MAX_ 13 /** This circuit is used for build time measurement only */ #define CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT 14 -#define CIRCUIT_PURPOSE_C_MAX_ 14 +/** This circuit is being held open by circuit padding */ +#define CIRCUIT_PURPOSE_C_CIRCUIT_PADDING 15 +#define CIRCUIT_PURPOSE_C_MAX_ 15 -#define CIRCUIT_PURPOSE_S_HS_MIN_ 15 +#define CIRCUIT_PURPOSE_S_HS_MIN_ 16 /** Hidden-service-side circuit purpose: at the service, waiting for * introductions. */ -#define CIRCUIT_PURPOSE_S_ESTABLISH_INTRO 15 +#define CIRCUIT_PURPOSE_S_ESTABLISH_INTRO 16 /** Hidden-service-side circuit purpose: at the service, successfully * established intro. */ -#define CIRCUIT_PURPOSE_S_INTRO 16 +#define CIRCUIT_PURPOSE_S_INTRO 17 /** Hidden-service-side circuit purpose: at the service, connecting to rend * point. */ -#define CIRCUIT_PURPOSE_S_CONNECT_REND 17 +#define CIRCUIT_PURPOSE_S_CONNECT_REND 18 /** Hidden-service-side circuit purpose: at the service, rendezvous * established. */ -#define CIRCUIT_PURPOSE_S_REND_JOINED 18 +#define CIRCUIT_PURPOSE_S_REND_JOINED 19 /** This circuit is used for uploading hsdirs */ -#define CIRCUIT_PURPOSE_S_HSDIR_POST 19 -#define CIRCUIT_PURPOSE_S_HS_MAX_ 19 +#define CIRCUIT_PURPOSE_S_HSDIR_POST 20 +#define CIRCUIT_PURPOSE_S_HS_MAX_ 20 /** A testing circuit; not meant to be used for actual traffic. */ -#define CIRCUIT_PURPOSE_TESTING 20 +#define CIRCUIT_PURPOSE_TESTING 21 /** A controller made this circuit and Tor should not use it. */ -#define CIRCUIT_PURPOSE_CONTROLLER 21 +#define CIRCUIT_PURPOSE_CONTROLLER 22 /** This circuit is used for path bias probing only */ -#define CIRCUIT_PURPOSE_PATH_BIAS_TESTING 22 +#define CIRCUIT_PURPOSE_PATH_BIAS_TESTING 23 /** This circuit is used for vanguards/restricted paths. * @@ -124,9 +126,9 @@ * on-demand. When an HS operation needs to take place (e.g. connect to an * intro point), these circuits are then cannibalized and repurposed to the * actual needed HS purpose. */ -#define CIRCUIT_PURPOSE_HS_VANGUARDS 23 +#define CIRCUIT_PURPOSE_HS_VANGUARDS 24 -#define CIRCUIT_PURPOSE_MAX_ 23 +#define CIRCUIT_PURPOSE_MAX_ 24 /** A catch-all for unrecognized purposes. Currently we don't expect * to make or see any circuits with this purpose. */ #define CIRCUIT_PURPOSE_UNKNOWN 255 diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 226295425e..bdab476837 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -1514,6 +1514,7 @@ circuit_expire_old_circuits_clientside(void) circ->purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT || circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || circ->purpose == CIRCUIT_PURPOSE_TESTING || + circ->purpose == CIRCUIT_PURPOSE_C_CIRCUIT_PADDING || (circ->purpose >= CIRCUIT_PURPOSE_C_INTRODUCING && circ->purpose <= CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED) || circ->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) { From 662825474cb7d5fb0e713844aef5e912edd6ed8a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 18 Apr 2019 20:40:55 +0000 Subject: [PATCH 2/7] Bug 28780: Make use of purpose to keep padding circuits open. When a circuit is marked for close, check to see if any of our padding machines want to take ownership of it and continue padding until the machine hits the END state. For safety, we also ensure that machines that do not terminate are still closed as follows: Because padding machine timers are UINT32_MAX in size, if some sort of network event doesn't happen on a padding-only circuit within that time, we can conclude it is deadlocked and allow circuit_expire_old_circuits_clientside() to close it. If too much network activity happens, then per-machine padding limits can be used to cease padding, which will cause network cell events to cease, on the circuit, which will cause circpad to abandon the circuit as per the above time limit. --- src/core/or/circuitlist.c | 5 ++ src/core/or/circuitpadding.c | 119 ++++++++++++++++++++++++++++++++++- src/core/or/circuitpadding.h | 31 +++++++++ 3 files changed, 153 insertions(+), 2 deletions(-) diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c index 91a02ce56f..72952a8a52 100644 --- a/src/core/or/circuitlist.c +++ b/src/core/or/circuitlist.c @@ -2195,6 +2195,11 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line, tor_assert(line); tor_assert(file); + /* Check whether the circuitpadding subsystem wants to block this close */ + if (circpad_marked_circuit_for_padding(circ, reason)) { + return; + } + if (circ->marked_for_close) { log_warn(LD_BUG, "Duplicate call to circuit_mark_for_close at %s:%d" diff --git a/src/core/or/circuitpadding.c b/src/core/or/circuitpadding.c index 58e8e053c7..68140356c9 100644 --- a/src/core/or/circuitpadding.c +++ b/src/core/or/circuitpadding.c @@ -163,6 +163,116 @@ circpad_circuit_machineinfo_free_idx(circuit_t *circ, int idx) } } +/** + * Return true if circpad has decided to hold the circuit open for additional + * padding. This function is used to take and retain ownership of certain + * types of circuits that have padding machines on them, that have been passed + * to circuit_mark_for_close(). + * + * circuit_mark_for_close() calls this function to ask circpad if any padding + * machines want to keep the circuit open longer to pad. + * + * Any non-measurement circuit that was closed for a normal, non-error reason + * code may be held open for up to CIRCPAD_DELAY_INFINITE microseconds between + * network-driven cell events. + * + * After CIRCPAD_DELAY_INFINITE microseconds of silence on a circuit, this + * function will no longer hold it open (it will return 0 regardless of + * what the machines ask for, and thus circuit_expire_old_circuits_clientside() + * will close the circuit after roughly 1.25hr of idle time, maximum, + * regardless of the padding machine state. + */ +int +circpad_marked_circuit_for_padding(circuit_t *circ, int reason) +{ + /* If the circuit purpose is measurement or path bias, don't + * hold it open */ + if (circ->purpose == CIRCUIT_PURPOSE_PATH_BIAS_TESTING || + circ->purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) { + return 0; + } + + /* If the circuit is closed for any reason other than these three valid, + * client-side close reasons, do not try to keep it open. It is probably + * damaged or unusable. Note this is OK with vanguards because + * controller-closed circuits have REASON=REQUESTED, so vanguards-closed + * circuits will not be held open (we want them to close ASAP). */ + if (!(reason == END_CIRC_REASON_NONE || + reason == END_CIRC_REASON_FINISHED || + reason == END_CIRC_REASON_IP_NOW_REDUNDANT)) { + return 0; + } + + FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, circ) { + circpad_machine_runtime_t *mi = circ->padding_info[i]; + if (!mi) { + continue; // No padding runtime info; check next machine + } + + const circpad_state_t *state = circpad_machine_current_state(mi); + + /* If we're in END state (NULL here), then check next machine */ + if (!state) { + continue; // check next machine + } + + /* If the machine does not want to control the circuit close itself, then + * check the next machine */ + if (!circ->padding_machine[i]->manage_circ_lifetime) { + continue; // check next machine + } + + /* If the machine has reached the END state, we can close. Check next + * machine. */ + if (mi->current_state == CIRCPAD_STATE_END) { + continue; // check next machine + } + + log_info(LD_CIRC, "Circuit %d is not marked for close because of a " + " pending padding machine.", CIRCUIT_IS_ORIGIN(circ) ? + TO_ORIGIN_CIRCUIT(circ)->global_identifier : 0); + + /* If the machine has had no network events at all within the + * last circpad_delay_t timespan, it's in some deadlock state. + * Tell circuit_mark_for_close() that we don't own it anymore. + * This will allow circuit_expire_old_circuits_clientside() to + * close it. + */ + if (circ->padding_info[i]->last_cell_time_sec + + (time_t)CIRCPAD_DELAY_MAX_SECS < approx_time()) { + log_notice(LD_BUG, "Circuit %d was not marked for close because of a " + " pending padding machine for over an hour. Circuit is a %s", + CIRCUIT_IS_ORIGIN(circ) ? + TO_ORIGIN_CIRCUIT(circ)->global_identifier : 0, + circuit_purpose_to_string(circ->purpose)); + + return 0; // abort timer reached; mark the circuit for close now + } + + /* If we weren't marked dirty yet, let's pretend we're dirty now. + * ("Dirty" means that a circuit has been used for application traffic + * by Tor.. Dirty circuits have different expiry times, and are not + * considered in counts of built circuits, etc. By claiming that we're + * dirty, the rest of Tor will make decisions as if we were actually + * used by application data. + * + * This is most important for circuit_expire_old_circuits_clientside(), + * where we want that function to expire us after the padding machine + * has shut down, but using the MaxCircuitDirtiness timer instead of + * the idle circuit timer (again, we want this because we're not + * supposed to look idle to Guard nodes that can see our lifespan). */ + if (!circ->timestamp_dirty) + circ->timestamp_dirty = approx_time(); + + /* Take ownership of the circuit */ + circuit_change_purpose(circ, CIRCUIT_PURPOSE_C_CIRCUIT_PADDING); + + return 1; + } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END; + + return 0; // No machine wanted to keep the circuit open; mark for close +} + /** Free all the machineinfos in circ that match machine_num. */ static void free_circ_machineinfos_with_machine_num(circuit_t *circ, int machine_num) @@ -197,6 +307,7 @@ circpad_circuit_machineinfo_new(circuit_t *on_circ, int machine_index) tor_malloc_zero(sizeof(circpad_machine_runtime_t)); mi->machine_index = machine_index; mi->on_circ = on_circ; + mi->last_cell_time_sec = approx_time(); return mi; } @@ -1538,7 +1649,8 @@ circpad_cell_event_nonpadding_sent(circuit_t *on_circ) /* If there are no machines then this loop should not iterate */ FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) { - /* First, update any RTT estimate */ + /* First, update any timestamps */ + on_circ->padding_info[i]->last_cell_time_sec = approx_time(); 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 @@ -1564,7 +1676,8 @@ void circpad_cell_event_nonpadding_received(circuit_t *on_circ) { FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) { - /* First, update any RTT estimate */ + /* First, update any timestamps */ + on_circ->padding_info[i]->last_cell_time_sec = approx_time(); circpad_estimate_circ_rtt_on_received(on_circ, on_circ->padding_info[i]); circpad_machine_spec_transition(on_circ->padding_info[i], @@ -1584,6 +1697,7 @@ void circpad_cell_event_padding_sent(circuit_t *on_circ) { FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) { + on_circ->padding_info[i]->last_cell_time_sec = approx_time(); circpad_machine_spec_transition(on_circ->padding_info[i], CIRCPAD_EVENT_PADDING_SENT); } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END; @@ -1602,6 +1716,7 @@ circpad_cell_event_padding_received(circuit_t *on_circ) { /* identical to padding sent */ FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, on_circ) { + on_circ->padding_info[i]->last_cell_time_sec = approx_time(); circpad_machine_spec_transition(on_circ->padding_info[i], CIRCPAD_EVENT_PADDING_RECV); } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END; diff --git a/src/core/or/circuitpadding.h b/src/core/or/circuitpadding.h index f00369eb0a..56ed0a3467 100644 --- a/src/core/or/circuitpadding.h +++ b/src/core/or/circuitpadding.h @@ -73,6 +73,7 @@ typedef uint64_t circpad_time_t; /** The type for timer delays, in microseconds */ typedef uint32_t circpad_delay_t; +#define CIRCPAD_DELAY_UNITS_PER_SECOND (1000*1000) /** * An infinite padding cell delay means don't schedule any padding -- @@ -85,6 +86,13 @@ typedef uint32_t circpad_delay_t; */ #define CIRCPAD_DELAY_INFINITE (UINT32_MAX) +/** + * This is the maximum delay that the circuit padding system can have, in + * seconds. + */ +#define CIRCPAD_DELAY_MAX_SECS \ + ((CIRCPAD_DELAY_INFINITE/CIRCPAD_DELAY_UNITS_PER_SECOND)+1) + /** * Macro to clarify when we're checking the infinity bin. * @@ -524,6 +532,14 @@ typedef struct circpad_machine_runtime_t { * half. */ uint16_t nonpadding_sent; + /** + * Timestamp of the most recent cell event (sent, received, padding, + * non-padding), in seconds from approx_time(). + * + * Used as an emergency break to stop holding padding circuits open. + */ + time_t last_cell_time_sec; + /** * EWMA estimate of the RTT of the circuit from this hop * to the exit end, in microseconds. */ @@ -605,6 +621,19 @@ typedef struct circpad_machine_spec_t { * 1-indexed (ie: hop #1 is guard, #2 middle, #3 exit). */ unsigned target_hopnum : 3; + /** If this flag is enabled, don't close circuits that use this machine even + * if another part of Tor wants to close this circuit. + * + * If this flag is set, the circuitpadding subsystem will close circuits the + * moment the machine transitions to the END state, and only if the circuit + * has already been asked to be closed by another part of Tor. + * + * Circuits that should have been closed but were kept open by a padding + * machine are re-purposed to CIRCUIT_PURPOSE_C_CIRCUIT_PADDING, hence + * machines should take that purpose into account if they are filtering + * circuits by purpose. */ + unsigned manage_circ_lifetime : 1; + /** This machine only kills fascists if the following conditions are met. */ circpad_machine_conditions_t conditions; @@ -630,6 +659,8 @@ typedef struct circpad_machine_spec_t { void circpad_new_consensus_params(const networkstatus_t *ns); +int circpad_marked_circuit_for_padding(circuit_t *circ, int reason); + /** * The following are event call-in points that are of interest to * the state machines. They are called during cell processing. */ From e253a117c0a2db9315ac44f09cfba5e854f4e172 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 04:46:05 +0000 Subject: [PATCH 3/7] Bug 28780: Add tests Also test circpad expiry safeguard. --- src/core/or/circuituse.c | 4 +- src/test/test_circuitpadding.c | 188 +++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 2 deletions(-) diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index bdab476837..485c389054 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -70,7 +70,7 @@ #include "core/or/origin_circuit_st.h" #include "core/or/socks_request_st.h" -static void circuit_expire_old_circuits_clientside(void); +STATIC void circuit_expire_old_circuits_clientside(void); static void circuit_increment_failure_count(void); /** Check whether the hidden service destination of the stream at @@ -1474,7 +1474,7 @@ circuit_detach_stream(circuit_t *circ, edge_connection_t *conn) /** Find each circuit that has been unused for too long, or dirty * for too long and has no streams on it: mark it for close. */ -static void +STATIC void circuit_expire_old_circuits_clientside(void) { struct timeval cutoff, now; diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index db175fecee..2e67159440 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -41,6 +41,7 @@ TOR_NSEC_PER_USEC*TOR_USEC_PER_SEC) extern smartlist_t *connection_array; +void circuit_expire_old_circuits_clientside(void); circid_t get_unique_circ_id_by_chan(channel_t *chan); void helper_create_basic_machine(void); @@ -2550,6 +2551,192 @@ test_circuitpadding_reduce_disable(void *arg) circuitmux_free(dummy_channel.cmux); } +/** Just a basic machine whose whole purpose is to reach the END state */ +static void +helper_create_ender_machine(void) +{ + /* Start, burst */ + circpad_machine_states_init(&circ_client_machine, 2); + + circ_client_machine.states[CIRCPAD_STATE_START]. + next_state[CIRCPAD_EVENT_NONPADDING_RECV] = CIRCPAD_STATE_END; + + circ_client_machine.conditions.state_mask = CIRCPAD_STATE_ALL; + circ_client_machine.conditions.purpose_mask = CIRCPAD_PURPOSE_ALL; +} + +static time_t mocked_timeofday; +/** Set timeval to a mock date and time. This is necessary + * to make tor_gettimeofday() mockable. */ +static void +mock_tor_gettimeofday(struct timeval *timeval) +{ + timeval->tv_sec = mocked_timeofday; + timeval->tv_usec = 0; +} + +/** Test manual managing of circuit lifetimes by the circuitpadding + * subsystem. In particular this test goes through all the cases of the + * circpad_marked_circuit_for_padding() function, via + * circuit_mark_for_close() as well as + * circuit_expire_old_circuits_clientside(). */ +static void +test_circuitpadding_manage_circuit_lifetime(void *arg) +{ + circpad_machine_runtime_t *mi; + + (void) arg; + + client_side = (circuit_t *)origin_circuit_new(); + client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; + monotime_enable_test_mocking(); + MOCK(tor_gettimeofday, mock_tor_gettimeofday); + mocked_timeofday = 23; + + helper_create_ender_machine(); + + /* Enable manual circuit lifetime manage for this test */ + circ_client_machine.manage_circ_lifetime = 1; + + /* Test setup */ + client_side->padding_machine[0] = &circ_client_machine; + client_side->padding_info[0] = + circpad_circuit_machineinfo_new(client_side, 0); + mi = client_side->padding_info[0]; + + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_START); + + /* Check that the circuit is not marked for close */ + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_GENERAL); + + /* Mark this circuit for close due to a remote reason */ + circuit_mark_for_close(client_side, + END_CIRC_REASON_FLAG_REMOTE|END_CIRC_REASON_NONE); + tt_ptr_op(client_side->padding_info[0], OP_NE, NULL); + tt_int_op(client_side->marked_for_close, OP_NE, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_GENERAL); + client_side->marked_for_close = 0; + + /* Mark this circuit for close due to a protocol issue */ + circuit_mark_for_close(client_side, END_CIRC_REASON_TORPROTOCOL); + tt_int_op(client_side->marked_for_close, OP_NE, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_GENERAL); + client_side->marked_for_close = 0; + + /* Mark a measurement circuit for close */ + client_side->purpose = CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT; + circuit_mark_for_close(client_side, END_CIRC_REASON_NONE); + tt_int_op(client_side->marked_for_close, OP_NE, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT); + client_side->marked_for_close = 0; + + /* Mark a general circuit for close */ + client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; + circuit_mark_for_close(client_side, END_CIRC_REASON_NONE); + + /* Check that this circuit is still not marked for close since we are + * managing the lifetime manually, but the circuit was tagged as such by the + * circpadding subsystem */ + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_CIRCUIT_PADDING); + + /* We just tested case (1) from the comments of + * circpad_circuit_should_be_marked_for_close() */ + + /* Transition the machine to the END state but did not delete its machine */ + tt_ptr_op(client_side->padding_info[0], OP_NE, NULL); + circpad_cell_event_nonpadding_received(client_side); + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END); + + /* We just tested case (3) from the comments of + * circpad_circuit_should_be_marked_for_close(). + * Now let's go for case (2). */ + + /* Reset the close mark */ + client_side->marked_for_close = 0; + + /* Mark this circuit for close */ + circuit_mark_for_close(client_side, 0); + + /* See that the circ got closed since we are already in END state */ + tt_int_op(client_side->marked_for_close, OP_NE, 0); + + /* We just tested case (2). Now let's see that case (4) is unreachable as + that comment claims */ + + /* First, reset all close marks and tags */ + client_side->marked_for_close = 0; + client_side->purpose = CIRCUIT_PURPOSE_C_GENERAL; + + /* Now re-create the ender machine so that we can transition to END again */ + /* Free up some stuff first */ + circpad_circuit_free_all_machineinfos(client_side); + tor_free(circ_client_machine.states); + helper_create_ender_machine(); + + client_side->padding_machine[0] = &circ_client_machine; + client_side->padding_info[0] = + circpad_circuit_machineinfo_new(client_side, 0); + mi = client_side->padding_info[0]; + + /* Check we are in START. */ + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_START); + + /* Test that we don't expire this circuit yet */ + client_side->timestamp_dirty = 0; + client_side->state = CIRCUIT_STATE_OPEN; + tor_gettimeofday(&client_side->timestamp_began); + TO_ORIGIN_CIRCUIT(client_side)->circuit_idle_timeout = 23; + mocked_timeofday += 24; + circuit_expire_old_circuits_clientside(); + circuit_expire_old_circuits_clientside(); + circuit_expire_old_circuits_clientside(); + tt_int_op(client_side->timestamp_dirty, OP_NE, 0); + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + tt_int_op(client_side->purpose, OP_EQ, CIRCUIT_PURPOSE_C_CIRCUIT_PADDING); + + /* Runaway circpad test: if the machine does not transition to end, + * test that after CIRCPAD_DELAY_MAX_SECS, we get marked anyway */ + mocked_timeofday = client_side->timestamp_dirty + + get_options()->MaxCircuitDirtiness + 2; + client_side->padding_info[0]->last_cell_time_sec = + approx_time()-(CIRCPAD_DELAY_MAX_SECS+10); + circuit_expire_old_circuits_clientside(); + tt_int_op(client_side->marked_for_close, OP_NE, 0); + + /* Test back to normal: if we had activity, we won't close */ + client_side->padding_info[0]->last_cell_time_sec = approx_time(); + client_side->marked_for_close = 0; + circuit_expire_old_circuits_clientside(); + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + + /* Transition to END, but before we're past the dirty timer */ + mocked_timeofday = client_side->timestamp_dirty; + circpad_cell_event_nonpadding_received(client_side); + tt_int_op(mi->current_state, OP_EQ, CIRCPAD_STATE_END); + + /* Verify that the circuit was not closed. */ + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + + /* Now that we are in END state, we can be closed by expiry, but via + * the timestamp_dirty path, not the idle path. So first test not dirty + * enough. */ + mocked_timeofday = client_side->timestamp_dirty; + circuit_expire_old_circuits_clientside(); + tt_int_op(client_side->marked_for_close, OP_EQ, 0); + mocked_timeofday = client_side->timestamp_dirty + + get_options()->MaxCircuitDirtiness + 2; + circuit_expire_old_circuits_clientside(); + tt_int_op(client_side->marked_for_close, OP_NE, 0); + + done: + free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side)); + tor_free(circ_client_machine.states); + monotime_disable_test_mocking(); + UNMOCK(tor_gettimeofday); +} + #define TEST_CIRCUITPADDING(name, flags) \ { #name, test_##name, (flags), NULL, NULL } @@ -2570,5 +2757,6 @@ struct testcase_t circuitpadding_tests[] = { TEST_CIRCUITPADDING(circuitpadding_closest_token_removal, TT_FORK), TEST_CIRCUITPADDING(circuitpadding_closest_token_removal_usec, TT_FORK), TEST_CIRCUITPADDING(circuitpadding_token_removal_exact, TT_FORK), + TEST_CIRCUITPADDING(circuitpadding_manage_circuit_lifetime, TT_FORK), END_OF_TESTCASES }; From 5cc988a947e8ba10e2682274465a82ff22e7c93d Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 18 Apr 2019 20:35:22 +0000 Subject: [PATCH 4/7] Bug 28780: Changes file --- changes/ticket28780 | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changes/ticket28780 diff --git a/changes/ticket28780 b/changes/ticket28780 new file mode 100644 index 0000000000..d7c6693f8c --- /dev/null +++ b/changes/ticket28780 @@ -0,0 +1,3 @@ + o Minor features (circuit padding): + - Provide the ability for circuit padding machines to hold a circuit open + until they are done padding it. Closes ticket 28780. From b98bcd789e45fb6a09ef92ab15052a6c19e24887 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 8 May 2019 21:21:03 +0000 Subject: [PATCH 5/7] Pathbias should continue to ignore previously ignored circs. If circuit padding wants to keep a circuit open and pathbias used to ignore it, pathbias should continue to ignore it. This may catch other purpose-change related miscounts (such as timeout measurement, cannibalization, onion service circuit transitions, and vanguards). --- src/feature/client/circpathbias.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/feature/client/circpathbias.c b/src/feature/client/circpathbias.c index e6af649ba7..2be9984a64 100644 --- a/src/feature/client/circpathbias.c +++ b/src/feature/client/circpathbias.c @@ -400,12 +400,13 @@ pathbias_should_count(origin_circuit_t *circ) /* Check to see if the shouldcount result has changed due to a * unexpected purpose change that would affect our results */ if (circ->pathbias_shouldcount == PATHBIAS_SHOULDCOUNT_IGNORED) { - log_info(LD_BUG, - "Circuit %d is now being counted despite being ignored " - "in the past. Purpose is %s, path state is %s", - circ->global_identifier, - circuit_purpose_to_string(circ->base_.purpose), - pathbias_state_to_string(circ->path_state)); + log_info(LD_CIRC, + "Circuit %d is not being counted by pathbias because it was " + "ignored in the past. Purpose is %s, path state is %s", + circ->global_identifier, + circuit_purpose_to_string(circ->base_.purpose), + pathbias_state_to_string(circ->path_state)); + return 0; } circ->pathbias_shouldcount = PATHBIAS_SHOULDCOUNT_COUNTED; From 56738ff8c626b21d6e3d146c9a2c0c4d18f3884a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 8 May 2019 21:28:00 +0000 Subject: [PATCH 6/7] Add control port circuit ID to all pathbias bug messages. To ease debugging of miscount issues, attach vanguards with --loglevel DEBUG and obtain control port logs (or use any other control port CIRC and CIRC_MINOR event logging mechanism). --- src/feature/client/circpathbias.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/feature/client/circpathbias.c b/src/feature/client/circpathbias.c index 2be9984a64..3544dbb859 100644 --- a/src/feature/client/circpathbias.c +++ b/src/feature/client/circpathbias.c @@ -371,8 +371,9 @@ pathbias_should_count(origin_circuit_t *circ) !circ->build_state->onehop_tunnel) { if ((rate_msg = rate_limit_log(&count_limit, approx_time()))) { log_info(LD_BUG, - "One-hop circuit has length %d. Path state is %s. " + "One-hop circuit %d has length %d. Path state is %s. " "Circuit is a %s currently %s.%s", + circ->global_identifier, circ->build_state->desired_path_len, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), @@ -437,8 +438,9 @@ pathbias_count_build_attempt(origin_circuit_t *circ) if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit, approx_time()))) { log_info(LD_BUG, - "Opened circuit is in strange path state %s. " + "Opened circuit %d is in strange path state %s. " "Circuit is a %s currently %s.%s", + circ->global_identifier, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), circuit_state_to_string(circ->base_.state), @@ -471,8 +473,9 @@ pathbias_count_build_attempt(origin_circuit_t *circ) if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit, approx_time()))) { log_info(LD_BUG, - "Unopened circuit has strange path state %s. " + "Unopened circuit %d has strange path state %s. " "Circuit is a %s currently %s.%s", + circ->global_identifier, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), circuit_state_to_string(circ->base_.state), @@ -541,8 +544,9 @@ pathbias_count_build_success(origin_circuit_t *circ) if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { log_info(LD_BUG, - "Succeeded circuit is in strange path state %s. " + "Succeeded circuit %d is in strange path state %s. " "Circuit is a %s currently %s.%s", + circ->global_identifier, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), circuit_state_to_string(circ->base_.state), @@ -577,8 +581,9 @@ pathbias_count_build_success(origin_circuit_t *circ) if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { log_info(LD_BUG, - "Opened circuit is in strange path state %s. " + "Opened circuit %d is in strange path state %s. " "Circuit is a %s currently %s.%s", + circ->global_identifier, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), circuit_state_to_string(circ->base_.state), @@ -604,8 +609,9 @@ pathbias_count_use_attempt(origin_circuit_t *circ) if (circ->path_state < PATH_STATE_BUILD_SUCCEEDED) { log_notice(LD_BUG, - "Used circuit is in strange path state %s. " + "Used circuit %d is in strange path state %s. " "Circuit is a %s currently %s.", + circ->global_identifier, pathbias_state_to_string(circ->path_state), circuit_purpose_to_string(circ->base_.purpose), circuit_state_to_string(circ->base_.state)); From ff2a9809353c1166cb494dac3a14502b8ee24013 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 May 2019 04:47:50 +0000 Subject: [PATCH 7/7] The practracker beatings will continue until the files get smaller. --- scripts/maint/practracker/exceptions.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 24452b6158..aea6ef84bf 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -92,7 +92,7 @@ problem function-size /src/core/or/circuitlist.c:circuits_handle_oom() 117 problem function-size /src/core/or/circuitmux.c:circuitmux_set_policy() 110 problem function-size /src/core/or/circuitmux.c:circuitmux_attach_circuit() 114 problem function-size /src/core/or/circuitstats.c:circuit_build_times_parse_state() 124 -problem file-size /src/core/or/circuituse.c 3155 +problem file-size /src/core/or/circuituse.c 3156 problem function-size /src/core/or/circuituse.c:circuit_is_acceptable() 133 problem function-size /src/core/or/circuituse.c:circuit_expire_building() 394 problem function-size /src/core/or/circuituse.c:circuit_log_ancient_one_hop_circuits() 126