BUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks

Another way to say it: "Safely unlink requester from a requester callbacks".

Requester callbacks must never try to unlink a requester from a resolution, for
the current requester or another one. First, these callback functions are called
in a loop on a request list, not necessarily safe. Thus unlink resolution at
this place, may be unsafe. And it is useless to try to make these loops safe
because, all this stuff is placed in a loop on a resolution list. Unlink a
requester may lead to release a resolution if it is the last requester.

However, the unkink is necessary because we cannot reset the server state
(hostname and IP) with some pending DNS resolution on it. So, to workaround
this issue, we introduce the "safe" unlink. It is only performed from a
requester callback. In this case, the unlink function never releases the
resolution, it only reset it if necessary. And when a resolution is found
with an empty requester list, it is released.

This patch depends on the following commits :

 * MINOR: resolvers: Purge answer items when a SRV resolution triggers an error
 * MINOR: resolvers: Use a function to remove answers attached to a resolution
 * MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
 * MINOR: resolvers: Add function to change the srv status based on SRV resolution

All the series must be backported as far as 2.2. It fixes a regression
introduced by the commit b4badf720 ("BUG/MINOR: resolvers: new callback to
properly handle SRV record errors").

don't release resolution from requester cb

diff --git a/include/haproxy/resolvers.h b/include/haproxy/resolvers.h
index 34bc0fbf474572631b9b8dc56580abfa85459f4a..baf43d5b009aa8d47ff4609fc79a2b7a25edbf56 100644 (file)
--- a/include/haproxy/resolvers.h
+++ b/include/haproxy/resolvers.h
@@ -45,7 +45,7 @@ int resolv_get_ip_from_response(struct resolv_response *r_res,
 
 void resolv_purge_resolution_answer_records(struct resolv_resolution *resolution);
 int resolv_link_resolution(void *requester, int requester_type, int requester_locked);
-void resolv_unlink_resolution(struct resolv_requester *requester);
+void resolv_unlink_resolution(struct resolv_requester *requester, int safe);
 void resolv_trigger_resolution(struct resolv_requester *requester);
 enum act_parse_ret resolv_parse_do_resolve(const char **args, int *orig_arg, struct proxy *px, struct act_rule *rule, char **err);
 int check_action_do_resolve(struct act_rule *rule, struct proxy *px, char **err);

diff --git a/src/resolvers.c b/src/resolvers.c
index 4844d7f3f26d6ed00ca38ca20e95246dbb43a642..dd4efc8b963528190142c85539d62990fbdbf3a2 100644 (file)
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -605,7 +605,7 @@ static void resolv_check_response(struct resolv_resolution *res)
                                        if (srv->srvrq == srvrq && srv->svc_port == item->port &&
                                            item->data_len == srv->hostname_dn_len &&
                                            !resolv_hostname_cmp(srv->hostname_dn, item->target, item->data_len)) {
-                                               resolv_unlink_resolution(srv->resolv_requester);
+                                               resolv_unlink_resolution(srv->resolv_requester, 0);
                                                srvrq_update_srv_status(srv, 1);
                                                ha_free(&srv->hostname);
                                                ha_free(&srv->hostname_dn);
@@ -684,7 +684,7 @@ static void resolv_check_response(struct resolv_resolution *res)
                                        /* Unlink A/AAAA resolution for this server if there is an AR item.
                                         * It is usless to perform an extra resolution
                                         */
-                                       resolv_unlink_resolution(srv->resolv_requester);
+                                       resolv_unlink_resolution(srv->resolv_requester, 0);
                                }
 
                                if (!srv->hostname_dn) {
@@ -1825,8 +1825,9 @@ int resolv_link_resolution(void *requester, int requester_type, int requester_lo
 
 /* 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 <safe> is set to 1, the corresponding resolution is not released.
  */
-void resolv_unlink_resolution(struct resolv_requester *requester)
+void resolv_unlink_resolution(struct resolv_requester *requester, int safe)
 {
        struct resolv_resolution *res;
        struct resolv_requester  *req;
@@ -1844,6 +1845,15 @@ void resolv_unlink_resolution(struct resolv_requester *requester)
        if (!LIST_ISEMPTY(&res->requesters))
                req = LIST_NEXT(&res->requesters, struct resolv_requester *, list);
        else {
+               if (safe) {
+                       /* Don't release it yet. */
+                       resolv_reset_resolution(res);
+                       res->hostname_dn = NULL;
+                       res->hostname_dn_len = 0;
+                       resolv_purge_resolution_answer_records(res);
+                       return;
+               }
+
                resolv_free_resolution(res);
                return;
        }
@@ -2070,6 +2080,11 @@ static struct task *process_resolvers(struct task *t, void *context, unsigned in
 
        /* Handle all expired resolutions from the active list */
        list_for_each_entry_safe(res, resback, &resolvers->resolutions.curr, list) {
+               if (LIST_ISEMPTY(&res->requesters)) {
+                       resolv_free_resolution(res);
+                       continue;
+               }
+
                /* When we find the first resolution in the future, then we can
                 * stop here */
                exp = tick_add(res->last_query, resolvers->timeout.retry);
@@ -2118,6 +2133,11 @@ static struct task *process_resolvers(struct task *t, void *context, unsigned in
 
        /* Handle all resolutions in the wait list */
        list_for_each_entry_safe(res, resback, &resolvers->resolutions.wait, list) {
+               if (LIST_ISEMPTY(&res->requesters)) {
+                       resolv_free_resolution(res);
+                       continue;
+               }
+
                exp = tick_add(res->last_resolution, resolv_resolution_timeout(res));
                if (tick_isset(res->last_resolution) && !tick_is_expired(exp, now_ms))
                        continue;
@@ -2676,7 +2696,7 @@ enum act_return resolv_action_do_resolve(struct act_rule *rule, struct proxy *px
        ha_free(&s->resolv_ctx.hostname_dn);
        s->resolv_ctx.hostname_dn_len = 0;
        if (s->resolv_ctx.requester) {
-               resolv_unlink_resolution(s->resolv_ctx.requester);
+               resolv_unlink_resolution(s->resolv_ctx.requester, 0);
                pool_free(resolv_requester_pool, s->resolv_ctx.requester);
                s->resolv_ctx.requester = NULL;
        }

diff --git a/src/server.c b/src/server.c
index b631a228202a7a63993c7724e1671f5a4637db70..be820e36d5640228a10432dea0401d156f1d5bfd 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -3294,7 +3294,7 @@ int srvrq_resolution_error_cb(struct resolv_requester *requester, int error_code
        for (s = srvrq->proxy->srv; s != NULL; s = s->next) {
                HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
                if (s->srvrq == srvrq) {
-                       resolv_unlink_resolution(s->resolv_requester);
+                       resolv_unlink_resolution(s->resolv_requester, 1);
                        srvrq_update_srv_status(s, 1);
                        free(s->hostname);
                        free(s->hostname_dn);
@@ -3448,7 +3448,7 @@ int srv_set_fqdn(struct server *srv, const char *hostname, int resolv_locked)
            strcmp(resolution->hostname_dn, hostname_dn) == 0)
                goto end;
 
-       resolv_unlink_resolution(srv->resolv_requester);
+       resolv_unlink_resolution(srv->resolv_requester, 0);
 
        free(srv->hostname);
        free(srv->hostname_dn);

diff --git a/src/stream.c b/src/stream.c
index c27ea539c0f86389ac883fdc255b829f02b00816..54c6a77bf59f7032cd4ea67b380fa90ad19f2b55 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -674,7 +674,7 @@ static void stream_free(struct stream *s)
                HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
                ha_free(&s->resolv_ctx.hostname_dn);
                s->resolv_ctx.hostname_dn_len = 0;
-               resolv_unlink_resolution(s->resolv_ctx.requester);
+               resolv_unlink_resolution(s->resolv_ctx.requester, 0);
                HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
 
                pool_free(resolv_requester_pool, s->resolv_ctx.requester);