Merge branch 'tor-github/pr/1456'

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

@@ -711,7 +711,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;
@@ -1662,7 +1662,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.
@@ -2372,10 +2372,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;
@@ -3328,11 +3324,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,
@@ -3727,10 +3718,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);