r12332@catbus: nickm | 2007-04-10 12:24:45 -0400

Yet another attempted Bug 411 fix: Under some circumstances, a circuit can have cells without being active.  The likeliest is that it has been unlinked from all connections in preparation for closing.  Therefore, stop enforcing this non-invariant.


svn:r9936

src/or/circuitlist.c

@@ -80,21 +80,25 @@ orconn_circid_circuit_map_t *_last_circid_orconn_ent = NULL;
 static void
 circuit_set_circid_orconn_helper(circuit_t *circ, int direction,
                                  uint16_t id,
-                                 or_connection_t *conn,
-                                 int active)
+                                 or_connection_t *conn)
 {
   orconn_circid_circuit_map_t search;
   orconn_circid_circuit_map_t *found;
   or_connection_t *old_conn, **conn_ptr;
   uint16_t old_id, *circid_ptr;
+  int was_active, make_active;
 
   if (direction == CELL_DIRECTION_OUT) {
     conn_ptr = &circ->n_conn;
     circid_ptr = &circ->n_circ_id;
+    was_active = circ->next_active_on_n_conn != NULL;
+    make_active = circ->n_conn_cells.n > 0;
   } else {
     or_circuit_t *c = TO_OR_CIRCUIT(circ);
     conn_ptr = &c->p_conn;
     circid_ptr = &c->p_circ_id;
+    was_active = c->next_active_on_p_conn != NULL;
+    make_active = c->p_conn_cells.n > 0;
   }
   old_conn = *conn_ptr;
   old_id = *circid_ptr;
@@ -119,7 +123,7 @@ circuit_set_circid_orconn_helper(circuit_t *circ, int direction,
       tor_free(found);
       --old_conn->n_circuits;
     }
-    if (active && old_conn != conn)
+    if (was_active && old_conn != conn)
       make_circuit_inactive_on_conn(circ,old_conn);
   }
 
@@ -144,7 +148,7 @@ circuit_set_circid_orconn_helper(circuit_t *circ, int direction,
     found->circuit = circ;
     HT_INSERT(orconn_circid_map, &orconn_circid_circuit_map, found);
   }
-  if (active && old_conn != conn)
+  if (make_active && old_conn != conn)
     make_circuit_active_on_conn(circ,conn);
 
   ++conn->n_circuits;
@@ -157,15 +161,11 @@ void
 circuit_set_p_circid_orconn(or_circuit_t *circ, uint16_t id,
                             or_connection_t *conn)
 {
-  int active = circ->p_conn_cells.n > 0;
-
-  tor_assert(bool_eq(active, circ->next_active_on_p_conn));
-
   circuit_set_circid_orconn_helper(TO_CIRCUIT(circ), CELL_DIRECTION_IN,
-                                   id, conn, active);
+                                   id, conn);
 
   if (conn)
-    tor_assert(bool_eq(active, circ->next_active_on_p_conn));
+    tor_assert(bool_eq(circ->p_conn_cells.n, circ->next_active_on_p_conn));
 }
 
 /** Set the n_conn field of a circuit <b>circ</b>, along
@@ -175,15 +175,10 @@ void
 circuit_set_n_circid_orconn(circuit_t *circ, uint16_t id,
                             or_connection_t *conn)
 {
-  int active = circ->n_conn_cells.n > 0;
-
-  tor_assert(bool_eq(active, circ->next_active_on_n_conn));
-
-  circuit_set_circid_orconn_helper(circ, CELL_DIRECTION_OUT,
-                                   id, conn, active);
+  circuit_set_circid_orconn_helper(circ, CELL_DIRECTION_OUT, id, conn);
 
   if (conn)
-    tor_assert(bool_eq(active, circ->next_active_on_n_conn));
+    tor_assert(bool_eq(circ->n_conn_cells.n, circ->next_active_on_n_conn));
 }
 
 /** Change the state of <b>circ</b> to <b>state</b>, adding it to or removing

src/or/or.h

@@ -1430,10 +1430,12 @@ typedef struct circuit_t {
                                       * circuit marked for close? */
 
   /** Next circuit in the doubly-linked ring of circuits waiting to add
-   * cells to n_conn.  NULL if we have no cells pending. */
+   * cells to n_conn.  NULL if we have no cells pending, or if we're not
+   * linked to an OR connection. */
   struct circuit_t *next_active_on_n_conn;
   /** Previous circuit in the doubly-linked ring of circuits waiting to add
-   * cells to n_conn.  NULL if we have no cells pending. */
+   * cells to n_conn.  NULL if we have no cells pending, or if we're not
+   * linked to an OR connection. */
   struct circuit_t *prev_active_on_n_conn;
   struct circuit_t *next; /**< Next circuit in linked list of all circuits. */
 } circuit_t;
