resolved: Read as much as possible per stream EPOLLIN event

In commit 2aaf6bb6e99b0f2bd73e0c49bef9e11a2844bf1a, an issue was fixed where
systemd-resolved could get stuck for multiple seconds waiting for incoming data,
since GnuTLS/OpenSSL can buffer a TLS record, so data could be available, but
no EPOLLIN event would be generated.

To fix this, a somewhat elaborate logic consisting on asking the TLS library
whether it had buffered data, then "faking" an EPOLLIN event was implemented.

However, there is a much simpler solution: Always read as much data as available
(i.e. until we get an event like EAGAIN when trying to read) from the stream
when we get an EPOLLIN event, instead of at most a single packet per event.
This approach does not require asking the TLS library whether it has buffered
data, and the logic is exactly the same for both the TCP and TLS case.

test-resolved-stream is fixed to avoid a latent double free bug.

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 5c4a9ebb9990ee63823b035f8583fd8938d3f4b3..61e92bea831e8580334dcad482138f8bc22373d5 100644 (file)
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -279,7 +279,7 @@ static DnsPacket *dns_stream_take_read_packet(DnsStream *s) {
          * Even this makes a room to read in the stream, this does not call dns_stream_update(), hence
          * EPOLLIN flag is not set automatically. So, to read further packets from the stream,
          * dns_stream_update() must be called explicitly. Currently, this is only called from
-         * on_stream_io_impl(), and there dns_stream_update() is called. */
+         * on_stream_io(), and there dns_stream_update() is called. */
 
         if (!s->read_packet)
                 return NULL;
@@ -294,15 +294,13 @@ static DnsPacket *dns_stream_take_read_packet(DnsStream *s) {
         return TAKE_PTR(s->read_packet);
 }
 
-static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
+static int on_stream_io(sd_event_source *es, int fd, uint32_t revents, void *userdata) {
+        _cleanup_(dns_stream_unrefp) DnsStream *s = dns_stream_ref(userdata); /* Protect stream while we process it */
         bool progressed = false;
         int r;
 
         assert(s);
 
-        /* This returns 1 when possible remaining stream exists, 0 on completed
-           stream or recoverable error, and negative errno on failure. */
-
 #if ENABLE_DNS_OVER_TLS
         if (s->encrypted) {
                 r = dnstls_stream_on_io(s, revents);
@@ -354,9 +352,9 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                 }
         }
 
-        if ((revents & (EPOLLIN|EPOLLHUP|EPOLLRDHUP)) &&
-            (!s->read_packet ||
-             s->n_read < sizeof(s->read_size) + s->read_packet->size)) {
+        while ((revents & (EPOLLIN|EPOLLHUP|EPOLLRDHUP)) &&
+               (!s->read_packet ||
+                s->n_read < sizeof(s->read_size) + s->read_packet->size)) {
 
                 if (s->n_read < sizeof(s->read_size)) {
                         ssize_t ss;
@@ -365,6 +363,7 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                         if (ss < 0) {
                                 if (!ERRNO_IS_TRANSIENT(ss))
                                         return dns_stream_complete(s, -ss);
+                                break;
                         } else if (ss == 0)
                                 return dns_stream_complete(s, ECONNRESET);
                         else {
@@ -418,6 +417,7 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                                 if (ss < 0) {
                                         if (!ERRNO_IS_TRANSIENT(ss))
                                                 return dns_stream_complete(s, -ss);
+                                        break;
                                 } else if (ss == 0)
                                         return dns_stream_complete(s, ECONNRESET);
                                 else
@@ -438,6 +438,10 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                                         return dns_stream_complete(s, -r);
 
                                 s->packet_received = true;
+
+                                /* If we just disabled the read event, stop reading */
+                                if (!FLAGS_SET(s->requested_events, EPOLLIN))
+                                        break;
                         }
                 }
         }
@@ -455,41 +459,6 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                         log_warning_errno(errno, "Couldn't restart TCP connection timeout, ignoring: %m");
         }
 
-        return 1;
-}
-
-static int on_stream_io(sd_event_source *es, int fd, uint32_t revents, void *userdata) {
-        _cleanup_(dns_stream_unrefp) DnsStream *s = dns_stream_ref(userdata); /* Protect stream while we process it */
-        int r;
-
-        assert(s);
-
-        r = on_stream_io_impl(s, revents);
-        if (r <= 0)
-                return r;
-
-#if ENABLE_DNS_OVER_TLS
-        if (!s->encrypted)
-                return 0;
-
-        /* When using DNS-over-TLS, the underlying TLS library may read the entire TLS record
-           and buffer it internally. If this happens, we will not receive further EPOLLIN events,
-           and unless there's some unrelated activity on the socket, we will hang until time out.
-           To avoid this, if there's buffered TLS data, generate a "fake" EPOLLIN event.
-           This is hacky, but it makes this case transparent to the rest of the IO code. */
-        while (dnstls_stream_has_buffered_data(s)) {
-                uint32_t events;
-
-                /* Make sure the stream still wants to process more data... */
-                if (!FLAGS_SET(s->requested_events, EPOLLIN))
-                        break;
-
-                r = on_stream_io_impl(s, EPOLLIN);
-                if (r <= 0)
-                        return r;
-        }
-#endif
-
         return 0;
 }
 

