From b2cf6704e724ad9bb2912adab8a222ba130d2103 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:13:55 +0100
Subject: [PATCH 01/20] resolved: simplify on_stream_io() a bit

---
 src/resolve/resolved-dns-stream.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 26d4663d746..8964a8636aa 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -281,18 +281,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
 

From 898892e825e7a05891a9f8a5616f041958ef4d61 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:14:28 +0100
Subject: [PATCH 02/20] resolved: ensure DnsStream.fd is initialized before
 first error path

---
 src/resolve/resolved-dns-stream.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 8964a8636aa..5aefb94407e 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -465,18 +465,20 @@ int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd, co
         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;

From 08e254c8184e03eafab1d06703ac0078a044b80f Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:14:51 +0100
Subject: [PATCH 03/20] resolved: reorder things, to place registration of
 DnsStream in Manager close to each other

---
 src/resolve/resolved-dns-stream.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 5aefb94407e..0112e301ac7 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -497,15 +497,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;

From b27a32a0def7dfd32aa9c311cdf14fd90b8d1297 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:26:54 +0100
Subject: [PATCH 04/20] resolved: line split dns_stream_new() function
 signature

---
 src/resolve/resolved-dns-stream.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 0112e301ac7..7473cc40a44 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -455,7 +455,13 @@ 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;
 

From e6dc55566bb3633dcb959167aef4eb13e24c3204 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:27:25 +0100
Subject: [PATCH 05/20] resolved: be more careful with types in
 dns_stream_writev()

Let's not name a variable of type ssize_t "r". We usually use "r" for
return values of API calls that return some kind of error as in int.
This creates a lot of confusion if used differently here, which actually
resulted in connect()'s return value being assigned to this mistyped "r"
by accident.

Let's rename the variable "m" hence, and not use it for connect() return
values.
---
 src/resolve/resolved-dns-stream.c | 32 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 7473cc40a44..1db7cf53071 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -193,7 +193,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 +203,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 +223,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) {

From 499aa1d31b1607a4dcd81b9b19c757ee4f4243b9 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:29:15 +0100
Subject: [PATCH 06/20] resolved: add some assert()s

---
 src/resolve/resolved-dns-stream.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 1db7cf53071..b71b898fde5 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -466,6 +466,7 @@ int dns_stream_new(
         int r;
 
         assert(m);
+        assert(ret);
         assert(fd >= 0);
 
         if (m->n_dns_streams > DNS_STREAMS_MAX)
@@ -522,6 +523,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)

From 56e267dee20fbd4565d27870f2903d4d31881818 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 19:50:11 +0100
Subject: [PATCH 07/20] resolved: remove redundant code

---
 src/resolve/resolved-dns-transaction.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index ddab2850dfd..095f222cae9 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -512,13 +512,10 @@ static int on_stream_complete(DnsStream *s, int error) {
                 p = TAKE_PTR(s->server->stream);
 
         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) {
                         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);
                 }
         }

From aa337a5e729829058888a4da0b5ad5813dd1d5b4 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:18:11 +0100
Subject: [PATCH 08/20] resolved: add new accessor
 dns_stream_take_read_packet() for taking read packet from stream

This ensures the packet is complete when it is taken out, and resets
n_read so that we can start reading the next one.
---
 src/resolve/resolved-dns-stream.c      | 16 ++++++++++++++++
 src/resolve/resolved-dns-stream.h      |  2 ++
 src/resolve/resolved-dns-stub.c        | 12 ++++++++----
 src/resolve/resolved-dns-transaction.c | 22 ++++++++++------------
 src/resolve/resolved-llmnr.c           | 13 ++++++++-----
 5 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index b71b898fde5..be9e8a67b83 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -533,3 +533,19 @@ 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);
+}
diff --git a/src/resolve/resolved-dns-stream.h b/src/resolve/resolved-dns-stream.h
index 46d2704afef..0d04ae77c49 100644
--- a/src/resolve/resolved-dns-stream.h
+++ b/src/resolve/resolved-dns-stream.h
@@ -87,3 +87,5 @@ static inline bool DNS_STREAM_QUEUED(DnsStream *s) {
 
         return !!s->write_packet;
 }
+
+DnsPacket *dns_stream_take_read_packet(DnsStream *s);
diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c
index 015aabaf9bb..a00716cd857 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);
 
