From 6f45101327592333dcc54e08800fbc2cb68ccd49 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 30 Jul 2010 18:55:24 -0400 Subject: [PATCH 1/5] Clear cell queues when marking or truncating a circuit. 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 0000000000..003ad0d916 --- /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 f448f6da2c..5283442fe9 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 c581365f8b..fa800db1a4 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 405df1578b..c94325a5b7 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 22ecdaafa0..e740fbf595 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 circ for orconn. */ +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 orconn is * corrupt. */ void diff --git a/src/or/relay.h b/src/or/relay.h index 73855a52bf..7fb0655ef7 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 From c4b83b21776bd2c205d038ded5a7e5260a1c39df Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 2 Aug 2010 12:28:25 -0400 Subject: [PATCH 2/5] Clarify that TRUNCATE behavior isn't as-intended In tor-spec.txt, instead of saying "nodes may X" instead say "Current nodes do X; this is nonconformant. Clients should watch out for that." Based on observations by wanoskarnet. --- doc/spec/tor-spec.txt | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/doc/spec/tor-spec.txt b/doc/spec/tor-spec.txt index 5283442fe9..d0c60c0e6a 100644 --- a/doc/spec/tor-spec.txt +++ b/doc/spec/tor-spec.txt @@ -595,9 +595,15 @@ 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. 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. + RELAY_TRUNCATED cell. + + [Note: If an OR receives a TRUNCATE cell and it any RELAY cells queued on + the circuit for the next node in that it had not yet sent, it will drop + them without sending them. This is not considered conformant behavior, + but it probably won't get fixed till a later versions of Tor. Thus, + clients SHOULD NOT send a TRUNCATE cell to a node running any current + version of Tor if they have sent relay cells through that node, and they + aren't sure whether those cells have been sent on.] When an unrecoverable error occurs along one connection in a circuit, the nodes on either side of the connection should, if they From 4e3373f7fe24c281b88c9a139854afa1c213d163 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Tue, 3 Aug 2010 17:28:19 +0200 Subject: [PATCH 3/5] Make tor-spec wording easier to understand --- doc/spec/tor-spec.txt | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/doc/spec/tor-spec.txt b/doc/spec/tor-spec.txt index d0c60c0e6a..314169f78a 100644 --- a/doc/spec/tor-spec.txt +++ b/doc/spec/tor-spec.txt @@ -597,13 +597,14 @@ see tor-design.pdf. cell to the next node in the circuit, and replies to the OP with a RELAY_TRUNCATED cell. - [Note: If an OR receives a TRUNCATE cell and it any RELAY cells queued on - the circuit for the next node in that it had not yet sent, it will drop - them without sending them. This is not considered conformant behavior, - but it probably won't get fixed till a later versions of Tor. Thus, - clients SHOULD NOT send a TRUNCATE cell to a node running any current - version of Tor if they have sent relay cells through that node, and they - aren't sure whether those cells have been sent on.] + [Note: If an OR receives a TRUNCATE cell and it has any RELAY cells + still queued on the circuit for the next node that it had not yet sent, + it will drop them without sending them. This is not considered + conformant behavior, but it probably won't get fixed till a later + version of Tor. Thus, clients SHOULD NOT send a TRUNCATE cell to a + node running any current version of Tor if they have sent relay cells + through that node, and they aren't sure whether those cells have been + sent on.] When an unrecoverable error occurs along one connection in a circuit, the nodes on either side of the connection should, if they From 87f18c9578323cbb9229aba9a271bc7a6362a07a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Sep 2010 13:06:54 -0400 Subject: [PATCH 4/5] Never queue a cell on a marked circuit --- changes/bug1184 | 2 ++ src/or/relay.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/changes/bug1184 b/changes/bug1184 index 003ad0d916..856cdc644d 100644 --- a/changes/bug1184 +++ b/changes/bug1184 @@ -4,4 +4,6 @@ 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. + - Never queue a cell for a circuit that's already been marked + for close. diff --git a/src/or/relay.c b/src/or/relay.c index e740fbf595..6d51f18a3a 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2272,6 +2272,9 @@ append_cell_to_circuit_queue(circuit_t *circ, or_connection_t *orconn, { cell_queue_t *queue; int streams_blocked; + if (circ->marked_for_close) + return; + if (direction == CELL_DIRECTION_OUT) { queue = &circ->n_conn_cells; streams_blocked = circ->streams_blocked_on_n_conn; From 69508d04a2db530c0bbb4c1cc421e6b258b822be Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Sep 2010 13:08:44 -0400 Subject: [PATCH 5/5] tor-spec.txt tweaks from arma --- doc/spec/tor-spec.txt | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/doc/spec/tor-spec.txt b/doc/spec/tor-spec.txt index 314169f78a..a08005c6da 100644 --- a/doc/spec/tor-spec.txt +++ b/doc/spec/tor-spec.txt @@ -598,13 +598,12 @@ see tor-design.pdf. RELAY_TRUNCATED cell. [Note: If an OR receives a TRUNCATE cell and it has any RELAY cells - still queued on the circuit for the next node that it had not yet sent, - it will drop them without sending them. This is not considered - conformant behavior, but it probably won't get fixed till a later - version of Tor. Thus, clients SHOULD NOT send a TRUNCATE cell to a - node running any current version of Tor if they have sent relay cells - through that node, and they aren't sure whether those cells have been - sent on.] + still queued on the circuit for the next node it will drop them + without sending them. This is not considered conformant behavior, + but it probably won't get fixed until a later version of Tor. Thus, + clients SHOULD NOT send a TRUNCATE cell to a node running any current + version of Tor if a) they have sent relay cells through that node, + and b) they aren't sure whether those cells have been sent on yes.] When an unrecoverable error occurs along one connection in a circuit, the nodes on either side of the connection should, if they