From 97935302283729c9206b84f5e00b1aff0f78ad19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 1 Mar 2021 23:10:06 +0100 Subject: [PATCH] resolved: disable event sources before unreffing them We generally operate on the assumption that a source is "gone" as soon as we unref it. This is generally true because we have the only reference. But if something else holds the reference, our unref doesn't really stop the source and it could fire again. In particular, on_query_timeout() is called with DnsQuery* as userdata, and it calls dns_query_stop() which invalidates that pointer. If it was ever called again, we'd be accessing already-freed memory. I don't see what would hold the reference. sd-event takes a temporary reference, but on the sd_event object, not on the individual sources. And our sources are non-floating, so there is no reference from the sd_event object to the sources. For #18427. --- src/resolve/resolved-dns-query.c | 2 +- src/resolve/resolved-dns-scope.c | 8 ++++---- src/resolve/resolved-dns-stream.c | 4 ++-- src/resolve/resolved-dns-stub.c | 10 +++++----- src/resolve/resolved-dns-transaction.c | 4 ++-- src/resolve/resolved-llmnr.c | 8 ++++---- src/resolve/resolved-manager.c | 2 +- src/resolve/resolved-mdns.c | 4 ++-- src/resolve/resolved-socket-graveyard.c | 2 +- 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 7fb2e110e0..7554d1e82f 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -326,7 +326,7 @@ static void dns_query_stop(DnsQuery *q) { assert(q); - q->timeout_event_source = sd_event_source_unref(q->timeout_event_source); + q->timeout_event_source = sd_event_source_disable_unref(q->timeout_event_source); LIST_FOREACH(candidates_by_query, c, q->candidates) dns_query_candidate_stop(c); diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index 8eb91a013b..67c6f54dc5 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -111,9 +111,9 @@ DnsScope* dns_scope_free(DnsScope *s) { hashmap_free(s->transactions_by_key); ordered_hashmap_free_with_destructor(s->conflict_queue, dns_resource_record_unref); - sd_event_source_unref(s->conflict_event_source); + sd_event_source_disable_unref(s->conflict_event_source); - sd_event_source_unref(s->announce_event_source); + sd_event_source_disable_unref(s->announce_event_source); dns_cache_flush(&s->cache); dns_zone_flush(&s->zone); @@ -1147,7 +1147,7 @@ static int on_conflict_dispatch(sd_event_source *es, usec_t usec, void *userdata assert(es); assert(scope); - scope->conflict_event_source = sd_event_source_unref(scope->conflict_event_source); + scope->conflict_event_source = sd_event_source_disable_unref(scope->conflict_event_source); for (;;) { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; @@ -1358,7 +1358,7 @@ static int on_announcement_timeout(sd_event_source *s, usec_t usec, void *userda assert(s); - scope->announce_event_source = sd_event_source_unref(scope->announce_event_source); + scope->announce_event_source = sd_event_source_disable_unref(scope->announce_event_source); (void) dns_scope_announce(scope, false); return 0; diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c index 2ab6f6236d..10641f6ac5 100644 --- a/src/resolve/resolved-dns-stream.c +++ b/src/resolve/resolved-dns-stream.c @@ -18,8 +18,8 @@ static void dns_stream_stop(DnsStream *s) { assert(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->io_event_source = sd_event_source_disable_unref(s->io_event_source); + s->timeout_event_source = sd_event_source_disable_unref(s->timeout_event_source); s->fd = safe_close(s->fd); /* Disconnect us from the server object if we are now not usable anymore */ diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 2cfabb3629..c2734e57b9 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -81,8 +81,8 @@ DnsStubListenerExtra *dns_stub_listener_extra_free(DnsStubListenerExtra *p) { if (!p) return NULL; - p->udp_event_source = sd_event_source_unref(p->udp_event_source); - p->tcp_event_source = sd_event_source_unref(p->tcp_event_source); + p->udp_event_source = sd_event_source_disable_unref(p->udp_event_source); + p->tcp_event_source = sd_event_source_disable_unref(p->tcp_event_source); hashmap_free(p->queries_by_packet); @@ -1257,12 +1257,12 @@ int manager_dns_stub_start(Manager *m) { void manager_dns_stub_stop(Manager *m) { assert(m); - m->dns_stub_udp_event_source = sd_event_source_unref(m->dns_stub_udp_event_source); - m->dns_stub_tcp_event_source = sd_event_source_unref(m->dns_stub_tcp_event_source); + m->dns_stub_udp_event_source = sd_event_source_disable_unref(m->dns_stub_udp_event_source); + m->dns_stub_tcp_event_source = sd_event_source_disable_unref(m->dns_stub_tcp_event_source); } static const char* const dns_stub_listener_mode_table[_DNS_STUB_LISTENER_MODE_MAX] = { - [DNS_STUB_LISTENER_NO] = "no", + [DNS_STUB_LISTENER_NO] = "no", [DNS_STUB_LISTENER_UDP] = "udp", [DNS_STUB_LISTENER_TCP] = "tcp", [DNS_STUB_LISTENER_YES] = "yes", diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 590652e5d3..07c6eca3ba 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -66,7 +66,7 @@ static void dns_transaction_close_connection( t->stream = dns_stream_unref(t->stream); } - t->dns_udp_event_source = sd_event_source_unref(t->dns_udp_event_source); + t->dns_udp_event_source = sd_event_source_disable_unref(t->dns_udp_event_source); /* If we have an UDP socket where we sent a packet, but never received one, then add it to the socket * graveyard, instead of closing it right away. That way it will stick around for a moment longer, @@ -87,7 +87,7 @@ static void dns_transaction_close_connection( static void dns_transaction_stop_timeout(DnsTransaction *t) { assert(t); - t->timeout_event_source = sd_event_source_unref(t->timeout_event_source); + t->timeout_event_source = sd_event_source_disable_unref(t->timeout_event_source); } DnsTransaction* dns_transaction_free(DnsTransaction *t) { diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c index ccdf138db4..ce2db7d4b0 100644 --- a/src/resolve/resolved-llmnr.c +++ b/src/resolve/resolved-llmnr.c @@ -12,16 +12,16 @@ void manager_llmnr_stop(Manager *m) { assert(m); - m->llmnr_ipv4_udp_event_source = sd_event_source_unref(m->llmnr_ipv4_udp_event_source); + m->llmnr_ipv4_udp_event_source = sd_event_source_disable_unref(m->llmnr_ipv4_udp_event_source); m->llmnr_ipv4_udp_fd = safe_close(m->llmnr_ipv4_udp_fd); - m->llmnr_ipv6_udp_event_source = sd_event_source_unref(m->llmnr_ipv6_udp_event_source); + m->llmnr_ipv6_udp_event_source = sd_event_source_disable_unref(m->llmnr_ipv6_udp_event_source); m->llmnr_ipv6_udp_fd = safe_close(m->llmnr_ipv6_udp_fd); - m->llmnr_ipv4_tcp_event_source = sd_event_source_unref(m->llmnr_ipv4_tcp_event_source); + m->llmnr_ipv4_tcp_event_source = sd_event_source_disable_unref(m->llmnr_ipv4_tcp_event_source); m->llmnr_ipv4_tcp_fd = safe_close(m->llmnr_ipv4_tcp_fd); - m->llmnr_ipv6_tcp_event_source = sd_event_source_unref(m->llmnr_ipv6_tcp_event_source); + m->llmnr_ipv6_tcp_event_source = sd_event_source_disable_unref(m->llmnr_ipv6_tcp_event_source); m->llmnr_ipv6_tcp_fd = safe_close(m->llmnr_ipv6_tcp_fd); } diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 0b14436709..e815dacd7f 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -342,7 +342,7 @@ static int manager_clock_change_listen(Manager *m) { assert(m); - m->clock_change_event_source = sd_event_source_unref(m->clock_change_event_source); + m->clock_change_event_source = sd_event_source_disable_unref(m->clock_change_event_source); fd = time_change_fd(); if (fd < 0) diff --git a/src/resolve/resolved-mdns.c b/src/resolve/resolved-mdns.c index 15843e90b9..e3a46f4ca1 100644 --- a/src/resolve/resolved-mdns.c +++ b/src/resolve/resolved-mdns.c @@ -15,10 +15,10 @@ void manager_mdns_stop(Manager *m) { assert(m); - m->mdns_ipv4_event_source = sd_event_source_unref(m->mdns_ipv4_event_source); + m->mdns_ipv4_event_source = sd_event_source_disable_unref(m->mdns_ipv4_event_source); m->mdns_ipv4_fd = safe_close(m->mdns_ipv4_fd); - m->mdns_ipv6_event_source = sd_event_source_unref(m->mdns_ipv6_event_source); + m->mdns_ipv6_event_source = sd_event_source_disable_unref(m->mdns_ipv6_event_source); m->mdns_ipv6_fd = safe_close(m->mdns_ipv6_fd); } diff --git a/src/resolve/resolved-socket-graveyard.c b/src/resolve/resolved-socket-graveyard.c index 067cb666d4..471fe1d578 100644 --- a/src/resolve/resolved-socket-graveyard.c +++ b/src/resolve/resolved-socket-graveyard.c @@ -36,7 +36,7 @@ static SocketGraveyard* socket_graveyard_free(SocketGraveyard *g) { if (g->io_event_source) { log_debug("Closing graveyard socket fd %i", sd_event_source_get_io_fd(g->io_event_source)); - sd_event_source_unref(g->io_event_source); + sd_event_source_disable_unref(g->io_event_source); } return mfree(g);