[tor-commits] [tor/master] Stop libevent from reading data from closed connections.

[nickm at torproject.org]

commit 841cff6e4fe1cdd370cd51019e69c6c564e8059c
Author: George Kadianakis <desnacked at riseup.net>
Date:   Mon Sep 30 18:29:00 2019 +0300

    Stop libevent from reading data from closed connections.
    
    Code adapted from Rob's proposed patch in #30344.
    
    Also add a comment in connection_mark_for_close_internal_() on why we should
    not be adding extra code there without a very good reason.
---
 changes/bug30344               |  4 ++++
 src/core/mainloop/connection.c | 18 ++++++++++++------
 src/core/mainloop/mainloop.c   | 10 ++++++++++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/changes/bug30344 b/changes/bug30344
new file mode 100644
index 000000000..37561bf94
--- /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/src/core/mainloop/connection.c b/src/core/mainloop/connection.c
index 2f03d919a..3595bba85 100644
--- a/src/core/mainloop/connection.c
+++ b/src/core/mainloop/connection.c
@@ -897,13 +897,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 6e2b300fb..4b3c3bf6a 100644
--- a/src/core/mainloop/mainloop.c
+++ b/src/core/mainloop/mainloop.c
@@ -879,6 +879,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