Merge branch 'tor-github/pr/1573'

changes/ticket32020

@@ -0,0 +1,6 @@
+  o Major bugfixes (onion service):
+    - Report back HS circuit failure back into the HS subsytem so we take
+      appropriate action with regards to the client introduction point failure
+      cache. This improves reachability of onion services, since now clients
+      notice failing introduction circuits properly. Fixes bug 32020; bugfix on
+      0.3.2.1-alpha;

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();
@@ -2343,43 +2343,6 @@ circuit_about_to_free(circuit_t *circ)
      orig_reason);
   }
 
-  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);
-    tor_assert(circ->state == CIRCUIT_STATE_OPEN);
-    tor_assert(ocirc->build_state->chosen_exit);
-    if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT &&
-        ocirc->rend_data) {
-      /* treat this like getting a nack from it */
-      log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s",
-          safe_str_client(rend_data_get_address(ocirc->rend_data)),
-          safe_str_client(build_state_get_exit_nickname(ocirc->build_state)),
-          timed_out ? "Recording timeout." : "Removing from descriptor.");
-      rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit,
-                                             ocirc->rend_data,
-                                             timed_out ?
-                                             INTRO_POINT_FAILURE_TIMEOUT :
-                                             INTRO_POINT_FAILURE_GENERIC);
-    }
-  } else if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING &&
-             reason != END_CIRC_REASON_TIMEOUT) {
-    origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ);
-    if (ocirc->build_state->chosen_exit && ocirc->rend_data) {
-      if (orig_reason != END_CIRC_REASON_IP_NOW_REDUNDANT &&
-          ocirc->rend_data) {
-        log_info(LD_REND, "Failed intro circ %s to %s "
-            "(building circuit to intro point). "
-            "Marking intro point as possibly unreachable.",
-            safe_str_client(rend_data_get_address(ocirc->rend_data)),
-            safe_str_client(build_state_get_exit_nickname(
-                                              ocirc->build_state)));
-        rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit,
-                                              ocirc->rend_data,
-                                              INTRO_POINT_FAILURE_UNREACHABLE);
-      }
-    }
-  }
-
   if (circ->n_chan) {
     circuit_clear_cell_queue(circ, circ->n_chan);
     /* Only send destroy if the channel isn't closing anyway */

src/core/or/circuituse.c

@@ -1965,23 +1965,61 @@ have_enough_path_info(int need_exit)
 int
 circuit_purpose_is_hidden_service(uint8_t purpose)
 {
-   if (purpose == CIRCUIT_PURPOSE_HS_VANGUARDS) {
-     return 1;
-   }
+  /* HS Vanguard purpose. */
+  if (circuit_purpose_is_hs_vanguards(purpose)) {
+    return 1;
+  }
 
-   /* Client-side purpose */
-   if (purpose >= CIRCUIT_PURPOSE_C_HS_MIN_ &&
-       purpose <= CIRCUIT_PURPOSE_C_HS_MAX_) {
-     return 1;
-   }
+  /* Client-side purpose */
+  if (circuit_purpose_is_hs_client(purpose)) {
+    return 1;
+  }
 
-   /* Service-side purpose */
-   if (purpose >= CIRCUIT_PURPOSE_S_HS_MIN_ &&
-       purpose <= CIRCUIT_PURPOSE_S_HS_MAX_) {
-     return 1;
-   }
+  /* Service-side purpose */
+  if (circuit_purpose_is_hs_service(purpose)) {
+    return 1;
+  }
 
-   return 0;
+  return 0;
+}
+
+/** Retrun true iff the given circuit is an HS client circuit. */
+bool
+circuit_purpose_is_hs_client(const uint8_t purpose)
+{
+  return (purpose >= CIRCUIT_PURPOSE_C_HS_MIN_ &&
+          purpose <= CIRCUIT_PURPOSE_C_HS_MAX_);
+}
+
+/** Retrun true iff the given circuit is an HS service circuit. */
+bool
+circuit_purpose_is_hs_service(const uint8_t purpose)
+{
+  return (purpose >= CIRCUIT_PURPOSE_S_HS_MIN_ &&
+          purpose <= CIRCUIT_PURPOSE_S_HS_MAX_);
+}
+
+/** Retrun true iff the given circuit is an HS Vanguards circuit. */
+bool
+circuit_purpose_is_hs_vanguards(const uint8_t purpose)
+{
+  return (purpose == CIRCUIT_PURPOSE_HS_VANGUARDS);
+}
+
+/** Retrun true iff the given circuit is an HS v2 circuit. */
+bool
+circuit_is_hs_v2(const circuit_t *circ)
+{
+  return (CIRCUIT_IS_ORIGIN(circ) &&
+          (CONST_TO_ORIGIN_CIRCUIT(circ)->rend_data != NULL));
+}
+
+/** Retrun true iff the given circuit is an HS v3 circuit. */
+bool
+circuit_is_hs_v3(const circuit_t *circ)
+{
+  return (CIRCUIT_IS_ORIGIN(circ) &&
+          (CONST_TO_ORIGIN_CIRCUIT(circ)->hs_ident != NULL));
 }
 
 /**
@@ -3086,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);
     }
   }
 

src/core/or/circuituse.h

@@ -64,6 +64,15 @@ int hostname_in_track_host_exits(const or_options_t *options,
 void mark_circuit_unusable_for_new_conns(origin_circuit_t *circ);
 
 int circuit_purpose_is_hidden_service(uint8_t);
+
+/* Series of helper functions for hidden services. */
+bool circuit_purpose_is_hs_client(const uint8_t purpose);
+bool circuit_purpose_is_hs_service(const uint8_t purpose);
+bool circuit_purpose_is_hs_vanguards(const uint8_t purpose);
+
+bool circuit_is_hs_v2(const circuit_t *circ);
+bool circuit_is_hs_v3(const circuit_t *circ);
+
 int circuit_should_use_vanguards(uint8_t);
 void circuit_sent_valid_data(origin_circuit_t *circ, uint16_t relay_body_len);
 void circuit_read_valid_data(origin_circuit_t *circ, uint16_t relay_body_len);

src/feature/hs/hs_circuit.c

@@ -20,11 +20,13 @@
 #include "feature/hs/hs_cell.h"
 #include "feature/hs/hs_circuit.h"
 #include "feature/hs/hs_circuitmap.h"
+#include "feature/hs/hs_client.h"
 #include "feature/hs/hs_ident.h"
 #include "feature/hs/hs_service.h"
 #include "feature/nodelist/describe.h"
 #include "feature/nodelist/nodelist.h"
 #include "feature/rend/rendservice.h"
+#include "feature/rend/rendclient.h"
 #include "feature/stats/rephist.h"
 #include "lib/crypt_ops/crypto_dh.h"
 #include "lib/crypt_ops/crypto_rand.h"
@@ -618,6 +620,22 @@ setup_introduce1_data(const hs_desc_intro_point_t *ip,
   return ret;
 }
 
+/** Helper: cleanup function for client circuit. This is for every HS version.
+ * It is called from hs_circ_cleanup_on_free() entry point. */
+static void
+cleanup_on_free_client_circ(circuit_t *circ)
+{
+  tor_assert(circ);
+
+  if (circuit_is_hs_v2(circ)) {
+    rend_client_circuit_cleanup_on_free(circ);
+  } else if (circuit_is_hs_v3(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. */
+}
+
 /* ========== */
 /* Public API */
 /* ========== */
@@ -1197,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);
 
-  /* If it's a service-side intro circ, notify the HS subsystem for the 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));
-  }
+  /* 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_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. */
 
-  /* 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);
   }

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,

src/feature/hs/hs_client.c

@@ -1522,6 +1522,56 @@ get_hs_client_auths_map(void)
 /* Public API */
 /* ========== */
 
+/** Called when a circuit was just cleaned up. This is done right before the
+ * circuit is freed. */
+void
+hs_client_circuit_cleanup_on_free(const circuit_t *circ)
+{
+  bool has_timed_out;
+  rend_intro_point_failure_t failure = INTRO_POINT_FAILURE_GENERIC;
+  const origin_circuit_t *orig_circ = NULL;
+
+  tor_assert(circ);
+  tor_assert(CIRCUIT_IS_ORIGIN(circ));
+
+  orig_circ = CONST_TO_ORIGIN_CIRCUIT(circ);
+  tor_assert(orig_circ->hs_ident);
+
+  has_timed_out =
+    (circ->marked_for_close_orig_reason == END_CIRC_REASON_TIMEOUT);
+  if (has_timed_out) {
+    failure = INTRO_POINT_FAILURE_TIMEOUT;
+  }
+
+  switch (circ->purpose) {
+  case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT:
+    log_info(LD_REND, "Failed v3 intro circ for service %s to intro point %s "
+                      "(awaiting ACK). Failure code: %d",
+        safe_str_client(ed25519_fmt(&orig_circ->hs_ident->identity_pk)),
+        safe_str_client(build_state_get_exit_nickname(orig_circ->build_state)),
+        failure);
+    hs_cache_client_intro_state_note(&orig_circ->hs_ident->identity_pk,
+                                     &orig_circ->hs_ident->intro_auth_pk,
+                                     failure);
+    break;
+  case CIRCUIT_PURPOSE_C_INTRODUCING:
+    if (has_timed_out || !orig_circ->build_state) {
+      break;
+    }
+    failure = INTRO_POINT_FAILURE_UNREACHABLE;
+    log_info(LD_REND, "Failed v3 intro circ for service %s to intro point %s "
+                      "(while building circuit). Marking as unreachable.",
+       safe_str_client(ed25519_fmt(&orig_circ->hs_ident->identity_pk)),
+       safe_str_client(build_state_get_exit_nickname(orig_circ->build_state)));
+    hs_cache_client_intro_state_note(&orig_circ->hs_ident->identity_pk,
+                                     &orig_circ->hs_ident->intro_auth_pk,
+                                     failure);
+    break;
+  default:
+    break;
+  }
+}
+
 /** A circuit just finished connecting to a hidden service that the stream
  *  <b>conn</b> has been waiting for. Let the HS subsystem know about this. */
 void

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,6 +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_on_free(const circuit_t *circ);
 
 int hs_client_receive_rendezvous_acked(origin_circuit_t *circ,
                                        const uint8_t *payload,

src/feature/hs/hs_service.c

@@ -2404,12 +2404,10 @@ static void
 cleanup_intro_points(hs_service_t *service, time_t now)
 {
   /* List of intro points to close. We can't mark the intro circuits for close
-   * in the modify loop because doing so calls
-   * hs_service_intro_circ_has_closed() which does a digest256map_get() on the
-   * intro points map (that we are iterating over). This can't be done in a
-   * single iteration after a MAP_DEL_CURRENT, the object will still be
-   * returned leading to a use-after-free. So, we close the circuits and free
-   * the intro points after the loop if any. */
+   * in the modify loop because doing so calls back into the HS subsystem and
+   * we need to keep that code path outside of the service/desc loop so those
+   * maps don't get modified during the close making us in a possible
+   * use-after-free situation. */
   smartlist_t *ips_to_free = smartlist_new();
 
   tor_assert(service);
@@ -3684,44 +3682,6 @@ hs_service_get_num_services,(void))
   return HT_SIZE(hs_service_map);
 }
 
-/** Called once an introduction circuit is closed. If the circuit doesn't have
- * a v3 identifier, it is ignored. */
-void
-hs_service_intro_circ_has_closed(origin_circuit_t *circ)
-{
-  hs_service_t *service = NULL;
-  hs_service_intro_point_t *ip = NULL;
-  hs_service_descriptor_t *desc = NULL;
-
-  tor_assert(circ);
-
-  if (circ->hs_ident == NULL) {
-    /* This is not a v3 circuit, ignore. */
-    goto end;
-  }
-
-  get_objects_from_ident(circ->hs_ident, &service, &ip, &desc);
-  if (service == NULL) {
-    /* This is possible if the circuits are closed and the service is
-     * immediately deleted. */
-    log_info(LD_REND, "Unable to find any hidden service associated "
-                      "identity key %s on intro circuit %u.",
-             ed25519_fmt(&circ->hs_ident->identity_pk),
-             TO_CIRCUIT(circ)->n_circ_id);
-    goto end;
-  }
-  if (ip == NULL) {
-    /* The introduction point object has already been removed probably by our
-     * cleanup process so ignore. */
-    goto end;
-  }
-  /* Can't have an intro point object without a descriptor. */
-  tor_assert(desc);
-
- end:
-  return;
-}
-
 /** Given conn, a rendezvous edge connection acting as an exit stream, look up
  * the hidden service for the circuit circ, and look up the port and address
  * based on the connection port. Assign the actual connection address.

src/feature/hs/hs_service.h

@@ -344,8 +344,6 @@ int hs_service_receive_introduce2(origin_circuit_t *circ,
                                   const uint8_t *payload,
                                   size_t payload_len);
 
-void hs_service_intro_circ_has_closed(origin_circuit_t *circ);
-
 char *hs_service_lookup_current_desc(const ed25519_public_key_t *pk);
 
 hs_service_add_ephemeral_status_t

src/feature/rend/rendclient.c

@@ -1250,3 +1250,66 @@ rend_parse_service_authorization(const or_options_t *options,
   }
   return res;
 }
+
+/** The given circuit is being freed. Take appropriate action if it is of
+ * interest to the client subsystem. */
+void
+rend_client_circuit_cleanup_on_free(const circuit_t *circ)
+{
+  int reason, orig_reason;
+  bool has_timed_out, ip_is_redundant;
+  const origin_circuit_t *ocirc = NULL;
+
+  tor_assert(circ);
+  tor_assert(CIRCUIT_IS_ORIGIN(circ));
+
+  reason = circ->marked_for_close_reason;
+  orig_reason = circ->marked_for_close_orig_reason;
+  ocirc = CONST_TO_ORIGIN_CIRCUIT(circ);
+  tor_assert(ocirc->rend_data);
+
+  has_timed_out = (reason == END_CIRC_REASON_TIMEOUT);
+  ip_is_redundant = (orig_reason == END_CIRC_REASON_IP_NOW_REDUNDANT);
+
+  switch (circ->purpose) {
+  case CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT:
+  {
+    if (ip_is_redundant) {
+      break;
+    }
+    tor_assert(circ->state == CIRCUIT_STATE_OPEN);
+    tor_assert(ocirc->build_state->chosen_exit);
+    /* Treat this like getting a nack from it */
+    log_info(LD_REND, "Failed intro circ %s to %s (awaiting ack). %s",
+        safe_str_client(rend_data_get_address(ocirc->rend_data)),
+        safe_str_client(build_state_get_exit_nickname(ocirc->build_state)),
+        has_timed_out ? "Recording timeout." : "Removing from descriptor.");
+    rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit,
+                                           ocirc->rend_data,
+                                           has_timed_out ?
+                                           INTRO_POINT_FAILURE_TIMEOUT :
+                                           INTRO_POINT_FAILURE_GENERIC);
+    break;
+  }
+  case CIRCUIT_PURPOSE_C_INTRODUCING:
+  {
+    /* Ignore if we were introducing and it timed out, we didn't pick an exit
+     * point yet (IP) or the reason indicate that it was a redundant IP. */
+    if (has_timed_out || !ocirc->build_state->chosen_exit || ip_is_redundant) {
+      break;
+    }
+    log_info(LD_REND, "Failed intro circ %s to %s "
+             "(building circuit to intro point). "
+             "Marking intro point as possibly unreachable.",
+             safe_str_client(rend_data_get_address(ocirc->rend_data)),
+             safe_str_client(build_state_get_exit_nickname(
+                                                  ocirc->build_state)));
+    rend_client_report_intro_point_failure(ocirc->build_state->chosen_exit,
+                                           ocirc->rend_data,
+                                           INTRO_POINT_FAILURE_UNREACHABLE);
+    break;
+  }
+  default:
+    break;
+  }
+}

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,5 +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_on_free(const circuit_t *circ);
+
 #endif /* !defined(TOR_RENDCLIENT_H) */
 

src/test/test_hs_client.c

@@ -1226,6 +1226,113 @@ test_socks_hs_errors(void *arg)
   UNMOCK(check_private_dir);
 }
 
