From d0682fe0f190ce55973d86ec92978aa6b3f61f1f Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 4 Dec 2018 14:00:08 -0500 Subject: [PATCH 1/4] conn: Add an helper to mark a connection as waiting for an HS descriptor The transition for a connection to either become or go back in AP_CONN_STATE_RENDDESC_WAIT state must make sure that the entry connection is _not_ in the waiting for circuit list. This commit implements the helper function connection_ap_mark_as_waiting_for_renddesc() that removes the entry connection from the pending list and then change its state. This code pattern is used in many places in the code where next commit will remove this code duplication to use this new helper function. Part of #28669 Signed-off-by: David Goulet --- src/core/or/connection_edge.c | 15 +++++++++++++++ src/core/or/connection_edge.h | 3 +++ 2 files changed, 18 insertions(+) diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index 58aefcf8f2..96149bbb1c 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -1354,6 +1354,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.", \ From 00b59d92817f34c60fe9bc1f8b72b6e81ae1c431 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 4 Dec 2018 14:09:15 -0500 Subject: [PATCH 2/4] conn: Use connection_ap_mark_as_waiting_for_renddesc() Use the helper function connection_ap_mark_as_waiting_for_renddesc() introduced in previous commit everywhere in the code where an AP connection state is transitionned to AP_CONN_STATE_RENDDESC_WAIT. Part of #28669 Signed-off-by: David Goulet --- src/core/or/circuituse.c | 3 +-- src/feature/hs/hs_client.c | 3 +-- src/feature/rend/rendclient.c | 6 ++---- 3 files changed, 4 insertions(+), 8 deletions(-) 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/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index dfad216abb..43e5b80e52 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); 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; From 43bd4d7509ceab2d82a85483f08132e90b1ab10d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 4 Dec 2018 14:18:23 -0500 Subject: [PATCH 3/4] hs-v3: Add the helper function mark_conn_as_waiting_for_circuit This helper function marks an entry connection as pending for a circuit and changes its state to AP_CONN_STATE_CIRCUIT_WAIT. The timestamps are set to now() so it can be considered as new. No behaviour change, this helper function will be used in next commit. Part of #28669 Signed-off-by: David Goulet --- src/feature/hs/hs_client.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 43e5b80e52..22aacdfe99 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -200,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 @@ -1700,17 +1720,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: From cec616a0c8ff060cb722e54342fd30aeab3ad285 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 4 Dec 2018 14:27:46 -0500 Subject: [PATCH 4/4] hs-v3: Don't BUG() if descriptor is found on SOCKS connection retry When retrying all SOCKS connection because new directory information just arrived, do not BUG() if a connection in state AP_CONN_STATE_RENDDESC_WAIT is found to have a usable descriptor. There is a rare case when this can happen as detailed in #28669 so the right thing to do is put that connection back in circuit wait state so the descriptor can be retried. Fixes #28669 Signed-off-by: David Goulet --- changes/ticket28669 | 6 ++++++ src/feature/hs/hs_client.c | 19 +++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 changes/ticket28669 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/feature/hs/hs_client.c b/src/feature/hs/hs_client.c index 22aacdfe99..5fded92fe3 100644 --- a/src/feature/hs/hs_client.c +++ b/src/feature/hs/hs_client.c @@ -296,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