Fix circuitmux attach/detach logic in circuit_set_circid_chan_helper(); it's possible for id to be zero (not assigned yet) and shouldn't be attached then

1 files changed, 21 insertions, 11 deletions

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index cfef0aa55..a4dcec68a 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -100,7 +100,7 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
   chan_circid_circuit_map_t *found;
   channel_t *old_chan, **chan_ptr;
   circid_t old_id, *circid_ptr;
-  int was_active, make_active;
+  int was_active, make_active, attached = 0;
 
   if (direction == CELL_DIRECTION_OUT) {
     chan_ptr = &circ->n_chan;
@@ -128,7 +128,19 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
     _last_circid_chan_ent = NULL;
   }
 
-  if (old_chan) { /* we may need to remove it from the conn-circid map */
+  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).
+     */
+    if (id != 0 && (old_chan != chan || old_id != id)) {
+      tor_assert(old_chan->cmux);
+      circuitmux_detach_circuit(old_chan->cmux, circ);
+    }
+
+    /* we may need to remove it from the conn-circid map */
     search.circ_id = old_id;
     search.chan = old_chan;
     found = HT_REMOVE(chan_circid_map, &chan_circid_map, &search);
@@ -142,12 +154,6 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
         --(old_chan->num_p_circuits);
       }
     }
-
-    /* If we're changing channels, detach the circuit */
-    if (old_chan != chan) {
-      tor_assert(old_chan->cmux);
-      circuitmux_detach_circuit(old_chan->cmux, circ);
-    }
   }
 
   /* Change the values only after we have possibly made the circuit inactive
@@ -172,17 +178,21 @@ circuit_set_circid_chan_helper(circuit_t *circ, int direction,
     HT_INSERT(chan_circid_map, &chan_circid_map, found);
   }
 
-  /* Attach to the circuitmux if we're changing channels */
-  if (old_chan != chan) {
+  /*
+   * Attach to the circuitmux if we're changing channels or IDs and
+   * have a new channel and ID to use.
+   */
+  if (chan && id != 0 && (old_chan != chan || old_id != id)) {
     tor_assert(chan->cmux);
     circuitmux_attach_circuit(chan->cmux, circ, direction);
+    attached = 1;
   }
 
   /*
    * This is a no-op if we have no cells, but if we do it marks us active to
    * the circuitmux
    */
-  if (make_active && old_chan != chan)
+  if (make_active && attached)
     update_circuit_on_cmux(circ, direction);
 
   /* Adjust circuit counts on new channel */