From 7c70f713c31c0989a0008c7d0d92a1f12d498e32 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 23 Mar 2023 20:52:13 +0000 Subject: [PATCH] Avoid closing dirty circs with active half-edges In https://gitlab.torproject.org/tpo/core/tor/-/issues/40623, we changed the DESTROY propogation to ensure memory was freed quickly at relays. This was a good move, but it exacerbates the condition where a stream is closed on a circuit, and then it is immediately closed because it is dirty. This creates a race between the DESTROY and the last data sent on the stream. This race is visible in shadow, and does happen. This could be backported. A better solution to these kinds of problems is to create an ENDED cell, and not close any circuits until the ENDED comes back. But this will also require thinking, since this ENDED cell can also get lost, so some kind of timeout may be needed either way. The ENDED cell could just allow us to have much longer timeouts for this case. --- src/core/or/circuituse.c | 1 + src/core/or/connection_edge.c | 19 +++++++++++++++++++ src/core/or/connection_edge.h | 1 + 3 files changed, 21 insertions(+) diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index b10c140253..6956cf9849 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -1458,6 +1458,7 @@ circuit_expire_old_circuits_clientside(void) if (circ->timestamp_dirty && circ->timestamp_dirty + get_options()->MaxCircuitDirtiness < now.tv_sec && + !connection_half_edges_waiting(TO_ORIGIN_CIRCUIT(circ)) && !TO_ORIGIN_CIRCUIT(circ)->p_streams /* nothing attached */ ) { log_debug(LD_CIRC, "Closing n_circ_id %u (dirty %ld sec ago, " "purpose %d)", diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index ee6ab8596c..e1eeb2f64f 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -675,6 +675,25 @@ connection_half_edge_add(const edge_connection_t *conn, smartlist_insert(circ->half_streams, insert_at, half_conn); } +/** + * Return true if the circuit has any half-closed connections + * that are still within the end_ack_expected_usec timestamp + * from now. + */ +bool +connection_half_edges_waiting(const origin_circuit_t *circ) +{ + if (!circ->half_streams) + return false; + + SMARTLIST_FOREACH_BEGIN(circ->half_streams, const half_edge_t *, half_conn) { + if (half_conn->end_ack_expected_usec > monotime_absolute_usec()) + return true; + } SMARTLIST_FOREACH_END(half_conn); + + return false; +} + /** Release space held by he */ void half_edge_free_(half_edge_t *he) diff --git a/src/core/or/connection_edge.h b/src/core/or/connection_edge.h index 1816f2a463..59fc17dea5 100644 --- a/src/core/or/connection_edge.h +++ b/src/core/or/connection_edge.h @@ -218,6 +218,7 @@ int connection_half_edge_is_valid_end(smartlist_t *half_conns, streamid_t stream_id); int connection_half_edge_is_valid_resolved(smartlist_t *half_conns, streamid_t stream_id); +bool connection_half_edges_waiting(const origin_circuit_t *circ); size_t half_streams_get_total_allocation(void); struct half_edge_t;