From 73bfd7be042cc63e7649242b377ad494bf74ea4b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 5 Feb 2022 21:37:01 +0900 Subject: [PATCH 1/4] resolve: fix potential memleak and use-after-free When stub stream is closed early, then queries associated to the stream are freed. Previously, the timer event source for queries may not be disabled, hence may be triggered with already freed query. See also dns_stub_stream_complete(). Note that we usually not set NULL or zero when freeing simple objects. But, here DnsQuery is large and complicated object, and the element may be referenced in subsequent freeing process in the future. Hence, for safety, let's set NULL to the pointer. --- src/resolve/resolved-dns-query.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 3b5e456db2..192bfd3bf5 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -381,6 +381,8 @@ DnsQuery *dns_query_free(DnsQuery *q) { if (!q) return NULL; + q->timeout_event_source = sd_event_source_disable_unref(q->timeout_event_source); + while (q->auxiliary_queries) dns_query_free(q->auxiliary_queries); From fe8c5ce615ee2123f17b1f0b3728c439e19e4b5b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 5 Feb 2022 22:03:19 +0900 Subject: [PATCH 2/4] resolve: fix possible memleak Fortunately, unlike the issue fixed in the previous commit, the memleak should be superficial and not become apparent, as the queries handled here are managed by the stub stream, and will be freed when the stream is closed. Just for safety, and slightly reducing the runtime memory usage by the stub stream. --- src/resolve/resolved-dns-stub.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 992ae19bbc..d760213588 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -791,8 +791,10 @@ static void dns_stub_query_complete(DnsQuery *q) { * packet doesn't answer our question. In that case let's restart the query, * now with the redirected question. We'll */ r = dns_query_go(q); - if (r < 0) + if (r < 0) { log_debug_errno(r, "Failed to restart query: %m"); + dns_query_free(q); + } return; } From c704288c473fa08820566fdb16c38726d24db026 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 5 Feb 2022 22:04:42 +0900 Subject: [PATCH 3/4] resolve: use _cleanup_ attribute for freeing DnsQuery --- src/resolve/resolved-bus.c | 112 +++++++++++++++----------------- src/resolve/resolved-dns-stub.c | 27 +++----- src/resolve/resolved-varlink.c | 38 +++++------ 3 files changed, 79 insertions(+), 98 deletions(-) diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 48e5321d79..5607dcc29f 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -179,9 +179,10 @@ static int append_address(sd_bus_message *reply, DnsResourceRecord *rr, int ifin return 0; } -static void bus_method_resolve_hostname_complete(DnsQuery *q) { +static void bus_method_resolve_hostname_complete(DnsQuery *query) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *canonical = NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; _cleanup_free_ char *normalized = NULL; DnsQuestion *question; DnsResourceRecord *rr; @@ -202,8 +203,11 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } r = sd_bus_message_new_method_return(q->bus_request, &reply); if (r < 0) @@ -264,8 +268,6 @@ finish: log_error_errno(r, "Failed to send hostname reply: %m"); sd_bus_reply_method_errno(q->bus_request, r, NULL); } - - dns_query_free(q); } static int validate_and_mangle_flags( @@ -403,11 +405,11 @@ void bus_client_log(sd_bus_message *m, const char *what) { static int bus_method_resolve_hostname(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_(dns_question_unrefp) DnsQuestion *question_idna = NULL, *question_utf8 = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; Manager *m = userdata; const char *hostname; int family, ifindex; uint64_t flags; - DnsQuery *q; int r; assert(message); @@ -459,21 +461,19 @@ static int bus_method_resolve_hostname(sd_bus_message *message, void *userdata, r = dns_query_bus_track(q, message); if (r < 0) - goto fail; + return r; r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } -static void bus_method_resolve_address_complete(DnsQuery *q) { +static void bus_method_resolve_address_complete(DnsQuery *query) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; DnsQuestion *question; DnsResourceRecord *rr; unsigned added = 0; @@ -493,8 +493,11 @@ static void bus_method_resolve_address_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } r = sd_bus_message_new_method_return(q->bus_request, &reply); if (r < 0) @@ -550,17 +553,15 @@ finish: log_error_errno(r, "Failed to send address reply: %m"); sd_bus_reply_method_errno(q->bus_request, r, NULL); } - - dns_query_free(q); } static int bus_method_resolve_address(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; Manager *m = userdata; union in_addr_union a; int family, ifindex; uint64_t flags; - DnsQuery *q; int r; assert(message); @@ -604,17 +605,14 @@ static int bus_method_resolve_address(sd_bus_message *message, void *userdata, s r = dns_query_bus_track(q, message); if (r < 0) - goto fail; + return r; r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } static int bus_message_append_rr(sd_bus_message *m, DnsResourceRecord *rr, int ifindex) { @@ -645,8 +643,9 @@ static int bus_message_append_rr(sd_bus_message *m, DnsResourceRecord *rr, int i return sd_bus_message_close_container(m); } -static void bus_method_resolve_record_complete(DnsQuery *q) { +static void bus_method_resolve_record_complete(DnsQuery *query) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; DnsResourceRecord *rr; DnsQuestion *question; unsigned added = 0; @@ -667,8 +666,11 @@ static void bus_method_resolve_record_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } r = sd_bus_message_new_method_return(q->bus_request, &reply); if (r < 0) @@ -714,19 +716,17 @@ finish: log_error_errno(r, "Failed to send record reply: %m"); sd_bus_reply_method_errno(q->bus_request, r, NULL); } - - dns_query_free(q); } static int bus_method_resolve_record(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; Manager *m = userdata; uint16_t class, type; const char *name; int r, ifindex; uint64_t flags; - DnsQuery *q; assert(message); assert(m); @@ -782,17 +782,14 @@ static int bus_method_resolve_record(sd_bus_message *message, void *userdata, sd r = dns_query_bus_track(q, message); if (r < 0) - goto fail; + return r; r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } static int append_srv(DnsQuery *q, sd_bus_message *reply, DnsResourceRecord *rr) { @@ -952,10 +949,11 @@ static int append_txt(sd_bus_message *reply, DnsResourceRecord *rr) { return 1; } -static void resolve_service_all_complete(DnsQuery *q) { +static void resolve_service_all_complete(DnsQuery *query) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *canonical = NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_free_ char *name = NULL, *type = NULL, *domain = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; DnsQuestion *question; DnsResourceRecord *rr; unsigned added = 0; @@ -964,8 +962,10 @@ static void resolve_service_all_complete(DnsQuery *q) { assert(q); - if (q->block_all_complete > 0) + if (q->block_all_complete > 0) { + TAKE_PTR(q); return; + } if ((q->flags & SD_RESOLVED_NO_ADDRESS) == 0) { DnsQuery *bad = NULL; @@ -977,6 +977,7 @@ static void resolve_service_all_complete(DnsQuery *q) { case DNS_TRANSACTION_PENDING: /* If an auxiliary query is still pending, let's wait */ + TAKE_PTR(q); return; case DNS_TRANSACTION_SUCCESS: @@ -1093,8 +1094,6 @@ finish: log_error_errno(r, "Failed to send service reply: %m"); sd_bus_reply_method_errno(q->bus_request, r, NULL); } - - dns_query_free(q); } static void resolve_service_hostname_complete(DnsQuery *q) { @@ -1119,7 +1118,7 @@ static void resolve_service_hostname_complete(DnsQuery *q) { static int resolve_service_hostname(DnsQuery *q, DnsResourceRecord *rr, int ifindex) { _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; - DnsQuery *aux; + _cleanup_(dns_query_freep) DnsQuery *aux = NULL; int r; assert(q); @@ -1142,32 +1141,27 @@ static int resolve_service_hostname(DnsQuery *q, DnsResourceRecord *rr, int ifin aux->complete = resolve_service_hostname_complete; r = dns_query_make_auxiliary(aux, q); - if (r == -EAGAIN) { + if (r == -EAGAIN) /* Too many auxiliary lookups? If so, don't complain, * let's just not add this one, we already have more * than enough */ - - dns_query_free(aux); return 0; - } if (r < 0) - goto fail; + return r; /* Note that auxiliary queries do not track the original bus * client, only the primary request does that. */ r = dns_query_go(aux); if (r < 0) - goto fail; + return r; + TAKE_PTR(aux); return 1; - -fail: - dns_query_free(aux); - return r; } -static void bus_method_resolve_service_complete(DnsQuery *q) { +static void bus_method_resolve_service_complete(DnsQuery *query) { + _cleanup_(dns_query_freep) DnsQuery *q = query; bool has_root_domain = false; DnsResourceRecord *rr; DnsQuestion *question; @@ -1188,8 +1182,11 @@ static void bus_method_resolve_service_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -1237,7 +1234,7 @@ static void bus_method_resolve_service_complete(DnsQuery *q) { } /* Maybe we are already finished? check now... */ - resolve_service_all_complete(q); + resolve_service_all_complete(TAKE_PTR(q)); return; finish: @@ -1245,17 +1242,15 @@ finish: log_error_errno(r, "Failed to send service reply: %m"); sd_bus_reply_method_errno(q->bus_request, r, NULL); } - - dns_query_free(q); } static int bus_method_resolve_service(sd_bus_message *message, void *userdata, sd_bus_error *error) { _cleanup_(dns_question_unrefp) DnsQuestion *question_idna = NULL, *question_utf8 = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; const char *name, *type, *domain; Manager *m = userdata; int family, ifindex; uint64_t flags; - DnsQuery *q; int r; assert(message); @@ -1316,17 +1311,14 @@ static int bus_method_resolve_service(sd_bus_message *message, void *userdata, s r = dns_query_bus_track(q, message); if (r < 0) - goto fail; + return r; r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } int bus_dns_server_append(sd_bus_message *reply, DnsServer *s, bool with_ifindex, bool extended) { diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index d760213588..9e34161eb3 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -720,7 +720,8 @@ static int dns_stub_patch_bypass_reply_packet( return 0; } -static void dns_stub_query_complete(DnsQuery *q) { +static void dns_stub_query_complete(DnsQuery *query) { + _cleanup_(dns_query_freep) DnsQuery *q = query; int r; assert(q); @@ -741,7 +742,6 @@ static void dns_stub_query_complete(DnsQuery *q) { else (void) dns_stub_send(q->manager, q->stub_listener_extra, q->request_stream, q->request_packet, reply); - dns_query_free(q); return; } } @@ -753,11 +753,8 @@ static void dns_stub_query_complete(DnsQuery *q) { q, dns_query_question_for_protocol(q, DNS_PROTOCOL_DNS), dns_stub_reply_with_edns0_do(q)); - if (r < 0) { - log_debug_errno(r, "Failed to assign sections: %m"); - dns_query_free(q); - return; - } + if (r < 0) + return (void) log_debug_errno(r, "Failed to assign sections: %m"); switch (q->state) { @@ -791,11 +788,10 @@ static void dns_stub_query_complete(DnsQuery *q) { * packet doesn't answer our question. In that case let's restart the query, * now with the redirected question. We'll */ r = dns_query_go(q); - if (r < 0) { - log_debug_errno(r, "Failed to restart query: %m"); - dns_query_free(q); - } + if (r < 0) + return (void) log_debug_errno(r, "Failed to restart query: %m"); + TAKE_PTR(q); return; } @@ -803,11 +799,8 @@ static void dns_stub_query_complete(DnsQuery *q) { q, dns_query_question_for_protocol(q, DNS_PROTOCOL_DNS), dns_stub_reply_with_edns0_do(q)); - if (r < 0) { - log_debug_errno(r, "Failed to assign sections: %m"); - dns_query_free(q); - return; - } + if (r < 0) + return (void) log_debug_errno(r, "Failed to assign sections: %m"); if (cname_result == DNS_QUERY_MATCH) /* A match? Then we are done, let's return what we got */ break; @@ -853,8 +846,6 @@ static void dns_stub_query_complete(DnsQuery *q) { default: assert_not_reached(); } - - dns_query_free(q); } static int dns_stub_stream_complete(DnsStream *s, int error) { diff --git a/src/resolve/resolved-varlink.c b/src/resolve/resolved-varlink.c index cc684608a6..793be41254 100644 --- a/src/resolve/resolved-varlink.c +++ b/src/resolve/resolved-varlink.c @@ -143,9 +143,10 @@ static bool validate_and_mangle_flags( return true; } -static void vl_method_resolve_hostname_complete(DnsQuery *q) { +static void vl_method_resolve_hostname_complete(DnsQuery *query) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *canonical = NULL; _cleanup_(json_variant_unrefp) JsonVariant *array = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; _cleanup_free_ char *normalized = NULL; DnsResourceRecord *rr; DnsQuestion *question; @@ -165,8 +166,11 @@ static void vl_method_resolve_hostname_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -228,8 +232,6 @@ finish: log_error_errno(r, "Failed to send hostname reply: %m"); r = varlink_error_errno(q->varlink_request, r); } - - dns_query_free(q); } static int parse_as_address(Varlink *link, LookupParameters *p) { @@ -284,7 +286,7 @@ static int vl_method_resolve_hostname(Varlink *link, JsonVariant *parameters, Va _cleanup_(lookup_parameters_destroy) LookupParameters p = { .family = AF_UNSPEC, }; - DnsQuery *q; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; Manager *m; int r; @@ -338,13 +340,10 @@ static int vl_method_resolve_hostname(Varlink *link, JsonVariant *parameters, Va r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } static int json_dispatch_address(const char *name, JsonVariant *variant, JsonDispatchFlags flags, void *userdata) { @@ -382,8 +381,9 @@ static int json_dispatch_address(const char *name, JsonVariant *variant, JsonDis return 0; } -static void vl_method_resolve_address_complete(DnsQuery *q) { +static void vl_method_resolve_address_complete(DnsQuery *query) { _cleanup_(json_variant_unrefp) JsonVariant *array = NULL; + _cleanup_(dns_query_freep) DnsQuery *q = query; DnsQuestion *question; DnsResourceRecord *rr; int ifindex, r; @@ -402,8 +402,11 @@ static void vl_method_resolve_address_complete(DnsQuery *q) { } if (r < 0) goto finish; - if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) { + /* This was a cname, and the query was restarted. */ + TAKE_PTR(q); return; + } question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -447,8 +450,6 @@ finish: log_error_errno(r, "Failed to send address reply: %m"); r = varlink_error_errno(q->varlink_request, r); } - - dns_query_free(q); } static int vl_method_resolve_address(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata) { @@ -464,7 +465,7 @@ static int vl_method_resolve_address(Varlink *link, JsonVariant *parameters, Var _cleanup_(lookup_parameters_destroy) LookupParameters p = { .family = AF_UNSPEC, }; - DnsQuery *q; + _cleanup_(dns_query_freep) DnsQuery *q = NULL; Manager *m; int r; @@ -509,13 +510,10 @@ static int vl_method_resolve_address(Varlink *link, JsonVariant *parameters, Var r = dns_query_go(q); if (r < 0) - goto fail; + return r; + TAKE_PTR(q); return 1; - -fail: - dns_query_free(q); - return r; } int manager_varlink_init(Manager *m) { From ecdfb9a1ae0a09d22a976e2ca0dc99aacc6b9d1f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 5 Feb 2022 22:31:06 +0900 Subject: [PATCH 4/4] resolve: reuse timer event source for DnsQuery If the query get CNAME or DNAME, then the query will be restarted. Even in that case, previously, the event source was freed and allocated again. Let's slightly optimize it. --- src/resolve/resolved-dns-query.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 192bfd3bf5..9b296e7aa6 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -3,6 +3,7 @@ #include "alloc-util.h" #include "dns-domain.h" #include "dns-type.h" +#include "event-util.h" #include "hostname-util.h" #include "local-addresses.h" #include "resolved-dns-query.h" @@ -348,7 +349,7 @@ static void dns_query_stop(DnsQuery *q) { assert(q); - q->timeout_event_source = sd_event_source_disable_unref(q->timeout_event_source); + event_source_disable(q->timeout_event_source); LIST_FOREACH(candidates_by_query, c, q->candidates) dns_query_candidate_stop(c); @@ -794,17 +795,16 @@ int dns_query_go(DnsQuery *q) { dns_query_reset_answer(q); - r = sd_event_add_time_relative( + r = event_reset_time_relative( q->manager->event, &q->timeout_event_source, clock_boottime_or_monotonic(), SD_RESOLVED_QUERY_TIMEOUT_USEC, - 0, on_query_timeout, q); + 0, on_query_timeout, q, + 0, "query-timeout", true); if (r < 0) goto fail; - (void) sd_event_source_set_description(q->timeout_event_source, "query-timeout"); - q->state = DNS_TRANSACTION_PENDING; q->block_ready++;