Merge branch 'maint-0.4.1' into release-0.4.1

14 files changed, 83 insertions, 15 deletions

diff --git a/changes/bug30344 b/changes/bug30344
new file mode 100644
index 0000000000..37561bf944
--- /dev/null
+++ b/changes/bug30344
@@ -0,0 +1,4 @@
+  o Minor bugfixes (connection):
+    - Avoid reading data from closed connections, which can cause needless
+      loops in libevent and infinite loops in Shadow. Fixes bug 30344; bugfix
+      on 0.1.1.1-alpha.
diff --git a/changes/bug31939 b/changes/bug31939
new file mode 100644
index 0000000000..a36ea495d6
--- /dev/null
+++ b/changes/bug31939
@@ -0,0 +1,3 @@
+  o Minor bugfixes (tls, logging):
+    - Log TLS read buffer length bugs once, rather than filling the logs
+      with similar warnings. Fixes bug 31939; bugfix on 0.3.0.4-rc.
diff --git a/changes/bug32108 b/changes/bug32108
new file mode 100644
index 0000000000..2806fa3e5d
--- /dev/null
+++ b/changes/bug32108
@@ -0,0 +1,8 @@
+  o Major bugfixes (relay):
+    - Relays now respect their AccountingMax bandwidth again. When relays
+      entered "soft" hibernation (which typically starts when we've hit
+      90% of our AccountingMax), we had stopped checking whether we should
+      enter hard hibernation. Soft hibernation refuses new connections and
+      new circuits, but the existing circuits can continue, meaning that
+      relays could have exceeded their configured AccountingMax. Fixes
+      bug 32108; bugfix on 0.4.0.1-alpha.
diff --git a/changes/ticket28970 b/changes/ticket28970
new file mode 100644
index 0000000000..138c575fcc
--- /dev/null
+++ b/changes/ticket28970
@@ -0,0 +1,6 @@
+  o Minor bugfixes (clietn, hidden service v3):
+    - Fix a BUG() assertion that occurs within a very small race window between
+      a client intro circuit opens and its descriptor that gets cleaned up from
+      the cache. The circuit is now closed which will trigger a re-fetch of the
+      descriptor and continue the HS connection. Fixes bug 28970; bugfix on
+      0.3.2.1-alpha.
diff --git a/changes/ticket31091 b/changes/ticket31091
new file mode 100644
index 0000000000..3cb9a2c37b
--- /dev/null
+++ b/changes/ticket31091
@@ -0,0 +1,3 @@
+  o Minor bugfixes (pluggable transports):
+    - Remove overly strict assertions that triggers when a pluggable transport
+      is spawned in an unsuccessful manner. Fixes bug 31091; bugfix on 0.4.0.1-alpha.
diff --git a/changes/ticket31548 b/changes/ticket31548
new file mode 100644
index 0000000000..fef0b5d01f
--- /dev/null
+++ b/changes/ticket31548
@@ -0,0 +1,7 @@
+  o Major bugfixes (hidden service v3):
+    - Make onion service always use the exact amount of configured intro points
+      (or less due to node exlusion). Before, a service could sometimes pick
+      more intro points than configured with the
+      HiddenServiceNumIntroductionPoints option. Fixes bug 31548; bugfix on
+      0.3.2.1-alpha.
+
diff --git a/changes/ticket32058 b/changes/ticket32058
new file mode 100644
index 0000000000..b40bcda416
--- /dev/null
+++ b/changes/ticket32058
@@ -0,0 +1,5 @@
+  o Minor bugfixes (mainloop, periodic events):
+    - Periodic events enabled flag was not unset properly when shutting down tor
+      cleanly. This had the side effect to not re-enable periodic events when
+      tor_api.h is used to relaunch tor after a shutdown. Fixes bug 32058;
+      bugfix on 0.3.3.1-alpha.
diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c
index 127f08683f..372494a577 100644
--- a/src/core/mainloop/connection.c
+++ b/src/core/mainloop/connection.c
@@ -900,13 +900,19 @@ connection_mark_for_close_(connection_t *conn, int line, const char *file)
 }
 
 /** Mark <b>conn</b> to be closed next time we loop through
- * conn_close_if_marked() in main.c; the _internal version bypasses the
- * CONN_TYPE_OR checks; this should be called when you either are sure that
- * if this is an or_connection_t the controlling channel has been notified
- * (e.g. with connection_or_notify_error()), or you actually are the
+ * conn_close_if_marked() in main.c.
+ *
+ * This _internal version bypasses the CONN_TYPE_OR checks; this should be
+ * called when you either are sure that if this is an or_connection_t the
+ * controlling channel has been notified (e.g. with
+ * connection_or_notify_error()), or you actually are the
  * connection_or_close_for_error() or connection_or_close_normally() function.
- * For all other cases, use connection_mark_and_flush() instead, which
- * checks for or_connection_t properly, instead.  See below.
+ * For all other cases, use connection_mark_and_flush() which checks for
+ * or_connection_t properly, instead.  See below.
+ *
+ * We want to keep this function simple and quick, since it can be called from
+ * quite deep in the call chain, and hence it should avoid having side-effects
+ * that interfere with its callers view of the connection.
  */
 MOCK_IMPL(void,
 connection_mark_for_close_internal_, (connection_t *conn,
diff --git a/src/core/mainloop/mainloop.c b/src/core/mainloop/mainloop.c
index c051b11566..6f29a6981a 100644
--- a/src/core/mainloop/mainloop.c
+++ b/src/core/mainloop/mainloop.c
@@ -874,6 +874,16 @@ conn_read_callback(evutil_socket_t fd, short event, void *_conn)
 
   /* assert_connection_ok(conn, time(NULL)); */
 
+  /* Handle marked for close connections early */
+  if (conn->marked_for_close && connection_is_reading(conn)) {
+    /* Libevent says we can read, but we are marked for close so we will never
+     * try to read again. We will try to close the connection below inside of
+     * close_closeable_connections(), but let's make sure not to cause Libevent
+     * to spin on conn_read_callback() while we wait for the socket to let us
+     * flush to it.*/
+    connection_stop_reading(conn);
+  }
+
   if (connection_handle_read(conn) < 0) {
     if (!conn->marked_for_close) {
 #ifndef _WIN32
@@ -1378,7 +1388,7 @@ STATIC periodic_event_item_t mainloop_periodic_events[] = {
   /* This is a legacy catch-all callback that runs once per second if
    * we are online and active. */
   CALLBACK(second_elapsed, NET_PARTICIPANT,
-           FL(NEED_NET)|FL(RUN_ON_DISABLE)),
+           FL(RUN_ON_DISABLE)),
 
   /* XXXX Do we have a reason to do this on a callback? Does it do any good at
    * all?  For now, if we're dormant, we can let our listeners decay. */
diff --git a/src/core/mainloop/periodic.c b/src/core/mainloop/periodic.c
index dbc4553a73..5c2f6f2b36 100644
--- a/src/core/mainloop/periodic.c
+++ b/src/core/mainloop/periodic.c
@@ -153,6 +153,11 @@ periodic_event_disconnect(periodic_event_item_t *event)
 {
   if (!event)
     return;
+
+  /* First disable the event so we first cancel the event and set its enabled
+   * flag properly. */
+  periodic_event_disable(event);
+
   mainloop_event_free(event->ev);
   event->last_action_time = 0;
 }
diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c
index 97bfc8ae30..3f731ac7d4 100644
--- a/src/feature/client/transports.c
+++ b/src/feature/client/transports.c
@@ -1826,15 +1826,13 @@ managed_proxy_stdout_callback(process_t *process,
 
   managed_proxy_t *mp = process_get_data(process);
 
-  if (BUG(mp == NULL))
+  if (mp == NULL)
     return;
 
   handle_proxy_line(line, mp);
 
-  if (proxy_configuration_finished(mp)) {
+  if (proxy_configuration_finished(mp))
     handle_finished_proxy(mp);
-    tor_assert(mp->conf_state == PT_PROTO_COMPLETED);
-  }
 }
 
 /** Callback function that is called when our PT process have data on its
diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index f8d47f0114..492e77faff 100644
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -682,8 +682,12 @@ setup_intro_circ_auth_key(origin_circuit_t *circ)
   tor_assert(circ);
 
   desc = hs_cache_lookup_as_client(&circ->hs_ident->identity_pk);
-  if (BUG(desc == NULL)) {
-    /* Opening intro circuit without the descriptor is no good... */
+  if (desc == NULL) {
+    /* There is a very small race window between the opening of this circuit
+     * and the client descriptor cache that gets purged (NEWNYM) or the
+     * cleaned up because it expired. Mark the circuit for close so a new
+     * descriptor fetch can occur. */
+    circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL);
     goto end;
   }
 
diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c
index 2835912742..d1ca33b12e 100644
--- a/src/feature/hs/hs_service.c
+++ b/src/feature/hs/hs_service.c
@@ -1652,6 +1652,15 @@ 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) {
+      /* 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.
+       *
+       * Due to #31561, it can stay in that list until rotation so this check
+       * prevents to publish an intro point without a circuit. */
+      continue;
+    }
     hs_desc_intro_point_t *desc_ip = hs_desc_intro_point_new();
     if (setup_desc_intro_point(&desc->signing_kp, ip, now, desc_ip) < 0) {
       hs_desc_intro_point_free(desc_ip);
diff --git a/src/lib/tls/buffers_tls.c b/src/lib/tls/buffers_tls.c
index 3c18cc7e43..bf03b61459 100644
--- a/src/lib/tls/buffers_tls.c
+++ b/src/lib/tls/buffers_tls.c
@@ -68,9 +68,9 @@ buf_read_from_tls(buf_t *buf, tor_tls_t *tls, size_t at_most)
 
   check_no_tls_errors();
 
-  if (BUG(buf->datalen >= INT_MAX))
+  IF_BUG_ONCE(buf->datalen >= INT_MAX)
     return -1;
-  if (BUG(buf->datalen >= INT_MAX - at_most))
+  IF_BUG_ONCE(buf->datalen >= INT_MAX - at_most)
     return -1;
 
   while (at_most > total_read) {