hs-v3: Remove the circuit_established intro flag

Only use the HS circuit map to know if an introduction circuit is established
or not. No need for a flag to keep state of something we already have in the
circuit map. Furthermore, the circuit map gets cleaned up properly so it will
always have the "latest truth".

This commit also removes a unit test that was testing specifically that flag
but now we rely solely on the HS circuit map which is also tested few lines
below the removed test.

Fixes #32094

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

changes/ticket32094

@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden service v3):
+    - Do not rely on a "circuit established" flag for intro circuit but instead
+      always query the HS circuit map. This is to avoid sync issue with that
+      flag and the map. Fixes bug 32094; bugfix on 0.3.2.1-alpha.

src/feature/hs/hs_circuit.c

@@ -637,6 +637,27 @@ hs_circ_service_get_intro_circ(const hs_service_intro_point_t *ip)
   }
 }
 
+/* Return an introduction point established circuit matching the given intro
+ * point object. The circuit purpose has to be CIRCUIT_PURPOSE_S_INTRO. NULL
+ * is returned is no such circuit can be found. */
+origin_circuit_t *
+hs_circ_service_get_established_intro_circ(const hs_service_intro_point_t *ip)
+{
+  origin_circuit_t *circ;
+
+  tor_assert(ip);
+
+  if (ip->base.is_only_legacy) {
+    circ = hs_circuitmap_get_intro_circ_v2_service_side(ip->legacy_key_digest);
+  } else {
+    circ = hs_circuitmap_get_intro_circ_v3_service_side(
+                                        &ip->auth_key_kp.pubkey);
+  }
+
+  /* Only return circuit if it is established. */
+  return (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_INTRO) ? circ : NULL;
+}
+
 /* Called when we fail building a rendezvous circuit at some point other than
  * the last hop: launches a new circuit to the same rendezvous point. This
  * supports legacy service.

src/feature/hs/hs_circuit.h

@@ -35,6 +35,8 @@ void hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ);
 
 origin_circuit_t *hs_circ_service_get_intro_circ(
                                       const hs_service_intro_point_t *ip);
+origin_circuit_t *hs_circ_service_get_established_intro_circ(
+                                      const hs_service_intro_point_t *ip);
 
 /* Cell API. */
 int hs_circ_handle_intro_established(const hs_service_t *service,

src/feature/hs/hs_service.c

@@ -709,7 +709,7 @@ count_desc_circuit_established(const hs_service_descriptor_t *desc)
 
   DIGEST256MAP_FOREACH(desc->intro_points.map, key,
                        const hs_service_intro_point_t *, ip) {
-    count += ip->circuit_established;
+    count += !!hs_circ_service_get_established_intro_circ(ip);
   } DIGEST256MAP_FOREACH_END;
 
   return count;
@@ -1660,7 +1660,7 @@ build_desc_intro_points(const hs_service_t *service,
 
   DIGEST256MAP_FOREACH(desc->intro_points.map, key,
                        const hs_service_intro_point_t *, ip) {
-    if (!ip->circuit_established) {
+    if (!hs_circ_service_get_established_intro_circ(ip)) {
       /* Ignore un-established intro points. They can linger in that list
        * because their circuit has not opened and they haven't been removed
        * yet even though we have enough intro circuits.
@@ -2370,10 +2370,6 @@ should_remove_intro_point(hs_service_intro_point_t *ip, time_t now)
    * remove it because it might simply be valid and opened at the previous
    * scheduled event for the last retry. */
 
-  /* Did we established already? */
-  if (ip->circuit_established) {
-    goto end;
-  }
   /* Do we simply have an existing circuit regardless of its state? */
   if (hs_circ_service_get_intro_circ(ip)) {
     goto end;
@@ -3326,11 +3322,6 @@ service_handle_intro_established(origin_circuit_t *circ,
     goto err;
   }
 
-  /* Flag that we have an established circuit for this intro point. This value
-   * is what indicates the upload scheduled event if we are ready to build the
-   * intro point into the descriptor and upload. */
-  ip->circuit_established = 1;
-
   log_info(LD_REND, "Successfully received an INTRO_ESTABLISHED cell "
                     "on circuit %u for service %s",
            TO_CIRCUIT(circ)->n_circ_id,
@@ -3725,10 +3716,6 @@ hs_service_intro_circ_has_closed(origin_circuit_t *circ)
   /* Can't have an intro point object without a descriptor. */
   tor_assert(desc);
 
-  /* Circuit disappeared so make sure the intro point is updated. By
-   * keeping the object in the descriptor, we'll be able to retry. */
-  ip->circuit_established = 0;
-
  end:
   return;
 }

src/feature/hs/hs_service.h

@@ -69,9 +69,6 @@ typedef struct hs_service_intro_point_t {
    * consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give up on it. */
   uint32_t circuit_retries;
 
-  /** Set if this intro point has an established circuit. */
-  unsigned int circuit_established : 1;
-
   /** Replay cache recording the encrypted part of an INTRODUCE2 cell that the
    * circuit associated with this intro point has received. This is used to
    * prevent replay attacks. */

src/test/test_hs_service.c

@@ -1013,7 +1013,6 @@ test_intro_established(void *arg)
   /* Send an empty payload. INTRO_ESTABLISHED cells are basically zeroes. */
   ret = hs_service_receive_intro_established(circ, payload, sizeof(payload));
   tt_int_op(ret, OP_EQ, 0);
-  tt_u64_op(ip->circuit_established, OP_EQ, 1);
   tt_int_op(TO_CIRCUIT(circ)->purpose, OP_EQ, CIRCUIT_PURPOSE_S_INTRO);
 
  done:
@@ -1296,18 +1295,11 @@ test_service_event(void *arg)
      * descriptor map so we can retry it. */
     ip = helper_create_service_ip();
     service_intro_point_add(service->desc_current->intro_points.map, ip);
-    ip->circuit_established = 1;  /* We'll test that, it MUST be 0 after. */
-    run_housekeeping_event(now);
-    tt_int_op(digest256map_size(service->desc_current->intro_points.map),
-              OP_EQ, 1);
-    /* No removal if we have an established circuit after retries. */
-    ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1;
     run_housekeeping_event(now);
     tt_int_op(digest256map_size(service->desc_current->intro_points.map),
               OP_EQ, 1);
     /* Remove the IP object at once for the next test. */
     ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1;
-    ip->circuit_established = 0;
     run_housekeeping_event(now);
     tt_int_op(digest256map_size(service->desc_current->intro_points.map),
               OP_EQ, 0);