diff --git a/changes/bug1184 b/changes/bug1184 new file mode 100644 index 0000000000..856cdc644d --- /dev/null +++ b/changes/bug1184 @@ -0,0 +1,9 @@ + 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. + - Never queue a cell for a circuit that's already been marked + for close. + diff --git a/doc/spec/tor-spec.txt b/doc/spec/tor-spec.txt index 2b1223ba7a..91ad561b8d 100644 --- a/doc/spec/tor-spec.txt +++ b/doc/spec/tor-spec.txt @@ -597,6 +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 has any RELAY cells + 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 are able, act as follows: the node closer to the OP should send a 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 686037f2de..4d82da63e9 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1323,10 +1323,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 935fad9200..a4f7718d90 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -1194,6 +1194,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); @@ -2377,6 +2378,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; @@ -2479,6 +2483,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 66d17579a3..08a1ffe789 100644 --- a/src/or/relay.h +++ b/src/or/relay.h @@ -62,6 +62,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