-                dns_stub_process_query(s->manager, s, s->read_packet);
+        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, 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 095f222cae9..b23ca54ade6 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -532,27 +532,25 @@ static int on_stream_complete(DnsStream *s, int error) {
 
 static int dns_stream_on_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 int dns_transaction_emit_tcp(DnsTransaction *t) {
diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c
index 65f5ceecd01..dfa55c577c4 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.");
 

From 97d5d9055f0cdfdfee901452848a519b49896382 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:19:16 +0100
Subject: [PATCH 09/20] resolved: don't read packet from DnsStream on
 on_stream_complete() of DnsTransaction

We register an on_packet() handler anyway, which is called first.
There's hence no need to check in on_stream_complete() again, as it is
already taken by that time.
---
 src/resolve/resolved-dns-transaction.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index b23ca54ade6..e26241754d8 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -504,8 +504,6 @@ 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)
@@ -515,19 +513,21 @@ static int on_stream_complete(DnsStream *s, int error) {
                 log_debug_errno(error, "Connection failure for DNS TCP stream: %m");
 
                 if (s->transactions) {
+                        DnsTransaction *t;
+
                         t = s->transactions;
                         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)
-                        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);
+        if (error != 0) {
+                DnsTransaction *t, *n;
 
-        return r;
+                LIST_FOREACH_SAFE(transactions_by_stream, t, n, s->transactions)
+                        on_transaction_stream_error(t, error);
+        }
+
+        return 0;
 }
 
 static int dns_stream_on_packet(DnsStream *s) {

From 94fdb4d9d11827c353e3bf116dfdff75ed0f6153 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:21:02 +0100
Subject: [PATCH 10/20] resolved: exit early on failure

---
 src/resolve/resolved-dns-stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index be9e8a67b83..d35c875e473 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -258,7 +258,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;

From 65b0179a25c6408f218912cd804403170d43fd38 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:24:26 +0100
Subject: [PATCH 11/20] resolved: use structured initialization for DnsServer
 allocation

---
 src/resolve/resolved-dns-server.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c
index e05ada29a88..c7f9de2cbd9 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);
 

From 747a8a74c04f986eb2a1f56829d3b5634d00f56e Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:40:20 +0100
Subject: [PATCH 12/20] =?UTF-8?q?resolved:=20rename=20dns=5Fstream=5Fon=5F?=
 =?UTF-8?q?packet()=20=E2=86=92=20on=5Fstream=5Fpacket()?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Let's name this similar to on_stream_complete(). Moreover we shouldn't
invade dns_stream's namespace if we are a consumer of it.
---
 src/resolve/resolved-dns-transaction.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index e26241754d8..f9b71a35c9a 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -530,7 +530,7 @@ static int on_stream_complete(DnsStream *s, int error) {
         return 0;
 }
 
-static int dns_stream_on_packet(DnsStream *s) {
+static int on_stream_packet(DnsStream *s) {
         _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL;
         DnsTransaction *t;
 
@@ -639,7 +639,7 @@ static int dns_transaction_emit_tcp(DnsTransaction *t) {
                 }
 
                 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

From ec962fba62f6741683c954a5f6391e193e86d4ee Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:41:06 +0100
Subject: [PATCH 13/20] resolved: add small helper to pick DNS port number

This shouldn't be hidden in some function argument expression. Let's
make this more explicit by turning this into its own helper function.
---
 src/resolve/resolved-dns-transaction.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index f9b71a35c9a..4c0acf3a3fd 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -553,6 +553,10 @@ static int on_stream_packet(DnsStream *s) {
         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) {
         _cleanup_close_ int fd = -1;
         _cleanup_(dns_stream_unrefp) DnsStream *s = NULL;
@@ -580,7 +584,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;
 

From 51bc63fef2be2e988222537c6a39fe07f75fd97a Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:52:43 +0100
Subject: [PATCH 14/20] resolved: comment headers better

---
 src/resolve/resolved-dns-server.h | 2 ++
 src/resolve/resolved-dns-stream.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h
index fda12510495..a6022ad97f5 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
diff --git a/src/resolve/resolved-dns-stream.h b/src/resolve/resolved-dns-stream.h
index 0d04ae77c49..424278ea0a6 100644
--- a/src/resolve/resolved-dns-stream.h
+++ b/src/resolve/resolved-dns-stream.h
@@ -59,7 +59,7 @@ struct DnsStream {
 
         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;

From 199dda9c25e02ac69c9a751a1e7b837a747cb630 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 20:53:14 +0100
Subject: [PATCH 15/20] resolved: before assuming we have a server, check we
 are talking DNS and not LLMNR/mDNS

---
 src/resolve/resolved-dns-transaction.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 4c0acf3a3fd..73e6306ba1d 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -628,7 +628,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)

From 904dcaf9d4933499f8334859f52ea8497f2d24ff Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 22:09:08 +0100
Subject: [PATCH 16/20] resolved: take particular care when detaching DnsServer
 from its default stream

DnsStream and DnsServer have a symbiotic relationship: one DnsStream is
the current "default" stream of the server (and thus reffed by it), but
each stream also refs the server it is connected to. This cyclic
dependency can result in weird situations: when one is
destroyed/unlinked/stopped it needs to unregister itself from the other,
but doing this will trigger unregistration of the other. Hence, let's
make sure we unregister the stream from the server before destroying it,
to break this cycle.

Most likely fixes: #10725
---
 src/resolve/resolved-dns-server.c      | 22 +++++++++++++++++++++-
 src/resolve/resolved-dns-server.h      |  2 ++
 src/resolve/resolved-dns-transaction.c |  2 +-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c
index c7f9de2cbd9..3e69741b880 100644
--- a/src/resolve/resolved-dns-server.c
+++ b/src/resolve/resolved-dns-server.c
@@ -103,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);
@@ -158,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);
 }
 
