diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c index 5c4a9ebb99..61e92bea83 100644 --- 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 3d361708a1..8c8628ebbb 100644 --- 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 3a03004862..4d3a88c8da 100644 --- 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 70b27d8d77..cda97e0b12 100644 --- a/src/resolve/resolved-dnstls.h +++ b/src/resolve/resolved-dnstls.h @@ -3,7 +3,6 @@ #if ENABLE_DNS_OVER_TLS -#include #include #include @@ -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 f9428989f0..158925e7e6 100644 --- 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) {