Refactor code that rolls back the use state

Also document it better.

Mention this refactoring in the comments for the path state machine.

4 files changed, 35 insertions, 19 deletions

diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 2a3c40cf06..545184b4fc 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -1678,6 +1678,31 @@ pathbias_mark_use_success(origin_circuit_t *circ)
 }
 
 /**
+ * If a stream ever detatches from a circuit in a retriable way,
+ * we need to mark this circuit as still needing either another
+ * successful stream, or in need of a probe.
+ *
+ * An adversary could let the first stream request succeed (ie the
+ * resolve), but then tag and timeout the remainder (via cell
+ * dropping), forcing them on new circuits.
+ *
+ * Rolling back the state will cause us to probe such circuits, which
+ * should lead to probe failures in the event of such tagging due to
+ * either unrecognized cells coming in while we wait for the probe,
+ * or the cipher state getting out of sync in the case of dropped cells.
+ */
+void
+pathbias_mark_use_rollback(origin_circuit_t *circ)
+{
+  if (circ->path_state == PATH_STATE_USE_SUCCEEDED) {
+    log_info(LD_CIRC,
+             "Rolling back pathbias use state to 'attempted' for detached "
+             "circuit %d", circ->global_identifier);
+    circ->path_state = PATH_STATE_USE_ATTEMPTED;
+  }
+}
+
+/**
  * Actually count a circuit success towards a guard's usage counters
  * if the path state is appropriate.
  */
diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h
index d03a7c5323..3ca8d1531d 100644
--- a/src/or/circuitbuild.h
+++ b/src/or/circuitbuild.h
@@ -65,6 +65,7 @@ int pathbias_check_close(origin_circuit_t *circ, int reason);
 int pathbias_check_probe_response(circuit_t *circ, const cell_t *cell);
 void pathbias_count_use_attempt(origin_circuit_t *circ);
 void pathbias_mark_use_success(origin_circuit_t *circ);
+void pathbias_mark_use_rollback(origin_circuit_t *circ);
 
 #endif
 
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 9e2c15d2ca..b4fa3e6fe2 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -637,21 +637,15 @@ connection_ap_expire_beginning(void)
     }
     if (circ->purpose == CIRCUIT_PURPOSE_C_REND_JOINED) {
       if (seconds_idle >= options->SocksTimeout) {
-        /* Path bias: We need to probe the circuit to ensure validity.
-         * Roll its state back if it succeeded so that we do so upon close. */
-        if (TO_ORIGIN_CIRCUIT(circ)->path_state == PATH_STATE_USE_SUCCEEDED) {
-          log_info(LD_CIRC,
-                   "Rolling back pathbias use state to 'attempted' for timed "
-                   "out rend circ %d",
-                   TO_ORIGIN_CIRCUIT(circ)->global_identifier);
-          TO_ORIGIN_CIRCUIT(circ)->path_state = PATH_STATE_USE_ATTEMPTED;
-        }
-
         log_fn(severity, LD_REND,
                "Rend stream is %d seconds late. Giving up on address"
                " '%s.onion'.",
                seconds_idle,
                safe_str_client(entry_conn->socks_request->address));
+        /* Roll back path bias use state so that we probe the circuit
+         * if nothing else succeeds on it */
+        pathbias_mark_use_rollback(TO_ORIGIN_CIRCUIT(circ));
+
         connection_edge_end(conn, END_STREAM_REASON_TIMEOUT);
         connection_mark_unattached_ap(entry_conn, END_STREAM_REASON_TIMEOUT);
       }
@@ -816,14 +810,9 @@ connection_ap_detach_retriable(entry_connection_t *conn,
   control_event_stream_status(conn, STREAM_EVENT_FAILED_RETRIABLE, reason);
   ENTRY_TO_CONN(conn)->timestamp_lastread = time(NULL);
 
-  /* Path bias: We need to probe the circuit to ensure validity.
-   * Roll its state back if it succeeded so that we do so upon close. */
-  if (circ->path_state == PATH_STATE_USE_SUCCEEDED) {
-    log_info(LD_CIRC,
-             "Rolling back pathbias use state to 'attempted' for detached "
-             "circuit %d", circ->global_identifier);
-    circ->path_state = PATH_STATE_USE_ATTEMPTED;
-  }
+  /* Roll back path bias use state so that we probe the circuit
+   * if nothing else succeeds on it */
+  pathbias_mark_use_rollback(circ);
 
   if (conn->pending_optimistic_data) {
     generic_buffer_set_to_copy(&conn->sending_optimistic_data,
diff --git a/src/or/or.h b/src/or/or.h
index e5fc06ac39..11158ff6f8 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -2862,7 +2862,8 @@ typedef enum {
       * this circuit?
       *
       * If any streams detatch/fail from this circuit, the code transitions
-      * the circuit back to PATH_STATE_USE_ATTEMPTED to ensure we probe.
+      * the circuit back to PATH_STATE_USE_ATTEMPTED to ensure we probe. See
+      * pathbias_mark_use_rollback() for that.
       */
     PATH_STATE_USE_SUCCEEDED = 4,