Merge remote-tracking branch 'tor-github/pr/563' into maint-0.3.5

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.
+    

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'.",

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 <b>entry_conn</b> 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,

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.", \

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 <b>identity_pk</b>
  * because of <b>status</b>. 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:

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;