diff --git a/changes/ticket28669 b/changes/ticket28669
new file mode 100644
index 0000000000..32c6114ffc
--- /dev/null
+++ b/changes/ticket28669
@@ -0,0 +1,6 @@
+ o Minor bugfix (hidden service v3, client):
+ - Avoid a BUG() stacktrace in case a SOCKS connection is found waiting for
+ the descriptor while we do have it in the cache. There is a rare case
+ when this can happen. Now, tor will recover and retry the descriptor.
+ Fixes bug 28669; bugfix on 0.3.2.4-alpha.
+
diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c
index 088358a4d6..7ea37fb3b5 100644
--- a/src/core/or/circuituse.c
+++ b/src/core/or/circuituse.c
@@ -2377,8 +2377,7 @@ circuit_get_open_circ_or_launch(entry_connection_t *conn,
} else {
hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk);
}
- connection_ap_mark_as_non_pending_circuit(conn);
- ENTRY_TO_CONN(conn)->state = AP_CONN_STATE_RENDDESC_WAIT;
+ connection_ap_mark_as_waiting_for_renddesc(conn);
return 0;
}
log_info(LD_REND,"Chose %s as intro point for '%s'.",
diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c
index 2ec83c1204..57cb1194cf 100644
--- a/src/core/or/connection_edge.c
+++ b/src/core/or/connection_edge.c
@@ -1357,6 +1357,21 @@ connection_ap_mark_as_non_pending_circuit(entry_connection_t *entry_conn)
smartlist_remove(pending_entry_connections, entry_conn);
}
+/** Mark entry_conn as waiting for a rendezvous descriptor. This
+ * function will remove the entry connection from the waiting for a circuit
+ * list (pending_entry_connections).
+ *
+ * This pattern is used across the code base because a connection in state
+ * AP_CONN_STATE_RENDDESC_WAIT must not be in the pending list. */
+void
+connection_ap_mark_as_waiting_for_renddesc(entry_connection_t *entry_conn)
+{
+ tor_assert(entry_conn);
+
+ connection_ap_mark_as_non_pending_circuit(entry_conn);
+ ENTRY_TO_CONN(entry_conn)->state = AP_CONN_STATE_RENDDESC_WAIT;
+}
+
/* DOCDOC */
void
connection_ap_warn_and_unmark_if_pending_circ(entry_connection_t *entry_conn,
diff --git a/src/core/or/connection_edge.h b/src/core/or/connection_edge.h
index 55b90d3eae..b8a7365a05 100644
--- a/src/core/or/connection_edge.h
+++ b/src/core/or/connection_edge.h
@@ -126,6 +126,9 @@ void connection_ap_mark_as_pending_circuit_(entry_connection_t *entry_conn,
#define connection_ap_mark_as_pending_circuit(c) \
connection_ap_mark_as_pending_circuit_((c), __FILE__, __LINE__)
void connection_ap_mark_as_non_pending_circuit(entry_connection_t *entry_conn);
+void connection_ap_mark_as_waiting_for_renddesc(
+ entry_connection_t *entry_conn);
+
#define CONNECTION_AP_EXPECT_NONPENDING(c) do { \
if (ENTRY_TO_CONN(c)->state == AP_CONN_STATE_CIRCUIT_WAIT) { \
log_warn(LD_BUG, "At %s:%d: %p was unexpectedly in circuit_wait.", \
diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index dfad216abb..5fded92fe3 100644
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -142,8 +142,7 @@ flag_all_conn_wait_desc(const ed25519_public_key_t *service_identity_pk)
if (edge_conn->hs_ident &&
ed25519_pubkey_eq(&edge_conn->hs_ident->identity_pk,
service_identity_pk)) {
- connection_ap_mark_as_non_pending_circuit(TO_ENTRY_CONN(conn));
- conn->state = AP_CONN_STATE_RENDDESC_WAIT;
+ connection_ap_mark_as_waiting_for_renddesc(TO_ENTRY_CONN(conn));
}
} SMARTLIST_FOREACH_END(conn);
@@ -201,6 +200,26 @@ directory_request_is_pending(const ed25519_public_key_t *identity_pk)
return ret;
}
+/* Helper function that changes the state of an entry connection to waiting
+ * for a circuit. For this to work properly, the connection timestamps are set
+ * to now and the connection is then marked as pending for a circuit. */
+static void
+mark_conn_as_waiting_for_circuit(connection_t *conn, time_t now)
+{
+ tor_assert(conn);
+
+ /* Because the connection can now proceed to opening circuit and ultimately
+ * connect to the service, reset those timestamp so the connection is
+ * considered "fresh" and can continue without being closed too early. */
+ conn->timestamp_created = now;
+ conn->timestamp_last_read_allowed = now;
+ conn->timestamp_last_write_allowed = now;
+ /* Change connection's state into waiting for a circuit. */
+ conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
+
+ connection_ap_mark_as_pending_circuit(TO_ENTRY_CONN(conn));
+}
+
/* We failed to fetch a descriptor for the service with identity_pk
* because of status. Find all pending SOCKS connections for this
* service that are waiting on the descriptor and close them with
@@ -277,12 +296,19 @@ retry_all_socks_conn_waiting_for_desc(void)
/* Order a refetch in case it works this time. */
status = hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk);
- if (BUG(status == HS_CLIENT_FETCH_HAVE_DESC)) {
- /* This case is unique because it can NOT happen in theory. Once we get
- * a new descriptor, the HS client subsystem is notified immediately and
- * the connections waiting for it are handled which means the state will
- * change from renddesc wait state. Log this and continue to next
- * connection. */
+ if (status == HS_CLIENT_FETCH_HAVE_DESC) {
+ /* This is a rare case where a SOCKS connection is in state waiting for
+ * a descriptor but we do have it in the cache.
+ *
+ * This can happen is tor comes back from suspend where it previously
+ * had the descriptor but the intro points were not usuable. Once it
+ * came back to life, the intro point failure cache was cleaned up and
+ * thus the descriptor became usable again leaving us in this code path.
+ *
+ * We'll mark the connection as waiting for a circuit so the descriptor
+ * can be retried. This is safe because a connection in state waiting
+ * for a descriptor can not be in the entry connection pending list. */
+ mark_conn_as_waiting_for_circuit(base_conn, approx_time());
continue;
}
/* In the case of an error, either all SOCKS connections have been
@@ -1701,17 +1727,9 @@ hs_client_desc_has_arrived(const hs_ident_dir_conn_t *ident)
log_info(LD_REND, "Descriptor has arrived. Launching circuits.");
- /* Because the connection can now proceed to opening circuit and
- * ultimately connect to the service, reset those timestamp so the
- * connection is considered "fresh" and can continue without being closed
- * too early. */
- base_conn->timestamp_created = now;
- base_conn->timestamp_last_read_allowed = now;
- base_conn->timestamp_last_write_allowed = now;
- /* Change connection's state into waiting for a circuit. */
- base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
-
- connection_ap_mark_as_pending_circuit(entry_conn);
+ /* Mark connection as waiting for a circuit since we do have a usable
+ * descriptor now. */
+ mark_conn_as_waiting_for_circuit(base_conn, now);
} SMARTLIST_FOREACH_END(base_conn);
end:
diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c
index 10b67ceda9..6ecb3eb3c6 100644
--- a/src/feature/rend/rendclient.c
+++ b/src/feature/rend/rendclient.c
@@ -150,8 +150,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
while ((conn = connection_get_by_type_state_rendquery(CONN_TYPE_AP,
AP_CONN_STATE_CIRCUIT_WAIT, onion_address))) {
- connection_ap_mark_as_non_pending_circuit(TO_ENTRY_CONN(conn));
- conn->state = AP_CONN_STATE_RENDDESC_WAIT;
+ connection_ap_mark_as_waiting_for_renddesc(TO_ENTRY_CONN(conn));
}
}
@@ -864,8 +863,7 @@ rend_client_report_intro_point_failure(extend_info_t *failed_intro,
while ((conn = connection_get_by_type_state_rendquery(CONN_TYPE_AP,
AP_CONN_STATE_CIRCUIT_WAIT,
onion_address))) {
- connection_ap_mark_as_non_pending_circuit(TO_ENTRY_CONN(conn));
- conn->state = AP_CONN_STATE_RENDDESC_WAIT;
+ connection_ap_mark_as_waiting_for_renddesc(TO_ENTRY_CONN(conn));
}
return 0;