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

(cherry picked from commit 0efc0993ec5210c8cf5a98eb9c0220014e030fd6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 119ec5279df7241da25f2ffc1e43de4075717f35)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f721fd209cb7f7126a548bf5301f4dd008952184)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

diff --git a/include/proto/dns.h b/include/proto/dns.h
index 19159d6..bbe0d2e 100644 (file)
--- a/include/proto/dns.h
+++ b/include/proto/dns.h
@@ -44,7 +44,7 @@ int dns_get_ip_from_response(struct dns_response_packet *dns_p,
 
 void dns_purge_resolution_answer_records(struct dns_resolution *resolution);
 int dns_link_resolution(void *requester, int requester_type, int requester_locked);
-void dns_unlink_resolution(struct dns_requester *requester);
+void dns_unlink_resolution(struct dns_requester *requester, int safe);
 void dns_trigger_resolution(struct dns_requester *requester);
 enum act_parse_ret dns_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/dns.c b/src/dns.c
index 17d3307..1963b89 100644 (file)
--- a/src/dns.c
+++ b/src/dns.c
@@ -519,7 +519,7 @@ static void dns_check_dns_response(struct dns_resolution *res)
                                        if (srv->srvrq == srvrq && srv->svc_port == item->port &&
                                            item->data_len == srv->hostname_dn_len &&
                                            !dns_hostname_cmp(srv->hostname_dn, item->target, item->data_len)) {
-                                               dns_unlink_resolution(srv->dns_requester);
+                                               dns_unlink_resolution(srv->dns_requester, 0);
                                                srvrq_update_srv_status(srv, 1);
                                                free(srv->hostname);
                                                free(srv->hostname_dn);
@@ -1492,8 +1492,9 @@ int dns_link_resolution(void *requester, int requester_type, int requester_locke
 
 /* Removes a requester from a DNS resoltion. 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 dns_unlink_resolution(struct dns_requester *requester)
+void dns_unlink_resolution(struct dns_requester *requester, int safe)
 {
        struct dns_resolution *res;
        struct dns_requester  *req;
@@ -1511,6 +1512,15 @@ void dns_unlink_resolution(struct dns_requester *requester)
        if (!LIST_ISEMPTY(&res->requesters))
                req = LIST_NEXT(&res->requesters, struct dns_requester *, list);
        else {
+               if (safe) {
+                       /* Don't release it yet. */
+                       dns_reset_resolution(res);
+                       res->hostname_dn = NULL;
+                       res->hostname_dn_len = 0;
+                       dns_purge_resolution_answer_records(res);
+                       return;
+               }
+
                dns_free_resolution(res);
                return;
        }
@@ -1811,6 +1821,11 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne
 
        /* Handle all expired resolutions from the active list */
        list_for_each_entry_safe(res, resback, &resolvers->resolutions.curr, list) {
+               if (LIST_ISEMPTY(&res->requesters)) {
+                       dns_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);
@@ -1859,6 +1874,11 @@ static struct task *dns_process_resolvers(struct task *t, void *context, unsigne
 
        /* 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);
+                       continue;
+               }
+
                exp = tick_add(res->last_resolution, dns_resolution_timeout(res));
                if (tick_isset(res->last_resolution) && !tick_is_expired(exp, now_ms))
                        continue;
@@ -2284,7 +2304,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);
+               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;
        }

diff --git a/src/server.c b/src/server.c
index 840c764..0555fed 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -4253,7 +4253,7 @@ int srvrq_resolution_error_cb(struct dns_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) {
-                       dns_unlink_resolution(s->dns_requester);
+                       dns_unlink_resolution(s->dns_requester, 1);
                        srvrq_update_srv_status(s, 1);
                        free(s->hostname);
                        free(s->hostname_dn);
@@ -4408,7 +4408,7 @@ int srv_set_fqdn(struct server *srv, const char *hostname, int dns_locked)
            strcasecmp(resolution->hostname_dn, hostname_dn) == 0)
                goto end;
 
-       dns_unlink_resolution(srv->dns_requester);
+       dns_unlink_resolution(srv->dns_requester, 0);
 
        free(srv->hostname);
        free(srv->hostname_dn);

diff --git a/src/stream.c b/src/stream.c
index 54d7222..0ab3d17 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -637,7 +637,7 @@ static void stream_free(struct stream *s)
                HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
                free(s->dns_ctx.hostname_dn); s->dns_ctx.hostname_dn = NULL;
                s->dns_ctx.hostname_dn_len = 0;
-               dns_unlink_resolution(s->dns_ctx.dns_requester);
+               dns_unlink_resolution(s->dns_ctx.dns_requester, 0);
                HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
 
                pool_free(dns_requester_pool, s->dns_ctx.dns_requester);