Merge pull request #11055 from poettering/resolved-close-fix

a number of resolved fixes

7 files changed, 163 insertions, 89 deletions

diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c
index e05ada29a8..3e69741b88 100644
--- a/src/resolve/resolved-dns-server.c
+++ b/src/resolve/resolved-dns-server.c
@@ -43,16 +43,18 @@ int dns_server_new(
                         return -E2BIG;
         }
 
-        s = new0(DnsServer, 1);
+        s = new(DnsServer, 1);
         if (!s)
                 return -ENOMEM;
 
-        s->n_ref = 1;
-        s->manager = m;
-        s->type = type;
-        s->family = family;
-        s->address = *in_addr;
-        s->ifindex = ifindex;
+        *s = (DnsServer) {
+                .n_ref = 1,
+                .manager = m,
+                .type = type,
+                .family = family,
+                .address = *in_addr,
+                .ifindex = ifindex,
+        };
 
         dns_server_reset_features(s);
 
@@ -101,7 +103,7 @@ int dns_server_new(
 static DnsServer* dns_server_free(DnsServer *s)  {
         assert(s);
 
-        dns_stream_unref(s->stream);
+        dns_server_unref_stream(s);
 
 #if ENABLE_DNS_OVER_TLS
         dnstls_server_free(s);
@@ -156,6 +158,9 @@ void dns_server_unlink(DnsServer *s) {
         if (s->manager->current_dns_server == s)
                 manager_set_dns_server(s->manager, NULL);
 
+        /* No need to keep a default stream around anymore */
+        dns_server_unref_stream(s);
+
         dns_server_unref(s);
 }
 
@@ -824,6 +829,9 @@ void dns_server_reset_features(DnsServer *s) {
         s->warned_downgrade = false;
 
         dns_server_reset_counters(s);
+
+        /* Let's close the default stream, so that we reprobe with the new features */
+        dns_server_unref_stream(s);
 }
 
 void dns_server_reset_features_all(DnsServer *s) {
@@ -884,6 +892,20 @@ void dns_server_dump(DnsServer *s, FILE *f) {
                 yes_no(s->packet_rrsig_missing));
 }
 
+void dns_server_unref_stream(DnsServer *s) {
+        DnsStream *ref;
+
+        assert(s);
+
+        /* Detaches the default stream of this server. Some special care needs to be taken here, as that stream and
+         * this server reference each other. First, take the stream out of the server. It's destructor will check if it
+         * is registered with us, hence let's invalidate this separatly, so that it is already unregistered. */
+        ref = TAKE_PTR(s->stream);
+
+        /* And then, unref it */
+        dns_stream_unref(ref);
+}
+
 static const char* const dns_server_type_table[_DNS_SERVER_TYPE_MAX] = {
         [DNS_SERVER_SYSTEM] = "system",
         [DNS_SERVER_FALLBACK] = "fallback",
diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h
index fda1251049..6e73f32df4 100644
--- a/src/resolve/resolved-dns-server.h
+++ b/src/resolve/resolved-dns-server.h
@@ -54,6 +54,8 @@ struct DnsServer {
         int ifindex; /* for IPv6 link-local DNS servers */
 
         char *server_string;
+
+        /* The long-lived stream towards this server. */
         DnsStream *stream;
 
 #if ENABLE_DNS_OVER_TLS
@@ -149,3 +151,5 @@ void dns_server_reset_features(DnsServer *s);
 void dns_server_reset_features_all(DnsServer *s);
 
 void dns_server_dump(DnsServer *s, FILE *f);
+
+void dns_server_unref_stream(DnsServer *s);
diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 26d4663d74..aee339a4c8 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -17,6 +17,9 @@ static void dns_stream_stop(DnsStream *s) {
         s->io_event_source = sd_event_source_unref(s->io_event_source);
         s->timeout_event_source = sd_event_source_unref(s->timeout_event_source);
         s->fd = safe_close(s->fd);
+
+        /* Disconnect us from the server object if we are now not usable anymore */
+        dns_stream_detach(s);
 }
 
 static int dns_stream_update_io(DnsStream *s) {
@@ -46,6 +49,8 @@ static int dns_stream_update_io(DnsStream *s) {
 }
 
 static int dns_stream_complete(DnsStream *s, int error) {
+        _cleanup_(dns_stream_unrefp) _unused_ DnsStream *ref = dns_stream_ref(s); /* Protect stream while we process it */
+
         assert(s);
 
 #if ENABLE_DNS_OVER_TLS
@@ -59,6 +64,8 @@ static int dns_stream_complete(DnsStream *s, int error) {
 #endif
                 dns_stream_stop(s);
 
+        dns_stream_detach(s);
+
         if (s->complete)
                 s->complete(s, error);
         else /* the default action if no completion function is set is to close the stream */
@@ -193,7 +200,7 @@ static int dns_stream_identify(DnsStream *s) {
 }
 
 ssize_t dns_stream_writev(DnsStream *s, const struct iovec *iov, size_t iovcnt, int flags) {
-        ssize_t r;
+        ssize_t m;
 
         assert(s);
         assert(iov);
@@ -203,13 +210,13 @@ ssize_t dns_stream_writev(DnsStream *s, const struct iovec *iov, size_t iovcnt,
                 ssize_t ss;
                 size_t i;
 
-                r = 0;
+                m = 0;
                 for (i = 0; i < iovcnt; i++) {
                         ss = dnstls_stream_write(s, iov[i].iov_base, iov[i].iov_len);
                         if (ss < 0)
                                 return ss;
 
-                        r += ss;
+                        m += ss;
                         if (ss != (ssize_t) iov[i].iov_len)
                                 continue;
                 }
@@ -223,28 +230,28 @@ ssize_t dns_stream_writev(DnsStream *s, const struct iovec *iov, size_t iovcnt,
                         .msg_namelen = s->tfo_salen
                 };
 
-                r = sendmsg(s->fd, &hdr, MSG_FASTOPEN);
-                if (r < 0) {
+                m = sendmsg(s->fd, &hdr, MSG_FASTOPEN);
+                if (m < 0) {
                         if (errno == EOPNOTSUPP) {
                                 s->tfo_salen = 0;
-                                r = connect(s->fd, &s->tfo_address.sa, s->tfo_salen);
-                                if (r < 0)
+                                if (connect(s->fd, &s->tfo_address.sa, s->tfo_salen) < 0)
                                         return -errno;
 
-                                r = -EAGAIN;
-                        } else if (errno == EINPROGRESS)
-                                r = -EAGAIN;
-                        else
-                                r = -errno;
+                                return -EAGAIN;
+                        }
+                        if (errno == EINPROGRESS)
+                                return -EAGAIN;
+
+                        return -errno;
                 } else
                         s->tfo_salen = 0; /* connection is made */
         } else {
-                r = writev(s->fd, iov, iovcnt);
-                if (r < 0)
-                        r = -errno;
+                m = writev(s->fd, iov, iovcnt);
+                if (m < 0)
+                        return -errno;
         }
 
-        return r;
+        return m;
 }
 
 static ssize_t dns_stream_read(DnsStream *s, void *buf, size_t count) {
@@ -258,7 +265,7 @@ static ssize_t dns_stream_read(DnsStream *s, void *buf, size_t count) {
         {
                 ss = read(s->fd, buf, count);
                 if (ss < 0)
-                        ss = -errno;
+                        return -errno;
         }
 
         return ss;
@@ -273,7 +280,7 @@ static int on_stream_timeout(sd_event_source *es, usec_t usec, void *userdata) {
 }
 
 static int on_stream_io(sd_event_source *es, int fd, uint32_t revents, void *userdata) {
-        DnsStream *s = userdata;
+        _cleanup_(dns_stream_unrefp) DnsStream *s = dns_stream_ref(userdata); /* Protect stream while we process it */
         int r;
 
         assert(s);
@@ -281,18 +288,16 @@ static int on_stream_io(sd_event_source *es, int fd, uint32_t revents, void *use
 #if ENABLE_DNS_OVER_TLS
         if (s->encrypted) {
                 r = dnstls_stream_on_io(s, revents);
-
                 if (r == DNSTLS_STREAM_CLOSED)
                         return 0;
-                else if (r == -EAGAIN)
+                if (r == -EAGAIN)
                         return dns_stream_update_io(s);
-                else if (r < 0) {
+                if (r < 0)
                         return dns_stream_complete(s, -r);
-                } else {
-                        r = dns_stream_update_io(s);
-                        if (r < 0)
-                                return r;
-                }
+
+                r = dns_stream_update_io(s);
+                if (r < 0)
+                        return r;
         }
 #endif
 
@@ -430,9 +435,6 @@ static DnsStream *dns_stream_free(DnsStream *s) {
 
         dns_stream_stop(s);
 
-        if (s->server && s->server->stream == s)
-                s->server->stream = NULL;
-
         if (s->manager) {
                 LIST_REMOVE(streams, s->manager->dns_streams, s);
                 s->manager->n_dns_streams--;
@@ -457,28 +459,37 @@ static DnsStream *dns_stream_free(DnsStream *s) {
 
 DEFINE_TRIVIAL_REF_UNREF_FUNC(DnsStream, dns_stream, dns_stream_free);
 
-int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd, const union sockaddr_union *tfo_address) {
+int dns_stream_new(
+                Manager *m,
+                DnsStream **ret,
+                DnsProtocol protocol,
+                int fd,
+                const union sockaddr_union *tfo_address) {
+
         _cleanup_(dns_stream_unrefp) DnsStream *s = NULL;
         int r;
 
         assert(m);
+        assert(ret);
         assert(fd >= 0);
 
         if (m->n_dns_streams > DNS_STREAMS_MAX)
                 return -EBUSY;
 
-        s = new0(DnsStream, 1);
+        s = new(DnsStream, 1);
         if (!s)
                 return -ENOMEM;
 
+        *s = (DnsStream) {
+                .n_ref = 1,
+                .fd = -1,
+                .protocol = protocol,
+        };
+
         r = ordered_set_ensure_allocated(&s->write_queue, &dns_packet_hash_ops);
         if (r < 0)
                 return r;
 
-        s->n_ref = 1;
-        s->fd = -1;
-        s->protocol = protocol;
-
         r = sd_event_add_io(m->event, &s->io_event_source, fd, EPOLLIN, on_stream_io, s);
         if (r < 0)
                 return r;
@@ -497,15 +508,16 @@ int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd, co
         (void) sd_event_source_set_description(s->timeout_event_source, "dns-stream-timeout");
 
         LIST_PREPEND(streams, m->dns_streams, s);
+        m->n_dns_streams++;
         s->manager = m;
+
         s->fd = fd;
+
         if (tfo_address) {
                 s->tfo_address = *tfo_address;
                 s->tfo_salen = tfo_address->sa.sa_family == AF_INET6 ? sizeof(tfo_address->in6) : sizeof(tfo_address->in);
         }
 
-        m->n_dns_streams++;
-
         *ret = TAKE_PTR(s);
 
         return 0;
@@ -515,6 +527,7 @@ int dns_stream_write_packet(DnsStream *s, DnsPacket *p) {
         int r;
 
         assert(s);
+        assert(p);
 
         r = ordered_set_put(s->write_queue, p);
         if (r < 0)
@@ -524,3 +537,31 @@ int dns_stream_write_packet(DnsStream *s, DnsPacket *p) {
 
         return dns_stream_update_io(s);
 }
+
+DnsPacket *dns_stream_take_read_packet(DnsStream *s) {
+        assert(s);
+
+        if (!s->read_packet)
+                return NULL;
+
+        if (s->n_read < sizeof(s->read_size))
+                return NULL;
+
+        if (s->n_read < sizeof(s->read_size) + be16toh(s->read_size))
+                return NULL;
+
+        s->n_read = 0;
+        return TAKE_PTR(s->read_packet);
+}
+
+void dns_stream_detach(DnsStream *s) {
+        assert(s);
+
+        if (!s->server)
+                return;
+
+        if (s->server->stream != s)
+                return;
+
+        dns_server_unref_stream(s->server);
+}
diff --git a/src/resolve/resolved-dns-stream.h b/src/resolve/resolved-dns-stream.h
index 46d2704afe..f18fc919e7 100644
--- a/src/resolve/resolved-dns-stream.h
+++ b/src/resolve/resolved-dns-stream.h
@@ -53,13 +53,12 @@ struct DnsStream {
         size_t n_written, n_read;
         OrderedSet *write_queue;
 
-        int (*on_connection)(DnsStream *s);
         int (*on_packet)(DnsStream *s);
         int (*complete)(DnsStream *s, int error);
 
         LIST_HEAD(DnsTransaction, transactions); /* when used by the transaction logic */
         DnsServer *server;                       /* when used by the transaction logic */
-        DnsQuery *query;             /* when used by the DNS stub logic */
+        DnsQuery *query;                         /* when used by the DNS stub logic */
 
         /* used when DNS-over-TLS is enabled */
         bool encrypted:1;
@@ -87,3 +86,7 @@ static inline bool DNS_STREAM_QUEUED(DnsStream *s) {
 
         return !!s->write_packet;
 }
+
+DnsPacket *dns_stream_take_read_packet(DnsStream *s);
+
+void dns_stream_detach(DnsStream *s);
diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c
index 015aabaf9b..a00716cd85 100644
--- a/src/resolve/resolved-dns-stub.c
+++ b/src/resolve/resolved-dns-stub.c
@@ -437,13 +437,17 @@ static int manager_dns_stub_udp_fd(Manager *m) {
 }
 
 static int on_dns_stub_stream_packet(DnsStream *s) {
+        _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
+
         assert(s);
-        assert(s->read_packet);
 
-        if (dns_packet_validate_query(s->read_packet) > 0) {
-                log_debug("Got DNS stub TCP query packet for id %u", DNS_PACKET_ID(s->read_packet));
+        p = dns_stream_take_read_packet(s);
+        assert(p);
+
+        if (dns_packet_validate_query(p) > 0) {
+                log_debug("Got DNS stub TCP query packet for id %u", DNS_PACKET_ID(p));
 
-                dns_stub_process_query(s->manager, s, s->read_packet);
+                dns_stub_process_query(s->manager, s, p);
         } else
                 log_debug("Invalid DNS stub TCP packet, ignoring.");
 
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index ddab2850df..cc748ac95e 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -503,59 +503,54 @@ static int dns_transaction_on_stream_packet(DnsTransaction *t, DnsPacket *p) {
 }
 
 static int on_stream_complete(DnsStream *s, int error) {
-        _cleanup_(dns_stream_unrefp) DnsStream *p = NULL;
-        DnsTransaction *t, *n;
-        int r = 0;
-
-        /* Do not let new transactions use this stream */
-        if (s->server && s->server->stream == s)
-                p = TAKE_PTR(s->server->stream);
+        assert(s);
 
         if (ERRNO_IS_DISCONNECT(error) && s->protocol != DNS_PROTOCOL_LLMNR) {
-                usec_t usec;
-
                 log_debug_errno(error, "Connection failure for DNS TCP stream: %m");
 
                 if (s->transactions) {
+                        DnsTransaction *t;
+
                         t = s->transactions;
-                        assert_se(sd_event_now(t->scope->manager->event, clock_boottime_or_monotonic(), &usec) >= 0);
                         dns_server_packet_lost(t->server, IPPROTO_TCP, t->current_feature_level);
                 }
         }
 
-        LIST_FOREACH_SAFE(transactions_by_stream, t, n, s->transactions)
-                if (error != 0)
+        if (error != 0) {
+                DnsTransaction *t, *n;
+
+                LIST_FOREACH_SAFE(transactions_by_stream, t, n, s->transactions)
                         on_transaction_stream_error(t, error);
-                else if (DNS_PACKET_ID(s->read_packet) == t->id)
-                        /* As each transaction have a unique id the return code is only set once */
-                        r = dns_transaction_on_stream_packet(t, s->read_packet);
+        }
 
-        return r;
+        return 0;
 }
 
-static int dns_stream_on_packet(DnsStream *s) {
+static int on_stream_packet(DnsStream *s) {
         _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
-        int r = 0;
         DnsTransaction *t;
 
+        assert(s);
+
         /* Take ownership of packet to be able to receive new packets */
-        p = TAKE_PTR(s->read_packet);
-        s->n_read = 0;
+        p = dns_stream_take_read_packet(s);
+        assert(p);
 
         t = hashmap_get(s->manager->dns_transactions, UINT_TO_PTR(DNS_PACKET_ID(p)));
+        if (t)
+                return dns_transaction_on_stream_packet(t, p);
 
         /* Ignore incorrect transaction id as transaction can have been canceled */
-        if (t)
-                r = dns_transaction_on_stream_packet(t, p);
-        else {
-                if (dns_packet_validate_reply(p) <= 0) {
-                        log_debug("Invalid TCP reply packet.");
-                        on_stream_complete(s, 0);
-                }
-                return 0;
+        if (dns_packet_validate_reply(p) <= 0) {
+                log_debug("Invalid TCP reply packet.");
+                on_stream_complete(s, 0);
         }
 
-        return r;
+        return 0;
+}
+
+static uint16_t dns_port_for_feature_level(DnsServerFeatureLevel level) {
+        return DNS_SERVER_FEATURE_LEVEL_IS_TLS(level) ? 853 : 53;
 }
 
 static int dns_transaction_emit_tcp(DnsTransaction *t) {
@@ -585,7 +580,7 @@ static int dns_transaction_emit_tcp(DnsTransaction *t) {
                 if (t->server->stream && (DNS_SERVER_FEATURE_LEVEL_IS_TLS(t->current_feature_level) == t->server->stream->encrypted))
                         s = dns_stream_ref(t->server->stream);
                 else
-                        fd = dns_scope_socket_tcp(t->scope, AF_UNSPEC, NULL, t->server, DNS_SERVER_FEATURE_LEVEL_IS_TLS(t->current_feature_level) ? 853 : 53, &sa);
+                        fd = dns_scope_socket_tcp(t->scope, AF_UNSPEC, NULL, t->server, dns_port_for_feature_level(t->current_feature_level), &sa);
 
                 break;
 
@@ -629,7 +624,9 @@ static int dns_transaction_emit_tcp(DnsTransaction *t) {
                 fd = -1;
 
 #if ENABLE_DNS_OVER_TLS
-                if (DNS_SERVER_FEATURE_LEVEL_IS_TLS(t->current_feature_level)) {
+                if (t->scope->protocol == DNS_PROTOCOL_DNS &&
+                    DNS_SERVER_FEATURE_LEVEL_IS_TLS(t->current_feature_level)) {
+
                         assert(t->server);
                         r = dnstls_stream_connect_tls(s, t->server);
                         if (r < 0)
@@ -638,13 +635,13 @@ static int dns_transaction_emit_tcp(DnsTransaction *t) {
 #endif
 
                 if (t->server) {
-                        dns_stream_unref(t->server->stream);
+                        dns_server_unref_stream(t->server);
                         t->server->stream = dns_stream_ref(s);
                         s->server = dns_server_ref(t->server);
                 }
 
                 s->complete = on_stream_complete;
-                s->on_packet = dns_stream_on_packet;
+                s->on_packet = on_stream_packet;
 
                 /* The interface index is difficult to determine if we are
                  * connecting to the local host, hence fill this in right away
diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c
index 65f5ceecd0..dfa55c577c 100644
--- a/src/resolve/resolved-llmnr.c
+++ b/src/resolve/resolved-llmnr.c
@@ -260,18 +260,21 @@ int manager_llmnr_ipv6_udp_fd(Manager *m) {
 }
 
 static int on_llmnr_stream_packet(DnsStream *s) {
+        _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
         DnsScope *scope;
 
         assert(s);
-        assert(s->read_packet);
 
-        scope = manager_find_scope(s->manager, s->read_packet);
+        p = dns_stream_take_read_packet(s);
+        assert(p);
+
+        scope = manager_find_scope(s->manager, p);
         if (!scope)
                 log_debug("Got LLMNR TCP packet on unknown scope. Ignoring.");
-        else if (dns_packet_validate_query(s->read_packet) > 0) {
-                log_debug("Got LLMNR TCP query packet for id %u", DNS_PACKET_ID(s->read_packet));
+        else if (dns_packet_validate_query(p) > 0) {
+                log_debug("Got LLMNR TCP query packet for id %u", DNS_PACKET_ID(p));
 
-                dns_scope_process_query(scope, s, s->read_packet);
+                dns_scope_process_query(scope, s, p);
         } else
                 log_debug("Invalid LLMNR TCP packet, ignoring.");