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>

1 files changed, 74 insertions, 20 deletions

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);
   }