BUG/MAJOR: resolvers: add other missing references during resolution removal

There is a fundamental design bug in the resolvers code which is that
a list of active resolutions is being walked to try to delete outdated
entries, and that the code responsible for removing them also removes
other elements, including the next one which will be visited by the
list iterator. This randomly causes a use-after-free condition leading
to crashes, infinite loops and various other issues such as random memory
corruption.

A first fix for the memory fix for this was brought by commit 0efc0993e
("BUG/MEDIUM: resolvers: Don't release resolution from a requester
callbacks"). While preparing for more fixes, some code was factored by
commit 11c6c3965 ("MINOR: resolvers: Clean server in a dedicated function
when removing a SRV item"), which inadvertently passed "0" as the "safe"
argument all the time, missing one case of removal protection, instead
of always using "safe". This patch reintroduces the correct argument.

This must be backported with all fixes above.

Cc: Christopher Faulet <cfaulet@haproxy.com>

diff --git a/src/resolvers.c b/src/resolvers.c
index ae1eaeefadf75af54577732533d58e645b12bb1c..44305fc408e88c94baf310d54226e5f396be4193 100644 (file)
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -564,9 +564,9 @@ int resolv_read_name(unsigned char *buffer, unsigned char *bufend,
  *
  * Must be called with the DNS lock held.
  */
-static void resolv_srvrq_cleanup_srv(struct server *srv)
+static void resolv_srvrq_cleanup_srv(struct server *srv, int safe)
 {
-       resolv_unlink_resolution(srv->resolv_requester, 0);
+       resolv_unlink_resolution(srv->resolv_requester, safe);
        HA_SPIN_LOCK(SERVER_LOCK, &srv->lock);
        srvrq_update_srv_status(srv, 1);
        ha_free(&srv->hostname);
@@ -597,7 +597,7 @@ static struct task *resolv_srvrq_expire_task(struct task *t, void *context, unsi
                goto end;
 
        HA_SPIN_LOCK(DNS_LOCK, &srv->srvrq->resolvers->lock);
-       resolv_srvrq_cleanup_srv(srv);
+       resolv_srvrq_cleanup_srv(srv, 0);
        HA_SPIN_UNLOCK(DNS_LOCK, &srv->srvrq->resolvers->lock);
 
   end:
@@ -638,7 +638,7 @@ static void resolv_check_response(struct resolv_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)
-                                       resolv_srvrq_cleanup_srv(srv);
+                                       resolv_srvrq_cleanup_srv(srv, 0);
                        }
 
                        LIST_DELETE(&item->list);
@@ -1944,7 +1944,7 @@ void resolv_detach_from_resolution_answer_items(struct resolv_resolution *res,
                        if (item->type == DNS_RTYPE_SRV) {
                                list_for_each_entry_safe(srv, srvback, &item->attached_servers, srv_rec_item) {
                                        if (srv->srvrq == srvrq)
-                                               resolv_srvrq_cleanup_srv(srv);
+                                               resolv_srvrq_cleanup_srv(srv, safe);
                                }
                        }
                }