Merge 3ebf0bb into 3c64cfe

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 3152
+problem file-size /src/core/or/circuituse.c 3153
 problem function-size /src/core/or/circuituse.c:circuit_is_acceptable() 129
 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

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;
@@ -2199,6 +2205,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_circuit_should_be_marked_for_close(circ, reason)) {
+    return;
+  }
+
   if (circ->marked_for_close) {
     log_warn(LD_BUG,
         "Duplicate call to circuit_mark_for_close at %s:%d"

src/core/or/circuitlist.h

@@ -92,41 +92,43 @@
 #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.
  *
  *  This type of circuit is *only* created preemptively and never
  *  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_ 25
 /** 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

src/core/or/circuitpadding.c

@@ -161,6 +161,87 @@ circpad_circuit_machineinfo_free_idx(circuit_t *circ, int idx)
   }
 }
 
+/** Check if the current active machine on this circuit wants to manage circuit
+ * lifetimes on its own, and return true if the circuit should be marked for
+ * close, or false otherwise.
+ *
+ * If the machine does not manage the circuit lifetime itself, we just close
+ * the circuit if we get here. In the case where the machine manages the
+ * circuit lifetime there are 3 cases under which we can find ourselves here:
+ *
+ * 1) Machine has *not* reached END state, and another subsystem wants to close
+ *    the circuit: change the circuit purpose to ours but dont close it.
+ *
+ * 2) Machine has reached END state, and another subsystem wants to close the
+ *    circuit now: close the circuit!
+ *
+ * 3) Another subsystem had tried to close the circuit, and the machine just
+ *    reached END state: close the circuit.
+ */
+int
+circpad_circuit_should_be_marked_for_close(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 1;
+  }
+
+  /* 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 1;
+  }
+
+  FOR_EACH_ACTIVE_CIRCUIT_MACHINE_BEGIN(i, circ) {
+    circpad_machine_runtime_t *mi = circ->padding_info[i];
+    if (!mi) {
+      continue;
+    }
+
+    const circpad_state_t *state = circpad_machine_current_state(mi);
+
+    /* If we're in END state (NULL here), then allow circ to get marked */
+    if (!state) {
+      continue;
+    }
+
+    /* If the machine does not want to control the circuit close itself, then
+     * just close the circuit right now */
+    if (!circ->padding_machine[i]->manage_circ_lifetime) {
+      return 1;
+    }
+
+    /* We now know that the machine controls when a circuit should be
+     * closed. In this case, we only want to close the circuit if another
+     * subsystem of Tor has already asked us to close this circuit AND the
+     * machine has reached the end state */
+
+    /* Change the circuit purpose regardless of whether we close the
+     * circuit. */
+    circuit_change_purpose(circ, CIRCUIT_PURPOSE_C_CIRCUIT_PADDING);
+
+    /* If the machine has reached the END state, close the circuit, otherwise
+       keep it alive. */
+    if (mi->current_state == CIRCPAD_STATE_END) {
+      return 1;
+    } else {
+      log_info(LD_GENERAL, "Circuit %d is not marked for close because of a "
+               " pending padding machine.", CIRCUIT_IS_ORIGIN(circ) ?
+               TO_ORIGIN_CIRCUIT(circ)->global_identifier : 0);
+      return 0;
+    }
+  } FOR_EACH_ACTIVE_CIRCUIT_MACHINE_END;
+
+  return 1;
+}
+
 /** Free all the machineinfos in <b>circ</b> that match <b>machine_num</b>. */
 static void
 free_circ_machineinfos_with_machine_num(circuit_t *circ, int machine_num)

src/core/or/circuitpadding.h

@@ -594,6 +594,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;
 
@@ -619,6 +632,8 @@ typedef struct circpad_machine_spec_t {
 
 void circpad_new_consensus_params(const networkstatus_t *ns);
 
+int circpad_circuit_should_be_marked_for_close(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. */

src/core/or/circuituse.c

@@ -1511,6 +1511,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) {

src/test/test_circuitpadding.c

@@ -2396,6 +2396,137 @@ test_circuitpadding_global_rate_limiting(void *arg)
   smartlist_free(vote1.net_params);
 }
 
+/** 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;
+}
+
+/** Test manual managing of circuit lifetimes by the circuitpadding
+ *  subsystem. In particular this test goes through all the cases of the
+ *  circpad_circuit_should_be_marked_for_close() function doc. */
+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();
+
+  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);
+
+  /* Transition to END. */
+  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);
+
+ done:
+  free_fake_origin_circuit(TO_ORIGIN_CIRCUIT(client_side));
+  tor_free(circ_client_machine.states);
+  monotime_disable_test_mocking();
+}
+
 #define TEST_CIRCUITPADDING(name, flags) \
     { #name, test_##name, (flags), NULL, NULL }
 
@@ -2415,5 +2546,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
 };