[or-cvs] [tor/master 1/5] Clear cell queues when marking or truncating a circuit.

[nickm at torproject.org]

Author: Nick Mathewson <nickm at torproject.org>
Date: Fri, 30 Jul 2010 18:55:24 -0400
Subject: Clear cell queues when marking or truncating a circuit.
Commit: 6f45101327592333dcc54e08800fbc2cb68ccd49

At best, this patch helps us avoid sending queued relayed cells that
would get ignored during the time between when a destroy cell is
sent and when the circuit is finally freed.  At worst, it lets us
release some memory a little earlier than it would otherwise.

Fix for bug #1184.  Bugfix on 0.2.0.1-alpha.
---
 changes/bug1184        |    7 +++++++
 doc/spec/tor-spec.txt  |    4 +++-
 src/or/circuitlist.c   |    8 ++++++--
 src/or/connection_or.c |    4 ----
 src/or/relay.c         |   20 ++++++++++++++++++++
 src/or/relay.h         |    1 +
 6 files changed, 37 insertions(+), 7 deletions(-)
 create mode 100644 changes/bug1184

diff --git a/changes/bug1184 b/changes/bug1184
new file mode 100644
index 0000000..003ad0d
--- /dev/null
+++ b/changes/bug1184
@@ -0,0 +1,7 @@
+  o Minor bugfixes:
+    - Never relay a cell for a circuit we have already destroyed.
+      Between marking a circuit as closeable and finally closing it,
+      it may have been possible for a few queued cells to get relayed,
+      even though they would have been immediately dropped by the next
+      OR in the circuit.  Fix 1184; bugfix on 0.2.0.1-alpha.
+
diff --git a/doc/spec/tor-spec.txt b/doc/spec/tor-spec.txt
index f448f6d..5283442 100644
--- a/doc/spec/tor-spec.txt
+++ b/doc/spec/tor-spec.txt
@@ -595,7 +595,9 @@ see tor-design.pdf.
    To tear down part of a circuit, the OP may send a RELAY_TRUNCATE cell
    signaling a given OR (Stream ID zero).  That OR sends a DESTROY
    cell to the next node in the circuit, and replies to the OP with a
-   RELAY_TRUNCATED cell.
+   RELAY_TRUNCATED cell.  If the OR has any RELAY cells queued on the
+   circuit for the next node in that it had not yet sent, it MAY
+   drop them without sending them.
 
    When an unrecoverable error occurs along one connection in a
    circuit, the nodes on either side of the connection should, if they
diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index c581365..fa800db 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -1124,8 +1124,10 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line,
     rend_client_remove_intro_point(ocirc->build_state->chosen_exit,
                                    ocirc->rend_data);
   }
-  if (circ->n_conn)
+  if (circ->n_conn) {
+    circuit_clear_cell_queue(circ, circ->n_conn);
     connection_or_send_destroy(circ->n_circ_id, circ->n_conn, reason);
+  }
 
   if (! CIRCUIT_IS_ORIGIN(circ)) {
     or_circuit_t *or_circ = TO_OR_CIRCUIT(circ);
@@ -1149,8 +1151,10 @@ _circuit_mark_for_close(circuit_t *circ, int reason, int line,
       conn->on_circuit = NULL;
     }
 
-    if (or_circ->p_conn)
+    if (or_circ->p_conn) {
+      circuit_clear_cell_queue(circ, or_circ->p_conn);
       connection_or_send_destroy(or_circ->p_circ_id, or_circ->p_conn, reason);
+    }
   } else {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     edge_connection_t *conn;
diff --git a/src/or/connection_or.c b/src/or/connection_or.c
index 405df15..c94325a 100644
--- a/src/or/connection_or.c
+++ b/src/or/connection_or.c
@@ -1291,10 +1291,6 @@ connection_or_send_destroy(circid_t circ_id, or_connection_t *conn, int reason)
   cell.payload[0] = (uint8_t) reason;
   log_debug(LD_OR,"Sending destroy (circID %d).", circ_id);
 
-  /* XXXX It's possible that under some circumstances, we want the destroy
-   * to take precedence over other data waiting on the circuit's cell queue.
-   */
-
   connection_or_write_cell_to_buf(&cell, conn);
   return 0;
 }
diff --git a/src/or/relay.c b/src/or/relay.c
index 22ecdaa..e740fbf 100644
--- a/src/or/relay.c
+++ b/src/or/relay.c
@@ -1186,6 +1186,7 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ,
       }
       if (circ->n_conn) {
         uint8_t trunc_reason = *(uint8_t*)(cell->payload + RELAY_HEADER_SIZE);
+        circuit_clear_cell_queue(circ, circ->n_conn);
         connection_or_send_destroy(circ->n_circ_id, circ->n_conn,
                                    trunc_reason);
         circuit_set_n_circid_orconn(circ, 0, NULL);
@@ -2368,6 +2369,25 @@ decode_address_from_payload(tor_addr_t *addr_out, const char *payload,
   return payload + 2 + (uint8_t)payload[1];
 }
 
+/** Remove all the cells queued on <b>circ</b> for <b>orconn</b>. */
+void
+circuit_clear_cell_queue(circuit_t *circ, or_connection_t *orconn)
+{
+  cell_queue_t *queue;
+  if (circ->n_conn == orconn) {
+    queue = &circ->n_conn_cells;
+  } else {
+    or_circuit_t *orcirc = TO_OR_CIRCUIT(circ);
+    tor_assert(orcirc->p_conn == orconn);
+    queue = &orcirc->p_conn_cells;
+  }
+
+  if (queue->n)
+    make_circuit_inactive_on_conn(circ,orconn);
+
+  cell_queue_clear(queue);
+}
+
 /** Fail with an assert if the active circuits ring on <b>orconn</b> is
  * corrupt.  */
 void
diff --git a/src/or/relay.h b/src/or/relay.h
index 73855a5..7fb0655 100644
--- a/src/or/relay.h
+++ b/src/or/relay.h
@@ -60,6 +60,7 @@ const char *decode_address_from_payload(tor_addr_t *addr_out,
 unsigned cell_ewma_get_tick(void);
 void cell_ewma_set_scale_factor(or_options_t *options,
                                 networkstatus_t *consensus);
+void circuit_clear_cell_queue(circuit_t *circ, or_connection_t *orconn);
 
 #endif
 
-- 
1.7.1