From 810e1c03ce2b33caf88478e65d292c5d55a7f4b2 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 18 Oct 2021 16:46:38 +0200 Subject: [PATCH] MEDIUM: resolvers: use a kill list to preserve the list consistency When scanning resolution.curr it's possible to try to free some resolutions which will themselves result in freeing other ones. If one of these other ones is exactly the next one in the list, the list walk visits deleted nodes and causes memory corruption, double-frees and so on. The approach taken using the "safe" argument to some functions seems to work but it's extremely brittle as it is required to carefully check all call paths from process_ressolvers() and pass the argument to 1 there to refrain from deleting entries, so the bug is very likely to come back after some tiny changes to this code. A variant was tried, checking at various places that the current task corresponds to process_resolvers() but this is also quite brittle even though a bit less. This patch uses another approach which consists in carefully unlinking elements from the list and deferring their removal by placing it in a kill list instead of deleting them synchronously. The real benefit here is that the complexity only has to be placed where the complications are. A thread-local list is fed with elements to be deleted before scanning the resolutions, and it's flushed at the end by picking the first one until the list is empty. This way we never dereference the next element and do not care about its presence or not in the list. One function, resolv_unlink_resolution(), is exported and used outside, so it had to be modified to use this list as well. Internal code has to use _resolv_unlink_resolution() instead. (cherry picked from commit f766ec6b536725870db2ef48300c371c70c7dda0) Signed-off-by: Willy Tarreau (cherry picked from commit 2075165e20e5aa08382bf290eaa9bf08ad28f579) Signed-off-by: Christopher Faulet --- src/dns.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 104 insertions(+), 20 deletions(-) diff --git a/src/dns.c b/src/dns.c index 7ce4d32..7f45dc9 100644 --- a/src/dns.c +++ b/src/dns.c @@ -50,6 +50,7 @@ struct list dns_resolvers = LIST_HEAD_INIT(dns_resolvers); struct list dns_srvrq_list = LIST_HEAD_INIT(dns_srvrq_list); +static THREAD_LOCAL struct list death_row; /* list of deferred resolutions to kill, local validity only */ static THREAD_LOCAL uint64_t dns_query_id_seed = 0; /* random seed */ DECLARE_STATIC_POOL(dns_answer_item_pool, "dns_answer_item", sizeof(struct dns_answer_item)); @@ -58,6 +59,9 @@ DECLARE_POOL(dns_requester_pool, "dns_requester", sizeof(struct dns_requester) static unsigned int resolution_uuid = 1; unsigned int dns_failed_resolutions = 0; +static struct task *dns_process_resolvers(struct task *t, void *context, unsigned short state); +static void dns_free_resolution(struct dns_resolution *resolution); +static void _dns_unlink_resolution(struct dns_requester *requester, int safe); enum { DNS_STAT_ID, @@ -627,15 +631,51 @@ int dns_read_name(unsigned char *buffer, unsigned char *bufend, return 0; } +/* Reinitialize the list of aborted resolutions before calling certain + * functions relying on it. The list must be processed by calling + * free_aborted_resolutions() after operations. + */ +static void init_aborted_resolutions() +{ + LIST_INIT(&death_row); +} + +static void abort_resolution(struct dns_resolution *res) +{ + LIST_DEL_INIT(&res->list); + LIST_ADDQ(&death_row, &res->list); +} + +/* This releases any aborted resolution found in the death row. It is mandatory + * to call init_aborted_resolutions() first before the function (or loop) that + * needs to defer deletions. Note that some of them are in relation via internal + * objects and might cause the deletion of other ones from the same list, so we + * must absolutely not use a list_for_each_entry_safe() nor any such thing here, + * and solely rely on each call to remove the first remaining list element. + */ +static void free_aborted_resolutions() +{ + struct dns_resolution *res; + + while (!LIST_ISEMPTY(&death_row)) { + res = LIST_NEXT(&death_row, struct dns_resolution *, list); + dns_free_resolution(res); + } + + /* make sure nobody tries to add anything without having initialized it */ + death_row = (struct list){ }; +} + /* Cleanup fqdn/port and address of a server attached to a SRV resolution. This * happens when an SRV item is purged or when the server status is considered as * obsolete. * - * Must be called with the DNS lock held. + * Must be called with the DNS lock held, and with the death_row already + * initialized via init_aborted_resolutions(). */ -static void dns_srvrq_cleanup_srv(struct server *srv, int safe) +static void dns_srvrq_cleanup_srv(struct server *srv) { - dns_unlink_resolution(srv->dns_requester, safe); + _dns_unlink_resolution(srv->dns_requester, 0); HA_SPIN_LOCK(SERVER_LOCK, &srv->lock); srvrq_update_srv_status(srv, 1); free(srv->hostname); @@ -669,7 +709,9 @@ static struct task *dns_srvrq_expire_task(struct task *t, void *context, unsigne goto end; HA_SPIN_LOCK(DNS_LOCK, &srv->srvrq->resolvers->lock); - dns_srvrq_cleanup_srv(srv, 0); + init_aborted_resolutions(); + dns_srvrq_cleanup_srv(srv); + free_aborted_resolutions(); HA_SPIN_UNLOCK(DNS_LOCK, &srv->srvrq->resolvers->lock); end: @@ -677,7 +719,8 @@ static struct task *dns_srvrq_expire_task(struct task *t, void *context, unsigne } /* Checks for any obsolete record, also identify any SRV request, and try to - * find a corresponding server. + * find a corresponding server. Must be called with the death_row already + * initialized via init_aborted_resolutions(). */ static void dns_check_dns_response(struct dns_resolution *res) { @@ -710,7 +753,7 @@ static void dns_check_dns_response(struct dns_resolution *res) else if (item->type == DNS_RTYPE_SRV) { /* Remove any associated server */ list_for_each_entry_safe(srv, srvback, &item->attached_servers, srv_rec_item) - dns_srvrq_cleanup_srv(srv, 0); + dns_srvrq_cleanup_srv(srv); } LIST_DEL_INIT(&item->list); @@ -833,7 +876,7 @@ srv_found: /* Unlink A/AAAA resolution for this server if there is an AR item. * It is usless to perform an extra resolution */ - dns_unlink_resolution(srv->dns_requester, 0); + _dns_unlink_resolution(srv->dns_requester, 0); } if (!srv->hostname_dn) { @@ -1950,7 +1993,8 @@ dns_get_requester(struct dns_requester **req, enum obj_type *owner, } /* Links a requester (a server or a dns_srvrq) with a resolution. It returns 0 - * on success, -1 otherwise. + * on success, -1 otherwise. Must be called with the death_row already initialized + * via init_aborted_resolutions(). */ int dns_link_resolution(void *requester, int requester_type, int requester_locked) { @@ -2043,7 +2087,8 @@ int dns_link_resolution(void *requester, int requester_type, int requester_locke /* This function removes all server/srvrq references on answer items * if is set to 1, in case of srvrq, sub server resquesters unlink - * is called using safe == 1 to make it usable into callbacks + * is called using safe == 1 to make it usable into callbacks. Must be called + * with the death_row already initialized via init_aborted_resolutions(). */ void dns_detach_from_resolution_answer_items(struct dns_resolution *res, struct dns_requester *req, int safe) { @@ -2059,7 +2104,7 @@ void dns_detach_from_resolution_answer_items(struct dns_resolution *res, struct if (item->type == DNS_RTYPE_SRV) { list_for_each_entry_safe(srv, srvback, &item->attached_servers, srv_rec_item) { if (srv->srvrq == srvrq) - dns_srvrq_cleanup_srv(srv, safe); + dns_srvrq_cleanup_srv(srv); } } } @@ -2069,9 +2114,10 @@ void dns_detach_from_resolution_answer_items(struct dns_resolution *res, struct /* Removes a requester from a DNS resolution. It takes takes care of all the * consequences. It also cleans up some parameters from the requester. - * if is set to 1, the corresponding resolution is not released. + * if is set to 1, the corresponding resolution is not released. Must be + * called with the death_row already initialized via init_aborted_resolutions(). */ -void dns_unlink_resolution(struct dns_requester *requester, int safe) +void _dns_unlink_resolution(struct dns_requester *requester, int safe) { struct dns_resolution *res; struct dns_requester *req; @@ -2101,7 +2147,7 @@ void dns_unlink_resolution(struct dns_requester *requester, int safe) return; } - dns_free_resolution(res); + abort_resolution(res); return; } @@ -2126,12 +2172,22 @@ void dns_unlink_resolution(struct dns_requester *requester, int safe) } } +/* The public version of the function above that deals with the death row. */ +void dns_unlink_resolution(struct dns_requester *requester, int safe) +{ + init_aborted_resolutions(); + _dns_unlink_resolution(requester, safe); + free_aborted_resolutions(); +} + /* Called when a network IO is generated on a name server socket for an incoming * packet. It performs the following actions: * - check if the packet requires processing (not outdated resolution) * - ensure the DNS packet received is valid and call requester's callback * - call requester's error callback if invalid response * - check the dn_name in the packet against the one sent + * + * Must be called with the death_row already initialized via init_aborted_resolutions(). */ static void dns_resolve_recv(struct dgram_conn *dgram) { @@ -2423,10 +2479,19 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock); - /* Handle all expired resolutions from the active list */ - list_for_each_entry_safe(res, resback, &resolvers->resolutions.curr, list) { + /* Handle all expired resolutions from the active list. Elements that + * need to be removed will in fact be moved to the death_row. Other + * ones will be handled normally. + */ + + init_aborted_resolutions(); + res = LIST_NEXT(&resolvers->resolutions.curr, struct dns_resolution *, list); + while (&res->list != &resolvers->resolutions.curr) { + resback = LIST_NEXT(&res->list, struct dns_resolution *, list); + if (LIST_ISEMPTY(&res->requesters)) { - dns_free_resolution(res); + abort_resolution(res); + res = resback; continue; } @@ -2454,8 +2519,12 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne /* Clean up resolution info and remove it from the * current list */ dns_reset_resolution(res); + + /* subsequent entries might have been deleted here */ + resback = LIST_NEXT(&res->list, struct dns_resolution *, list); LIST_DEL_INIT(&res->list); LIST_ADDQ(&resolvers->resolutions.wait, &res->list); + res = resback; } else { /* Otherwise resend the DNS query and requeue the resolution */ @@ -2476,13 +2545,15 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne res->try--; } dns_send_query(res); + resback = LIST_NEXT(&res->list, struct dns_resolution *, list); + res = resback; } } /* Handle all resolutions in the wait list */ list_for_each_entry_safe(res, resback, &resolvers->resolutions.wait, list) { if (LIST_ISEMPTY(&res->requesters)) { - dns_free_resolution(res); + abort_resolution(res); continue; } @@ -2497,6 +2568,9 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne } } + /* now we can purge all queued deletions */ + free_aborted_resolutions(); + dns_update_resolvers_timeout(resolvers); HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock); return t; @@ -2517,6 +2591,7 @@ static void dns_deinit(void) struct dns_requester *req, *reqback; struct dns_srvrq *srvrq, *srvrqback; + init_aborted_resolutions(); list_for_each_entry_safe(resolvers, resolversback, &dns_resolvers, list) { list_for_each_entry_safe(ns, nsback, &resolvers->nameservers, list) { free(ns->id); @@ -2534,7 +2609,7 @@ static void dns_deinit(void) LIST_DEL_INIT(&req->list); pool_free(dns_requester_pool, req); } - dns_free_resolution(res); + abort_resolution(res); } list_for_each_entry_safe(res, resback, &resolvers->resolutions.wait, list) { @@ -2542,7 +2617,7 @@ static void dns_deinit(void) LIST_DEL_INIT(&req->list); pool_free(dns_requester_pool, req); } - dns_free_resolution(res); + abort_resolution(res); } free(resolvers->id); @@ -2558,6 +2633,8 @@ static void dns_deinit(void) LIST_DEL_INIT(&srvrq->list); free(srvrq); } + + free_aborted_resolutions(); } /* Finalizes the DNS configuration by allocating required resources and checking @@ -2570,6 +2647,8 @@ static int dns_finalize_config(void) struct proxy *px; int err_code = 0; + init_aborted_resolutions(); + /* allocate pool of resolution per resolvers */ list_for_each_entry(resolvers, &dns_resolvers, list) { struct dns_nameserver *ns; @@ -2683,8 +2762,10 @@ static int dns_finalize_config(void) if (err_code & (ERR_ALERT|ERR_ABORT)) goto err; + free_aborted_resolutions(); return err_code; err: + free_aborted_resolutions(); dns_deinit(); return err_code; @@ -2974,6 +3055,8 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px, resolvers = rule->arg.dns.resolvers; + init_aborted_resolutions(); + /* we have a response to our DNS resolution */ use_cache: if (s->dns_ctx.dns_requester && s->dns_ctx.dns_requester->resolution != NULL) { @@ -3056,6 +3139,7 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px, ret = ACT_RET_YIELD; end: + free_aborted_resolutions(); if (locked) HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock); return ret; @@ -3065,7 +3149,7 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px, s->dns_ctx.hostname_dn = NULL; s->dns_ctx.hostname_dn_len = 0; if (s->dns_ctx.dns_requester) { - dns_unlink_resolution(s->dns_ctx.dns_requester, 0); + _dns_unlink_resolution(s->dns_ctx.dns_requester, 0); pool_free(dns_requester_pool, s->dns_ctx.dns_requester); s->dns_ctx.dns_requester = NULL; } -- 1.7.10.4