+static void
+test_close_intro_circuit_failure(void *arg)
+{
+  char digest[DIGEST_LEN];
+  circuit_t *circ = NULL;
+  ed25519_keypair_t service_kp, intro_kp;
+  origin_circuit_t *ocirc = NULL;
+  tor_addr_t addr;
+  const hs_cache_intro_state_t *entry;
+
+  (void) arg;
+
+  hs_init();
+
+  /* Generate service keypair */
+  tt_int_op(0, OP_EQ, ed25519_keypair_generate(&service_kp, 0));
+  tt_int_op(0, OP_EQ, ed25519_keypair_generate(&intro_kp, 0));
+
+  /* Create and add to the global list a dummy client introduction circuit at
+   * the ACK WAIT state. */
+  circ = dummy_origin_circuit_new(0);
+  tt_assert(circ);
+  circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT;
+  ocirc = TO_ORIGIN_CIRCUIT(circ);
+  ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey);
+  ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+  /* Code path will log this exit so build it. */
+  ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest,
+                                                    NULL, NULL, NULL, &addr,
+                                                    4242);
+  ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey);
+
+  /* We'll make for close the circuit for a timeout failure. It should _NOT_
+   * end up in the failure cache just yet. We do that on free() only. */
+  circuit_mark_for_close(circ, END_CIRC_REASON_TIMEOUT);
+  tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey,
+                                              &intro_kp.pubkey));
+  /* Time to free. It should get removed. */
+  circuit_free(circ);
+  entry = hs_cache_client_intro_state_find(&service_kp.pubkey,
+                                           &intro_kp.pubkey);
+  tt_assert(entry);
+  tt_uint_op(entry->timed_out, OP_EQ, 1);
+  hs_cache_client_intro_state_purge();
+
+  /* Again, create and add to the global list a dummy client introduction
+   * circuit at the INTRODUCING state. */
+  circ = dummy_origin_circuit_new(0);
+  tt_assert(circ);
+  circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING;
+  ocirc = TO_ORIGIN_CIRCUIT(circ);
+  ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey);
+  ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+  /* Code path will log this exit so build it. */
+  ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest,
+                                                    NULL, NULL, NULL, &addr,
+                                                    4242);
+  ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey);
+
+  /* On free, we should get an unreachable failure. */
+  circuit_free(circ);
+  entry = hs_cache_client_intro_state_find(&service_kp.pubkey,
+                                           &intro_kp.pubkey);
+  tt_assert(entry);
+  tt_uint_op(entry->unreachable_count, OP_EQ, 1);
+  hs_cache_client_intro_state_purge();
+
+  /* Again, create and add to the global list a dummy client introduction
+   * circuit at the INTRODUCING state but we'll close it for timeout. It
+   * should not be noted as a timeout failure. */
+  circ = dummy_origin_circuit_new(0);
+  tt_assert(circ);
+  circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING;
+  ocirc = TO_ORIGIN_CIRCUIT(circ);
+  ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey);
+  ocirc->build_state = tor_malloc_zero(sizeof(cpath_build_state_t));
+  /* Code path will log this exit so build it. */
+  ocirc->build_state->chosen_exit = extend_info_new("TestNickname", digest,
+                                                    NULL, NULL, NULL, &addr,
+                                                    4242);
+  ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey);
+
+  circuit_mark_for_close(circ, END_CIRC_REASON_TIMEOUT);
+  circuit_free(circ);
+  tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey,
+                                              &intro_kp.pubkey));
+
+  /* Again, create and add to the global list a dummy client introduction
+   * circuit at the INTRODUCING state but without a chosen_exit. In theory, it
+   * can not happen but we'll make sure it doesn't end up in the failure cache
+   * anyway. */
+  circ = dummy_origin_circuit_new(0);
+  tt_assert(circ);
+  circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING;
+  ocirc = TO_ORIGIN_CIRCUIT(circ);
+  ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey);
+  ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, &intro_kp.pubkey);
+
+  circuit_free(circ);
+  tt_assert(!hs_cache_client_intro_state_find(&service_kp.pubkey,
+                                              &intro_kp.pubkey));
+
+ done:
+  circuit_free(circ);
+  hs_free_all();
+}
+
 struct testcase_t hs_client_tests[] = {
   { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy,
     TT_FORK, NULL, NULL },
@@ -1243,6 +1350,8 @@ struct testcase_t hs_client_tests[] = {
     TT_FORK, NULL, NULL },
   { "desc_has_arrived_cleanup", test_desc_has_arrived_cleanup,
     TT_FORK, NULL, NULL },
+  { "close_intro_circuit_failure", test_close_intro_circuit_failure,
+    TT_FORK, NULL, NULL },
   { "close_intro_circuits_new_desc", test_close_intro_circuits_new_desc,
     TT_FORK, NULL, NULL },
   { "close_intro_circuits_cache_clean", test_close_intro_circuits_cache_clean,