[or-cvs] r9915: Fix a crash bug in cell queues: It is possible for a connect (tor/trunk/src/or)

[nickm@xxxxxxxx]

Author: nickm
Date: 2007-03-28 22:41:36 -0400 (Wed, 28 Mar 2007)
New Revision: 9915

Modified:
   tor/trunk/src/or/circuitlist.c
   tor/trunk/src/or/relay.c
Log:
Fix a crash bug in cell queues: It is possible for a connection_write_to_buf to close the connection or otherwise unlink the circuit, which makes the circuit nonactive, which invalidates the pointer from the circuit to the next circuit on the active ring.  Also add a bunch of asserts, most #ifdefed out.

Modified: tor/trunk/src/or/circuitlist.c
===================================================================
--- tor/trunk/src/or/circuitlist.c	2007-03-29 02:37:06 UTC (rev 9914)
+++ tor/trunk/src/or/circuitlist.c	2007-03-29 02:41:36 UTC (rev 9915)
@@ -103,7 +103,7 @@
       tor_free(found);
       --old_conn->n_circuits;
     }
-    if (active)
+    if (active && old_conn != conn)
       make_circuit_inactive_on_conn(circ,old_conn);
   }
 
@@ -123,7 +123,7 @@
     found->circuit = circ;
     HT_INSERT(orconn_circid_map, &orconn_circid_circuit_map, found);
   }
-  if (active)
+  if (active && old_conn != conn)
     make_circuit_active_on_conn(circ,conn);
 
   ++conn->n_circuits;
@@ -145,6 +145,7 @@
   circ->p_circ_id = id;
   circ->p_conn = conn;
   active = circ->p_conn_cells.n > 0;
+  tor_assert(bool_eq(active, circ->next_active_on_p_conn));
 
   if (id == old_id && conn == old_conn)
     return;
@@ -168,6 +169,7 @@
   circ->n_circ_id = id;
   circ->n_conn = conn;
   active = circ->n_conn_cells.n > 0;
+  tor_assert(bool_eq(active, circ->next_active_on_n_conn));
 
   if (id == old_id && conn == old_conn)
     return;

Modified: tor/trunk/src/or/relay.c
===================================================================
--- tor/trunk/src/or/relay.c	2007-03-29 02:37:06 UTC (rev 9914)
+++ tor/trunk/src/or/relay.c	2007-03-29 02:41:36 UTC (rev 9915)
@@ -1470,6 +1470,13 @@
  * cells. */
 #define CELL_QUEUE_LOWWATER_SIZE 64
 
+#ifdef ACTIVE_CIRCUITS_PARANOIA
+#define assert_active_circuits_ok_paranoid(conn) \
+     assert_active_circuits_ok(conn)
+#else
+#define assert_active_circuits_ok_paranoid(conn)
+#endif
+
 /** Release storage held by <b>cell</b> */
 static INLINE void
 cell_free(cell_t *cell)
@@ -1546,6 +1553,8 @@
 static INLINE circuit_t **
 next_circ_on_conn_p(circuit_t *circ, or_connection_t *conn)
 {
+  tor_assert(circ);
+  tor_assert(conn);
   if (conn == circ->n_conn) {
     return &circ->next_active_on_n_conn;
   } else {
@@ -1560,6 +1569,8 @@
 static INLINE circuit_t **
 prev_circ_on_conn_p(circuit_t *circ, or_connection_t *conn)
 {
+  tor_assert(circ);
+  tor_assert(conn);
   if (conn == circ->n_conn) {
     return &circ->prev_active_on_n_conn;
   } else {
@@ -1589,6 +1600,7 @@
     *prev_circ_on_conn_p(head, conn) = circ;
     *prev_circ_on_conn_p(circ, conn) = old_tail;
   }
+  assert_active_circuits_ok_paranoid(conn);
 }
 
 /** Remove <b>circ</b> to the list of circuits with pending cells on
@@ -1598,6 +1610,7 @@
 {
   circuit_t *next = *next_circ_on_conn_p(circ, conn);
   circuit_t *prev = *prev_circ_on_conn_p(circ, conn);
+  tor_assert(next && prev);
   tor_assert(*prev_circ_on_conn_p(next, conn) == circ);
   tor_assert(*next_circ_on_conn_p(prev, conn) == circ);
 
@@ -1611,6 +1624,7 @@
   }
   *prev_circ_on_conn_p(circ, conn) = NULL;
   *next_circ_on_conn_p(circ, conn) = NULL;
+  assert_active_circuits_ok_paranoid(conn);
 }
 
 /** Remove all circuits from the list of circuits with pending cells on
@@ -1677,6 +1691,7 @@
   int streams_blocked;
   circ = conn->active_circuits;
   if (!circ) return 0;
+  assert_active_circuits_ok_paranoid(conn);
   if (circ->n_conn == conn) {
     queue = &circ->n_conn_cells;
     streams_blocked = circ->streams_blocked_on_n_conn;
@@ -1684,14 +1699,27 @@
     queue = &TO_OR_CIRCUIT(circ)->p_conn_cells;
     streams_blocked = circ->streams_blocked_on_p_conn;
   }
+  tor_assert(*next_circ_on_conn_p(circ,conn));
 
   for (n_flushed = 0; n_flushed < max && queue->head; ++n_flushed) {
     cell_t *cell = cell_queue_pop(queue);
+    tor_assert(*next_circ_on_conn_p(circ,conn));
 
     connection_or_write_cell_to_buf(cell, conn);
     cell_free(cell);
     ++n_flushed;
+    if (circ != conn->active_circuits) {
+      /* If this happens, the current circuit just got made inactive by
+       * a call in connection_write_to_buf().  That's nothing to worry about:
+       * circuit_make_inactive_on_conn() already advanced conn->active_circuits
+       * for us.
+       */
+      assert_active_circuits_ok_paranoid(conn);
+      return n_flushed;
+    }
   }
+  tor_assert(*next_circ_on_conn_p(circ,conn));
+  assert_active_circuits_ok_paranoid(conn);
   conn->active_circuits = *next_circ_on_conn_p(circ, conn);
 
   /* Is the cell queue low enough to unblock all the streams that are waiting