@@ -1490,10 +1492,12 @@ typedef struct or_circuit_t {
   circuit_t _base;
 
   /** Next circuit in the doubly-linked ring of circuits waiting to add
-   * cells to p_conn.  NULL if we have no cells pending. */
+   * cells to p_conn.  NULL if we have no cells pending, or if we're not
+   * linked to an OR connection. */
   struct circuit_t *next_active_on_p_conn;
   /** Previous circuit in the doubly-linked ring of circuits waiting to add
-   * cells to p_conn.  NULL if we have no cells pending. */
+   * cells to p_conn.  NULL if we have no cells pending, or if we're not
+   * linked to an OR connection. */
   struct circuit_t *prev_active_on_p_conn;
 
   /** The circuit_id used in the previous (backward) hop of this circuit. */

src/or/relay.c

@@ -1581,35 +1581,46 @@ prev_circ_on_conn_p(circuit_t *circ, or_connection_t *conn)
 }
 
 /** Add <b>circ</b> to the list of circuits with pending cells on
- * <b>conn</b>. */
+ * <b>conn</b>.   No effect if <b>circ</b> is already unlinked. */
 void
 make_circuit_active_on_conn(circuit_t *circ, or_connection_t *conn)
 {
-  tor_assert(! *prev_circ_on_conn_p(circ, conn));
-  tor_assert(! *next_circ_on_conn_p(circ, conn));
+  circuit_t **nextp = next_circ_on_conn_p(circ, conn);
+  circuit_t **prevp = prev_circ_on_conn_p(circ, conn);
+
+  if (*nextp && *prevp) {
+    /* Already active. */
+    return;
+  }
 
   if (! conn->active_circuits) {
     conn->active_circuits = circ;
-    *prev_circ_on_conn_p(circ, conn) = circ;
-    *next_circ_on_conn_p(circ, conn) = circ;
+    *prevp = *nextp = circ;
   } else {
     circuit_t *head = conn->active_circuits;
     circuit_t *old_tail = *prev_circ_on_conn_p(head, conn);
     *next_circ_on_conn_p(old_tail, conn) = circ;
-    *next_circ_on_conn_p(circ, conn) = head;
+    *nextp = head;
     *prev_circ_on_conn_p(head, conn) = circ;
-    *prev_circ_on_conn_p(circ, conn) = old_tail;
+    *prevp = old_tail;
   }
   assert_active_circuits_ok_paranoid(conn);
 }
 
 /** Remove <b>circ</b> to the list of circuits with pending cells on
- * <b>conn</b>. */
+ * <b>conn</b>.  No effect if <b>circ</b> is already unlinked. */
 void
 make_circuit_inactive_on_conn(circuit_t *circ, or_connection_t *conn)
 {
-  circuit_t *next = *next_circ_on_conn_p(circ, conn);
-  circuit_t *prev = *prev_circ_on_conn_p(circ, conn);
+  circuit_t **nextp = next_circ_on_conn_p(circ, conn);
+  circuit_t **prevp = prev_circ_on_conn_p(circ, conn);
+  circuit_t *next = *nextp, *prev = *prevp;
+
+  if (!next && !prev) {
+    /* Already inactive. */
+    return;
+  }
+
   tor_assert(next && prev);
   tor_assert(*prev_circ_on_conn_p(next, conn) == circ);
   tor_assert(*next_circ_on_conn_p(prev, conn) == circ);
@@ -1622,8 +1633,7 @@ make_circuit_inactive_on_conn(circuit_t *circ, or_connection_t *conn)
     if (conn->active_circuits == circ)
       conn->active_circuits = next;
   }
-  *prev_circ_on_conn_p(circ, conn) = NULL;
-  *next_circ_on_conn_p(circ, conn) = NULL;
+  *prevp = *nextp = NULL;
   assert_active_circuits_ok_paranoid(conn);
 }