@@ -826,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) {
@@ -886,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 a6022ad97f5..6e73f32df45 100644
--- a/src/resolve/resolved-dns-server.h
+++ b/src/resolve/resolved-dns-server.h
@@ -151,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-transaction.c b/src/resolve/resolved-dns-transaction.c
index 73e6306ba1d..f29a68e444a 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -639,7 +639,7 @@ 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);
                 }

From d973d94dec349fb676fdd844f6fe2ada3538f27c Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 22:13:39 +0100
Subject: [PATCH 17/20] resolved: pin stream while calling callbacks for it

These callbacks might unref the stream, but we still have to access it,
let's hence ref it explicitly.

Maybe fixes: #10725
---
 src/resolve/resolved-dns-stream.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index d35c875e473..deb5abac5be 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -46,6 +46,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
@@ -273,7 +275,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);

From 808089ae3d96f0e015e323188f0795c3446d3b96 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 22:15:22 +0100
Subject: [PATCH 18/20] resolved: add new helper for carefully detach a stream
 from any server

This adds a helper call for detaching a DnsServer from a DnsStream if
the latter is the "default" stream of the server.

Also, let's unref the stream in dns_stream_stop() rather than
dns_stream_free(): as soon as our stream is disconnected by stopping
there's really no need to keep it as default stream for the server
around.

Since dns_stream_free() calls dns_stream_stop() we can remove it from
the former.
---
 src/resolve/resolved-dns-stream.c | 18 +++++++++++++++---
 src/resolve/resolved-dns-stream.h |  2 ++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index deb5abac5be..8bca32d2532 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) {
@@ -430,9 +433,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--;
@@ -551,3 +551,15 @@ DnsPacket *dns_stream_take_read_packet(DnsStream *s) {
         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 424278ea0a6..3faec834679 100644
--- a/src/resolve/resolved-dns-stream.h
+++ b/src/resolve/resolved-dns-stream.h
@@ -89,3 +89,5 @@ static inline bool DNS_STREAM_QUEUED(DnsStream *s) {
 }
 
 DnsPacket *dns_stream_take_read_packet(DnsStream *s);
+
+void dns_stream_detach(DnsStream *s);

From 7172e4ee1ea27c33f8d125132a7498f1182ea784 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 22:17:49 +0100
Subject: [PATCH 19/20] resolved: implicitly disconnect a stream from its
 server when a stream is closed

Previously, the callback function did this, but let's do this in the
caller instead, to make this more robust, and use our new helper
function for it.
---
 src/resolve/resolved-dns-stream.c      | 2 ++
 src/resolve/resolved-dns-transaction.c | 6 +-----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
index 8bca32d2532..aee339a4c8d 100644
--- a/src/resolve/resolved-dns-stream.c
+++ b/src/resolve/resolved-dns-stream.c
@@ -64,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 */
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index f29a68e444a..cc748ac95ea 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -503,11 +503,7 @@ 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;
-
-        /* 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) {
                 log_debug_errno(error, "Connection failure for DNS TCP stream: %m");

From c23b62b40ba857afd72a260e4a92d3b13af9c272 Mon Sep 17 00:00:00 2001
From: Lennart Poettering <lennart@poettering.net>
Date: Tue, 4 Dec 2018 22:18:57 +0100
Subject: [PATCH 20/20] resolved: drop unused field structure

---
 src/resolve/resolved-dns-stream.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/resolve/resolved-dns-stream.h b/src/resolve/resolved-dns-stream.h
index 3faec834679..f18fc919e73 100644
--- a/src/resolve/resolved-dns-stream.h
+++ b/src/resolve/resolved-dns-stream.h
@@ -53,7 +53,6 @@ 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);