diff --git a/src/resolve/resolved-dnstls-gnutls.c b/src/resolve/resolved-dnstls-gnutls.c
index 3d361708a10fe0682d4d458052d27a6dece151ea..8c8628ebbbde00aae4c10f13f35d53dbd0a560f3 100644 (file)
--- a/src/resolve/resolved-dnstls-gnutls.c
+++ b/src/resolve/resolved-dnstls-gnutls.c
@@ -223,14 +223,6 @@ ssize_t dnstls_stream_read(DnsStream *stream, void *buf, size_t count) {
         return ss;
 }
 
-bool dnstls_stream_has_buffered_data(DnsStream *stream) {
-        assert(stream);
-        assert(stream->encrypted);
-        assert(stream->dnstls_data.session);
-
-        return gnutls_record_check_pending(stream->dnstls_data.session) > 0;
-}
-
 void dnstls_server_free(DnsServer *server) {
         assert(server);
 

diff --git a/src/resolve/resolved-dnstls-openssl.c b/src/resolve/resolved-dnstls-openssl.c
index 3a030048625615118da45674317db2e7404acd61..4d3a88c8dafcedde4275c84ba528fc82c7b035c4 100644 (file)
--- a/src/resolve/resolved-dnstls-openssl.c
+++ b/src/resolve/resolved-dnstls-openssl.c
@@ -361,7 +361,15 @@ ssize_t dnstls_stream_read(DnsStream *stream, void *buf, size_t count) {
         if (r <= 0) {
                 error = SSL_get_error(stream->dnstls_data.ssl, r);
                 if (IN_SET(error, SSL_ERROR_WANT_READ, SSL_ERROR_WANT_WRITE)) {
-                        stream->dnstls_events = error == SSL_ERROR_WANT_READ ? EPOLLIN : EPOLLOUT;
+                        /* If we receive SSL_ERROR_WANT_READ here, there are two possible scenarios:
+                           * OpenSSL needs to renegotiate (so we want to get an EPOLLIN event), or
+                           * There is no more application data is available, so we can just return
+                           And apparently there's no nice way to distinguish between the two.
+                           To handle this, never set EPOLLIN and just continue as usual.
+                           If OpenSSL really wants to read due to renegotiation, it will tell us
+                           again on SSL_write (at which point we will request EPOLLIN force a read);
+                           or we will just eventually read data anyway while we wait for a packet */
+                        stream->dnstls_events = error == SSL_ERROR_WANT_READ ? 0 : EPOLLOUT;
                         ss = -EAGAIN;
                 } else if (error == SSL_ERROR_ZERO_RETURN) {
                         stream->dnstls_events = 0;
@@ -385,14 +393,6 @@ ssize_t dnstls_stream_read(DnsStream *stream, void *buf, size_t count) {
         return ss;
 }
 
-bool dnstls_stream_has_buffered_data(DnsStream *stream) {
-        assert(stream);
-        assert(stream->encrypted);
-        assert(stream->dnstls_data.ssl);
-
-        return SSL_has_pending(stream->dnstls_data.ssl) > 0;
-}
-
 void dnstls_server_free(DnsServer *server) {
         assert(server);
 

diff --git a/src/resolve/resolved-dnstls.h b/src/resolve/resolved-dnstls.h
index 70b27d8d77fcd5695bdf8d8b73ce9bb193415e2a..cda97e0b126b3027e148b98d937b8b0013ab5ee2 100644 (file)
--- a/src/resolve/resolved-dnstls.h
+++ b/src/resolve/resolved-dnstls.h
@@ -3,7 +3,6 @@
 
 #if ENABLE_DNS_OVER_TLS
 
-#include <stdbool.h>
 #include <stdint.h>
 #include <sys/uio.h>
 
@@ -30,7 +29,6 @@ int dnstls_stream_on_io(DnsStream *stream, uint32_t revents);
 int dnstls_stream_shutdown(DnsStream *stream, int error);
 ssize_t dnstls_stream_writev(DnsStream *stream, const struct iovec *iov, size_t iovcnt);
 ssize_t dnstls_stream_read(DnsStream *stream, void *buf, size_t count);
-bool dnstls_stream_has_buffered_data(DnsStream *stream);
 
 void dnstls_server_free(DnsServer *server);
 

diff --git a/src/resolve/test-resolved-stream.c b/src/resolve/test-resolved-stream.c
index f9428989f087ff17c2fafef6a619a2deb38224bf..158925e7e6b8007c0022abc5e5329b603f3afc3d 100644 (file)
--- a/src/resolve/test-resolved-stream.c
+++ b/src/resolve/test-resolved-stream.c
@@ -144,7 +144,7 @@ static void *tls_dns_server(void *p) {
 
         r = safe_fork_full("(test-resolved-stream-tls-openssl)", (int[]) { fd_server, fd_tls }, 2,
                 FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG|FORK_LOG|FORK_REOPEN_LOG, &openssl_pid);
-        assert(r >= 0);
+        assert_se(r >= 0);
         if (r == 0) {
                 /* Child */
                 assert_se(dup2(fd_tls, STDIN_FILENO) >= 0);
@@ -200,6 +200,10 @@ static int on_stream_packet(DnsStream *stream, DnsPacket *p) {
         return 0;
 }
 
+static int on_stream_complete_do_nothing(DnsStream *s, int error) {
+        return 0;
+}
+
 static void test_dns_stream(bool tls) {
         Manager manager = {};
          _cleanup_(dns_stream_unrefp) DnsStream *stream = NULL;
@@ -251,9 +255,10 @@ static void test_dns_stream(bool tls) {
         /* systemd-resolved uses (and requires) the socket to be in nonblocking mode */
         assert_se(fcntl(clientfd, F_SETFL, O_NONBLOCK) >= 0);
 
-        /* Initialize DNS stream */
+        /* Initialize DNS stream (disabling the default self-destruction
+           behaviour when no complete callback is set) */
         assert_se(dns_stream_new(&manager, &stream, DNS_STREAM_LOOKUP, DNS_PROTOCOL_DNS,
-                                 TAKE_FD(clientfd), NULL, on_stream_packet, NULL,
+                                 TAKE_FD(clientfd), NULL, on_stream_packet, on_stream_complete_do_nothing,
                                  DNS_STREAM_DEFAULT_TIMEOUT_USEC) >= 0);
 #if ENABLE_DNS_OVER_TLS
         if (tls) {