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.

diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c
index b10c1402536e57f88c11ae7ce864a9bae05db35d..6956cf9849686d7d6ac44f09e2c0099e7eaa07f9 100644 (file)
--- 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 ee6ab8596c5aa9010785a507a0e0d212b8ebfcde..e1eeb2f64f36db4fc02618873b01cb56301d813a 100644 (file)
--- 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 <b>he</b> */
 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 1816f2a4630e3557423e1b626720800f35c0ec6e..59fc17dea56001abc042b506b97fa7c8fefbc964 100644 (file)
--- 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;