Handle closing circuits correctly with circuitmux_t

2 files changed, 68 insertions, 51 deletions

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index a4dcec68a..a87f989f3 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -131,11 +131,12 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
   if (old_chan) {
     /*
      * If we're changing channels or ID and had an old channel and a non
-     * zero old ID (i.e., we should have been attached), detach the circuit.
-     * ID changes require this because circuitmux hashes on (channel_id,
-     * circuit_id).
+     * zero old ID and weren't marked for close (i.e., we should have been
+     * attached), detach the circuit. ID changes require this because
+     * circuitmux hashes on (channel_id, circuit_id).
      */
-    if (id != 0 && (old_chan != chan || old_id != id)) {
+    if (id != 0 && (old_chan != chan || old_id != id) &&
+        !(circ->marked_for_close)) {
       tor_assert(old_chan->cmux);
       circuitmux_detach_circuit(old_chan->cmux, circ);
     }
@@ -180,9 +181,11 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
 
   /*
    * Attach to the circuitmux if we're changing channels or IDs and
-   * have a new channel and ID to use.
+   * have a new channel and ID to use and the circuit is not marked for
+   * close.
    */
-  if (chan && id != 0 && (old_chan != chan || old_id != id)) {
+  if (chan && id != 0 && (old_chan != chan || old_id != id) &&
+      !(circ->marked_for_close)) {
     tor_assert(chan->cmux);
     circuitmux_attach_circuit(chan->cmux, circ, direction);
     attached = 1;
@@ -1398,6 +1401,7 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line,
   if (circ->n_chan) {
     circuit_clear_cell_queue(circ, circ->n_chan);
     channel_send_destroy(circ->n_circ_id, circ->n_chan, reason);
+    circuitmux_detach_circuit(circ->n_chan->cmux, circ);
   }
 
   if (! CIRCUIT_IS_ORIGIN(circ)) {
@@ -1425,6 +1429,7 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line,
     if (or_circ->p_chan) {
       circuit_clear_cell_queue(circ, or_circ->p_chan);
       channel_send_destroy(or_circ->p_circ_id, or_circ->p_chan, reason);
+      circuitmux_detach_circuit(or_circ->p_chan->cmux, circ);
     }
   } else {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c
index 2bf511fe4..48336d831 100644
--- a/src/or/circuitmux.c
+++ b/src/or/circuitmux.c
@@ -130,6 +130,8 @@ struct chanid_circid_muxinfo_t {
  * Internal-use #defines
  */
 
+#define CMUX_PARANOIA
+
 #ifdef CMUX_PARANOIA
 #define circuitmux_assert_okay_paranoid(cmux) \
   circuitmux_assert_okay(cmux)
@@ -1476,53 +1478,63 @@ circuitmux_assert_okay_pass_one(circuitmux_t *cmux)
     chan = channel_find_by_global_id(chan_id);
     tor_assert(chan);
     circ = circuit_get_by_circid_channel(circ_id, chan);
-    tor_assert(circ);
-
-    /* Clear the circ_is_active bit to start */
-    circ_is_active = 0;
-
-    /* Assert that we know which direction this is going */
-    tor_assert((*i)->muxinfo.direction == CELL_DIRECTION_OUT ||
-               (*i)->muxinfo.direction == CELL_DIRECTION_IN);
-
-    if ((*i)->muxinfo.direction == CELL_DIRECTION_OUT) {
-      /* We should be n_mux on this circuit */
-      tor_assert(cmux == circ->n_mux);
-      tor_assert(chan == circ->n_chan);
-      /* Get next and prev for next test */
-      next_p = &(circ->next_active_on_n_chan);
-      prev_p = &(circ->prev_active_on_n_chan);
-    } else {
-      /* This should be an or_circuit_t and we should be p_mux */
-      or_circ = TO_OR_CIRCUIT(circ);
-      tor_assert(cmux == or_circ->p_mux);
-      tor_assert(chan == or_circ->p_chan);
-      /* Get next and prev for next test */
-      next_p = &(or_circ->next_active_on_p_chan);
-      prev_p = &(or_circ->prev_active_on_p_chan);
-    }
+    if (circ) {
+      /* Clear the circ_is_active bit to start */
+      circ_is_active = 0;
+
+      /* Assert that we know which direction this is going */
+      tor_assert((*i)->muxinfo.direction == CELL_DIRECTION_OUT ||
+                 (*i)->muxinfo.direction == CELL_DIRECTION_IN);
+
+      if ((*i)->muxinfo.direction == CELL_DIRECTION_OUT) {
+        /* We should be n_mux on this circuit */
+        tor_assert(cmux == circ->n_mux);
+        tor_assert(chan == circ->n_chan);
+        /* Get next and prev for next test */
+        next_p = &(circ->next_active_on_n_chan);
+        prev_p = &(circ->prev_active_on_n_chan);
+      } else {
+        /* This should be an or_circuit_t and we should be p_mux */
+        or_circ = TO_OR_CIRCUIT(circ);
+        tor_assert(cmux == or_circ->p_mux);
+        tor_assert(chan == or_circ->p_chan);
+        /* Get next and prev for next test */
+        next_p = &(or_circ->next_active_on_p_chan);
+        prev_p = &(or_circ->prev_active_on_p_chan);
+      }
 
-    /*
-     * Should this circuit be active?  I.e., does the mux know about > 0
-     * cells on it?
-     */
-    circ_is_active = ((*i)->muxinfo.cell_count > 0);
-
-    /* It should be in the linked list iff it's active */
-    if (circ_is_active) {
-      /* Either we have a next link or we are the tail */
-      tor_assert(*next_p || (circ == cmux->active_circuits_tail));
-      /* Either we have a prev link or we are the head */
-      tor_assert(*prev_p || (circ == cmux->active_circuits_head));
-      /* Increment the active circuits counter */
-      ++n_active_circuits;
+      /*
+       * Should this circuit be active?  I.e., does the mux know about > 0
+       * cells on it?
+       */
+      circ_is_active = ((*i)->muxinfo.cell_count > 0);
+
+      /* It should be in the linked list iff it's active */
+      if (circ_is_active) {
+        /* Either we have a next link or we are the tail */
+        tor_assert(*next_p || (circ == cmux->active_circuits_tail));
+        /* Either we have a prev link or we are the head */
+        tor_assert(*prev_p || (circ == cmux->active_circuits_head));
+        /* Increment the active circuits counter */
+        ++n_active_circuits;
+      } else {
+        /* Shouldn't be in list, so no next or prev link */
+        tor_assert(!(*next_p));
+        tor_assert(!(*prev_p));
+        /* And can't be head or tail */
+        tor_assert(circ != cmux->active_circuits_head);
+        tor_assert(circ != cmux->active_circuits_tail);
+      }
     } else {
-      /* Shouldn't be in list, so no next or prev link */
-      tor_assert(!(*next_p));
-      tor_assert(!(*prev_p));
-      /* And can't be head or tail */
-      tor_assert(circ != cmux->active_circuits_head);
-      tor_assert(circ != cmux->active_circuits_tail);
+      /*
+       * circuit_get_by_circid_channel() doesn't want us to have a circuit
+       * that's marked for close, but we can test for that case with
+       * circuit_id_in_use_on_channel().  Assert if it really, really isn't
+       * there.
+       */
+      tor_assert(circuit_id_in_use_on_channel(circ_id, chan));
+      /* It definitely isn't active */
+      circ_is_active = 0;
     }
 
     /* Increment the circuits counter */