hs-v3: Give a cleanup type to hs_circ_cleanup()

By centralizing the circuit cleanup type that is: on close, free and
repurpose, some actions on the circuit can not happen for a certain cleanup
type or for all types.

This passes a cleanup type so the HS subsystem (v2 and v3) can take actions
based on the type of cleanup.

For instance, there is slow code that we do not run on a circuit close but
rather only on free.

Part of #32020

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

8 files changed, 88 insertions, 29 deletions

diff --git a/src/core/or/circuitlist.c b/src/core/or/circuitlist.c
index 3ae482a5b2..49a63c50a1 100644
--- a/src/core/or/circuitlist.c
+++ b/src/core/or/circuitlist.c
@@ -1137,7 +1137,7 @@ circuit_free_(circuit_t *circ)
    * 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);
+  hs_circ_cleanup_on_free(circ);
 
   if (CIRCUIT_IS_ORIGIN(circ)) {
     origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
@@ -2261,7 +2261,7 @@ circuit_mark_for_close_, (circuit_t *circ, int reason, int line,
   }
 
   /* Notify the HS subsystem that this circuit is closing. */
-  hs_circ_cleanup(circ);
+  hs_circ_cleanup_on_close(circ);
 
   if (circuits_pending_close == NULL)
     circuits_pending_close = smartlist_new();
diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c
index 8a667024dc..ad9841cc36 100644
--- a/src/core/or/circuituse.c
+++ b/src/core/or/circuituse.c
@@ -3124,7 +3124,7 @@ circuit_change_purpose(circuit_t *circ, uint8_t new_purpose)
     /* Take specific actions if we are repurposing a hidden service circuit. */
     if (circuit_purpose_is_hidden_service(circ->purpose) &&
         !circuit_purpose_is_hidden_service(new_purpose)) {
-      hs_circ_cleanup(circ);
+      hs_circ_cleanup_on_repurpose(circ);
     }
   }
 
diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c
index 8640129f3f..a43e7f0e23 100644
--- a/src/feature/hs/hs_circuit.c
+++ b/src/feature/hs/hs_circuit.c
@@ -621,16 +621,16 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip,
 }
 
 /** Helper: cleanup function for client circuit. This is for every HS version.
- * It is called from hs_circ_cleanup() entry point. */
+ * It is called from hs_circ_cleanup_on_free() entry point. */
 static void
-cleanup_client_circ(circuit_t *circ)
+cleanup_on_free_client_circ(circuit_t *circ)
 {
   tor_assert(circ);
 
   if (circuit_is_hs_v2(circ)) {
-    rend_client_circuit_cleanup(circ);
+    rend_client_circuit_cleanup_on_free(circ);
   } else if (circuit_is_hs_v3(circ)) {
-    hs_client_circuit_cleanup(circ);
+    hs_client_circuit_cleanup_on_free(circ);
   }
   /* It is possible the circuit has an HS purpose but no identifier (rend_data
    * or hs_ident). Thus possible that this passess through. */
@@ -1215,29 +1215,83 @@ hs_circ_send_establish_rendezvous(origin_circuit_t *circ)
   return -1;
 }
 
-/** We are about to close or free this <b>circ</b>. Clean it up from any
- * related HS data structures. This function can be called multiple times
- * safely for the same circuit. */
+/** Circuit cleanup strategy:
+ *
+ *  What follows is a series of functions that notifies the HS subsystem of 3
+ *  different circuit cleanup phase: close, free and repurpose.
+ *
+ *  Tor can call any of those in any orders so they have to be safe between
+ *  each other. In other words, the free should never depend on close to be
+ *  called before.
+ *
+ *  The "on_close()" is called from circuit_mark_for_close() which is
+ *  considered the tor fast path and thus as little work as possible should
+ *  done in that function. Currently, we only remove the circuit from the HS
+ *  circuit map and move on.
+ *
+ *  The "on_free()" is called from circuit circuit_free_() and it is very
+ *  important that at the end of the function, no state or objects related to
+ *  this circuit remains alive.
+ *
+ *  The "on_repurpose()" is called from circuit_change_purpose() for which we
+ *  simply remove it from the HS circuit map. We do not have other cleanup
+ *  requirements after that.
+ *
+ *  NOTE: The onion service code, specifically the service code, cleans up
+ *  lingering objects or state if any of its circuit disappear which is why
+ *  our cleanup strategy doesn't involve any service specific actions. As long
+ *  as the circuit is removed from the HS circuit map, it won't be used.
+ */
+
+/** We are about to close this <b>circ</b>. Clean it up from any related HS
+ * data structures. This function can be called multiple times safely for the
+ * same circuit. */
 void
