resolve: llmnr: fix never hit condition

yuwata · yuwata · commit a5e2a488e83f · 2022-01-28T09:37:50.000+09:00
Previously, the condition in on_stream_io_impl() never hit, as the read packet is always taken from the stream in the few lines above. Instead of the dns_stream_complete() under the condition, the stream is unref()ed in the on_packet callback for LLMNR stream, unlike the other on_packet callbacks. That's quite tricky. Also, potentially, the stream may still have queued packets to write. This fix the condition, and drops the unref() in the on_packet callback. C.f. systemd#22274 (comment). Closes systemd#22266.
diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c
@@ -446,17 +446,25 @@ static int on_stream_io_impl(DnsStream *s, uint32_t revents) {
                                 r = dns_stream_update_io(s);
                                 if (r < 0)
                                         return dns_stream_complete(s, -r);
+
+                                s->packet_received = true;
                         }
                 }
         }
 
-        /* Call "complete" callback if finished reading and writing one packet, and there's nothing else left
-         * to write. */
-        if (s->type == DNS_STREAM_LLMNR_SEND &&
-            (s->write_packet && s->n_written >= sizeof(s->write_size) + s->write_packet->size) &&
-            ordered_set_isempty(s->write_queue) &&
-            (s->read_packet && s->n_read >= sizeof(s->read_size) + s->read_packet->size))
-                return dns_stream_complete(s, 0);
+        if (s->type == DNS_STREAM_LLMNR_SEND && s->packet_received) {
+                uint32_t events;
+
+                /* Complete the stream if finished reading and writing one packet, and there's nothing
+                 * else left to write. */
+
+                r = sd_event_source_get_io_events(s->io_event_source, &events);
+                if (r < 0)
+                        return r;
+
+                if (!FLAGS_SET(events, EPOLLOUT))
+                        return dns_stream_complete(s, 0);
+        }
 
         /* If we did something, let's restart the timeout event source */
         if (progressed && s->timeout_event_source) {
diff --git a/src/resolve/resolved-dns-stream.h b/src/resolve/resolved-dns-stream.h
@@ -60,6 +60,7 @@ struct DnsStream {
         int ifindex;
         uint32_t ttl;
         bool identified;
+        bool packet_received; /* At least one packet is received. Used by LLMNR. */
 
         /* only when using TCP fast open */
         union sockaddr_union tfo_address;
diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c
@@ -294,7 +294,6 @@ static int on_llmnr_stream_packet(DnsStream *s, DnsPacket *p) {
         } else
                 log_debug("Invalid LLMNR TCP packet, ignoring.");
 
-        dns_stream_unref(s);
         return 0;
 }
 
@@ -311,8 +310,7 @@ static int on_llmnr_stream(sd_event_source *s, int fd, uint32_t revents, void *u
                 return -errno;
         }
 
-        /* We don't configure a "complete" handler here, we rely on the default handler than simply drops the
-         * reference to the stream, thus freeing it */
+        /* We don't configure a "complete" handler here, we rely on the default handler, thus freeing it */
         r = dns_stream_new(m, &stream, DNS_STREAM_LLMNR_RECV, DNS_PROTOCOL_LLMNR, cfd, NULL,
                            on_llmnr_stream_packet, NULL, DNS_STREAM_DEFAULT_TIMEOUT_USEC);
         if (r < 0) {

Original file line number	Diff line number	Diff line change
`@@ -294,7 +294,6 @@ static int on_llmnr_stream_packet(DnsStream s, DnsPacket p) {`
`294`	`294`	`} else`
`295`	`295`	`log_debug("Invalid LLMNR TCP packet, ignoring.");`
`296`	`296`
`297`		`- dns_stream_unref(s);`
`298`	`297`	`return 0;`
`299`	`298`	`}`
`300`	`299`
`@@ -311,8 +310,7 @@ static int on_llmnr_stream(sd_event_source s, int fd, uint32_t revents, void u`
`311`	`310`	`return -errno;`
`312`	`311`	`}`
`313`	`312`
`314`		`- /* We don't configure a "complete" handler here, we rely on the default handler than simply drops the`
`315`		`- * reference to the stream, thus freeing it */`
	`313`	`+ /* We don't configure a "complete" handler here, we rely on the default handler, thus freeing it */`
`316`	`314`	`r = dns_stream_new(m, &stream, DNS_STREAM_LLMNR_RECV, DNS_PROTOCOL_LLMNR, cfd, NULL,`
`317`	`315`	`on_llmnr_stream_packet, NULL, DNS_STREAM_DEFAULT_TIMEOUT_USEC);`
`318`	`316`	`if (r < 0) {`