hs-v3: Cleanup HS circuits when marking as closed

First, hs_service_intro_circ_has_closed() is now called in circuit_mark_for
close() because the HS subsystem needs to learn when an intro point is
actually not established anymore as soon as possible. There is a time window
between a close and a free.

Second, when we mark for close, we also remove it from the circuitmap because
between the close and the free, a service can launch an new circuit to that
same intro point and thus register it which only succeeds if the intro point
authentication key is not already in the map.

However, we still do a remove from the circuitmap in circuit_free() in order
to also cleanup the circuit if it wasn't marked for close prior to the free.

Fixes #23603

Signed-off-by: David Goulet <dgoulet@torproject.org>

1 files changed, 13 insertions, 13 deletions

diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c
index d37d986f17..45d4521c22 100644
--- a/src/or/circuitlist.c
+++ b/src/or/circuitlist.c
@@ -67,7 +67,6 @@
 #include "main.h"
 #include "hs_circuit.h"
 #include "hs_circuitmap.h"
-#include "hs_common.h"
 #include "hs_ident.h"
 #include "networkstatus.h"
 #include "nodelist.h"
@@ -938,6 +937,12 @@ circuit_free(circuit_t *circ)
 
   circuit_clear_testing_cell_stats(circ);
 
+  /* Cleanup circuit from anything HS v3 related. We also do this when the
+   * circuit is closed. This is to avoid any code path that free registered
+   * circuits without closing them before. This needs to be done before the
+   * hs identifier is freed. */
+  hs_circ_cleanup(circ);
+
   if (CIRCUIT_IS_ORIGIN(circ)) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     mem = ocirc;
@@ -963,7 +968,11 @@ circuit_free(circuit_t *circ)
 
     crypto_pk_free(ocirc->intro_key);
     rend_data_free(ocirc->rend_data);
+
+    /* Finally, free the identifier of the circuit and nullify it so multiple
+     * cleanup will work. */
     hs_ident_circuit_free(ocirc->hs_ident);
+    ocirc->hs_ident = NULL;
 
     tor_free(ocirc->dest_address);
     if (ocirc->socks_username) {
@@ -1022,11 +1031,6 @@ circuit_free(circuit_t *circ)
   /* Remove from map. */
   circuit_set_n_circid_chan(circ, 0, NULL);
 
-  /* Clear HS circuitmap token from this circ (if any) */
-  if (circ->hs_token) {
-    hs_circuitmap_remove_circuit(circ);
-  }
-
   /* Clear cell queue _after_ removing it from the map.  Otherwise our
    * "active" checks will be violated. */
   cell_queue_clear(&circ->n_chan_cells);
@@ -1917,6 +1921,9 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
     }
   }
 
+  /* Notify the HS subsystem that this circuit is closing. */
+  hs_circ_cleanup(circ);
+
   if (circuits_pending_close == NULL)
     circuits_pending_close = smartlist_new();
 
@@ -1997,13 +2004,6 @@ circuit_about_to_free(circuit_t *circ)
      orig_reason);
   }
 
-  /* Notify the HS subsystem for any intro point circuit closing so it can be
-   * dealt with cleanly. */
-  if (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO ||
-      circ->purpose == CIRCUIT_PURPOSE_S_INTRO) {
-    hs_service_intro_circ_has_closed(TO_ORIGIN_CIRCUIT(circ));
-  }
-
   if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
     int timed_out = (reason == END_CIRC_REASON_TIMEOUT);