-hs_circ_cleanup(circuit_t *circ)
+hs_circ_cleanup_on_close(circuit_t *circ)
 {
   tor_assert(circ);
 
+  /* On close, we simply remove it from the circuit map. It can not be used
+   * anymore. We keep this code path fast and lean. */
+
+  if (circ->hs_token) {
+    hs_circuitmap_remove_circuit(circ);
+  }
+}
+
+/** We are about to free this <b>circ</b>. Clean it up from any related HS
+ * data structures. This function can be called multiple times safely for the
+ * same circuit. */
+void
+hs_circ_cleanup_on_free(circuit_t *circ)
+{
+  tor_assert(circ);
+
+  /* NOTE: Bulk of the work of cleaning up a circuit is done here. */
+
   if (circuit_purpose_is_hs_client(circ->purpose)) {
-    cleanup_client_circ(circ);
+    cleanup_on_free_client_circ(circ);
+  }
+
+  /* We have no assurance that the given HS circuit has been closed before and
+   * thus removed from the HS map. This actually happens in unit tests. */
+  if (circ->hs_token) {
+    hs_circuitmap_remove_circuit(circ);
   }
+}
+
+/** We are about to repurpose this <b>circ</b>. Clean it up from any related
+ * HS data structures. This function can be called multiple times safely for
+ * the same circuit. */
+void
+hs_circ_cleanup_on_repurpose(circuit_t *circ)
+{
+  tor_assert(circ);
+
+  /* On repurpose, we simply remove it from the circuit map but we do not do
+   * the on_free actions since we don't treat a repurpose as something we need
+   * to report in the client cache failure. */
 
-  /* Actions that MUST happen for every circuits regardless of what was done
-   * on it before. */
-
-  /* Clear HS circuitmap token for this circ (if any). Very important to be
-   * done after the HS subsystem has been notified of the close else the
-   * circuit will not be found.
-   *
-   * We do this at the close if possible because from that point on, the
-   * circuit is good as dead. We can't rely on removing it in the circuit
-   * free() function because we open a race window between the close and free
-   * where we can't register a new circuit for the same intro point. */
   if (circ->hs_token) {
     hs_circuitmap_remove_circuit(circ);
   }
diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h
index c817f3e37a..42e5ca1348 100644
--- a/src/feature/hs/hs_circuit.h
+++ b/src/feature/hs/hs_circuit.h
@@ -14,8 +14,10 @@
 
 #include "feature/hs/hs_service.h"
 
-/* Cleanup function when the circuit is closed or/and freed. */
-void hs_circ_cleanup(circuit_t *circ);
+/* Cleanup function when the circuit is closed or freed. */
+void hs_circ_cleanup_on_close(circuit_t *circ);
+void hs_circ_cleanup_on_free(circuit_t *circ);
+void hs_circ_cleanup_on_repurpose(circuit_t *circ);
 
 /* Circuit API. */
 int hs_circ_service_intro_has_opened(hs_service_t *service,
diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index 716b6a26a2..a2a47c1da6 100644
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -1525,7 +1525,7 @@ get_hs_client_auths_map(void)
 /** Called when a circuit was just cleaned up. This is done right before the
  * circuit is freed. */
 void
-hs_client_circuit_cleanup(const circuit_t *circ)
+hs_client_circuit_cleanup_on_free(const circuit_t *circ)
 {
   bool has_timed_out;
   rend_intro_point_failure_t failure = INTRO_POINT_FAILURE_GENERIC;
diff --git a/src/feature/hs/hs_client.h b/src/feature/hs/hs_client.h
index a0f1c7758f..23effc06bd 100644
--- a/src/feature/hs/hs_client.h
+++ b/src/feature/hs/hs_client.h
@@ -10,6 +10,8 @@
 #define TOR_HS_CLIENT_H
 
 #include "lib/crypt_ops/crypto_ed25519.h"
+
+#include "feature/hs/hs_circuit.h"
 #include "feature/hs/hs_descriptor.h"
 #include "feature/hs/hs_ident.h"
 
@@ -112,7 +114,7 @@ int hs_client_send_introduce1(origin_circuit_t *intro_circ,
                               origin_circuit_t *rend_circ);
 
 void hs_client_circuit_has_opened(origin_circuit_t *circ);
-void hs_client_circuit_cleanup(const circuit_t *circ);
+void hs_client_circuit_cleanup_on_free(const circuit_t *circ);
 
 int hs_client_receive_rendezvous_acked(origin_circuit_t *circ,
                                        const uint8_t *payload,
diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c
index 632b00c852..14484f1ced 100644
--- a/src/feature/rend/rendclient.c
+++ b/src/feature/rend/rendclient.c
@@ -1254,7 +1254,7 @@ rend_parse_service_authorization(const or_options_t *options,
 /** The given circuit is being freed. Take appropriate action if it is of
  * interest to the client subsystem. */
 void
-rend_client_circuit_cleanup(const circuit_t *circ)
+rend_client_circuit_cleanup_on_free(const circuit_t *circ)
 {
   int reason, orig_reason;
   bool has_timed_out, ip_is_redundant;
diff --git a/src/feature/rend/rendclient.h b/src/feature/rend/rendclient.h
index da6f4646dc..63191737c4 100644
--- a/src/feature/rend/rendclient.h
+++ b/src/feature/rend/rendclient.h
@@ -12,6 +12,7 @@
 #ifndef TOR_RENDCLIENT_H
 #define TOR_RENDCLIENT_H
 
+#include "feature/hs/hs_circuit.h"
 #include "feature/rend/rendcache.h"
 
 void rend_client_purge_state(void);
@@ -47,7 +48,7 @@ rend_service_authorization_t *rend_client_lookup_service_authorization(
                                                 const char *onion_address);
 void rend_service_authorization_free_all(void);
 
-void rend_client_circuit_cleanup(const circuit_t *circ);
+void rend_client_circuit_cleanup_on_free(const circuit_t *circ);
 
 #endif /* !defined(TOR_RENDCLIENT_H) */