Merge branch 'bug40827' into 'main'

Fix assert crash on relay-side due to on_circuit backpointer

See merge request tpo/core/tor!737

4 files changed, 89 insertions, 4 deletions

diff --git a/changes/bug40827 b/changes/bug40827
new file mode 100644
index 0000000000..69878e7785
--- /dev/null
+++ b/changes/bug40827
@@ -0,0 +1,8 @@
+  o Major bugfixes (conflux):
+    - Fix a relay-side assert crash caused by attempts to use a conflux
+      circuit between circuit close and free, such that no legs were on
+      the conflux set. Fixed by nulling out the stream's circuit
+      back-pointer when the last leg is removed. Additional checks and
+      log messages have been added to detect other cases. Fixes bug 40827;
+      bugfix on 0.4.8.1-alpha.
+
diff --git a/src/core/or/conflux.c b/src/core/or/conflux.c
index c979077e1f..eb004b3626 100644
--- a/src/core/or/conflux.c
+++ b/src/core/or/conflux.c
@@ -536,7 +536,7 @@ conflux_note_cell_sent(conflux_t *cfx, circuit_t *circ, uint8_t relay_command)
 
 /** Find the leg with lowest non-zero curr_rtt_usec, and
  * pick it for our current leg. */
-static inline void
+static inline bool
 conflux_pick_first_leg(conflux_t *cfx)
 {
   conflux_leg_t *min_leg = NULL;
@@ -555,8 +555,20 @@ conflux_pick_first_leg(conflux_t *cfx)
   } CONFLUX_FOR_EACH_LEG_END(leg);
 
   if (!min_leg) {
-    // Get the 0th leg; if it does not exist, assert
-    tor_assert(smartlist_len(cfx->legs) > 0);
+    // Get the 0th leg; if it does not exist, log the set.
+    // Bug 40827 managed to hit this, so let's dump the sets
+    // in case it happens again.
+    if (BUG(smartlist_len(cfx->legs) <= 0)) {
+      // Since we have no legs, we have no idea if this is really a client
+      // or server set. Try to find any that match:
+      log_warn(LD_BUG, "Matching client sets:");
+      conflux_log_set(cfx, true);
+      log_warn(LD_BUG, "Matching server sets:");
+      conflux_log_set(cfx, false);
+      log_warn(LD_BUG, "End conflux set dump");
+      return false;
+    }
+
     min_leg = smartlist_get(cfx->legs, 0);
     tor_assert(min_leg);
     if (BUG(min_leg->linked_sent_usec == 0)) {
@@ -572,6 +584,8 @@ conflux_pick_first_leg(conflux_t *cfx)
   cfx->cells_until_switch = 0;
 
   cfx->curr_leg = min_leg;
+
+  return true;
 }
 
 /**
@@ -589,7 +603,8 @@ conflux_decide_next_circ(conflux_t *cfx)
   /* If we don't have a current leg yet, pick one.
    * (This is the only non-const operation in this function). */
   if (!cfx->curr_leg) {
-    conflux_pick_first_leg(cfx);
+    if (!conflux_pick_first_leg(cfx))
+      return NULL;
   }
 
   /* First, check if we can switch. */
diff --git a/src/core/or/conflux_pool.c b/src/core/or/conflux_pool.c
index 79fd6c1648..837c1da720 100644
--- a/src/core/or/conflux_pool.c
+++ b/src/core/or/conflux_pool.c
@@ -1509,6 +1509,49 @@ linked_update_stream_backpointers(circuit_t *circ)
   }
 }
 
+/** This is called when this circuit is the last leg.
+ *
+ * The "on_circuit" pointer is nullified here so it is not given back to the
+ * conflux subsytem between the circuit mark for close step and actually
+ * freeing the circuit which is when streams are destroyed.
+ *
+ * Reason is that when the connection edge inbuf is packaged in
+ * connection_edge_package_raw_inbuf(), the on_circuit pointer is used and
+ * then passed on to conflux to learn which leg to use. However, if the circuit
+ * was marked prior but not yet freed, there are no more legs remaining which
+ * leads to a linked circuit being used without legs. No bueno. */
+static void
+linked_detach_circuit(circuit_t *circ)
+{
+  tor_assert(circ);
+  tor_assert_nonfatal(circ->conflux);
+
+  if (CIRCUIT_IS_ORIGIN(circ)) {
+    origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
+    tor_assert_nonfatal(circ->purpose == CIRCUIT_PURPOSE_CONFLUX_LINKED);
+    /* Iterate over stream list using next_stream pointer, until null */
+    for (edge_connection_t *stream = ocirc->p_streams; stream;
+           stream = stream->next_stream) {
+      /* Update the circuit pointer of each stream */
+      stream->on_circuit = NULL;
+    }
+  } else {
+    or_circuit_t *orcirc = TO_OR_CIRCUIT(circ);
+    /* Iterate over stream list using next_stream pointer, until null */
+    for (edge_connection_t *stream = orcirc->n_streams; stream;
+           stream = stream->next_stream) {
+      /* Update the circuit pointer of each stream */
+      stream->on_circuit = NULL;
+    }
+    /* Iterate over stream list using next_stream pointer, until null */
+    for (edge_connection_t *stream = orcirc->resolving_streams; stream;
+           stream = stream->next_stream) {
+      /* Update the circuit pointer of each stream */
+      stream->on_circuit = NULL;
+    }
+  }
+}
+
 /** Nullify all streams of the given circuit. */
 static void
 linked_nullify_streams(circuit_t *circ)
@@ -1549,6 +1592,7 @@ linked_circuit_closed(circuit_t *circ)
   if (CONFLUX_NUM_LEGS(circ->conflux) == 0) {
     /* Last leg, remove conflux object from linked set. */
     linked_pool_del(circ->conflux->nonce, is_client);
+    linked_detach_circuit(circ);
   } else {
     /* If there are other circuits, update streams backpointers and
      * nullify the stream lists. We do not free those streams in circuit_free_.
diff --git a/src/core/or/relay.c b/src/core/or/relay.c
index 3af9435a76..5336c2ea73 100644
--- a/src/core/or/relay.c
+++ b/src/core/or/relay.c
@@ -2352,6 +2352,24 @@ connection_edge_package_raw_inbuf(edge_connection_t *conn, int package_partial,
     return -1;
   }
 
+  // Bug 40827: With conflux, we suspect marked circuits were getting here.
+  // We think we fixed it, but let's add a check and log sets if it still
+  // happens.
+  if (BUG(circ->marked_for_close)) {
+    log_warn(LD_BUG,
+             "called on circ that's already marked for close at %s:%d.",
+             circ->marked_for_close_file, circ->marked_for_close);
+    if (CIRCUIT_IS_CONFLUX(circ)) {
+      if (circ->conflux) {
+        conflux_log_set(circ->conflux, CIRCUIT_IS_ORIGIN(circ));
+      } else {
+        log_warn(LD_BUG, "  - circ is unlinked conflux");
+      }
+    }
+    conn->end_reason = END_STREAM_REASON_INTERNAL;
+    return -1;
+  }
+
   if (circuit_consider_stop_edge_reading(circ, cpath_layer))
     return 0;