BUG/MAJOR: dns: Make the do-resolve action thread-safe

The do-resolve HTTP action, performing a DNS resolution of a sample expression
output, is not thread-safe at all. The resolver object used to do the resolution
must be locked when the action is executed or when the stream is released
because its curr or wait resolution lists and the requester list inside a
resolution are updated. It is also important to not wake up a released stream
(with a destroyed task).

Of course, because of this bug, various kind of crashes may be observed.

This patch should fix the issue #236. It must be backported as far as 2.0.

diff --git a/src/dns.c b/src/dns.c
index 740e066ef15552ef2352204882816bdbeab1cd8e..b38f58bed14d57da7565e0b6a2d593a8d8f873fa 100644 (file)
--- a/src/dns.c
+++ b/src/dns.c
@@ -2396,14 +2396,23 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
        struct dns_requester *req;
        struct dns_resolvers  *resolvers;
        struct dns_resolution *res;
-       int exp;
+       int exp, locked = 0;
+       enum act_return ret = ACT_RET_CONT;
+
+       resolvers = rule->arg.dns.resolvers;
 
        /* we have a response to our DNS resolution */
  use_cache:
        if (s->dns_ctx.dns_requester && s->dns_ctx.dns_requester->resolution != NULL) {
                resolution = s->dns_ctx.dns_requester->resolution;
+               if (!locked) {
+                       HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
+                       locked = 1;
+               }
+
                if (resolution->step == RSLV_STEP_RUNNING) {
-                       return ACT_RET_YIELD;
+                       ret = ACT_RET_YIELD;
+                       goto end;
                }
                if (resolution->step == RSLV_STEP_NONE) {
                        /* We update the variable only if we have a valid response. */
@@ -2445,29 +2454,33 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
                pool_free(dns_requester_pool, s->dns_ctx.dns_requester);
                s->dns_ctx.dns_requester = NULL;
 
-               return ACT_RET_CONT;
+               goto end;
        }
 
        /* need to configure and start a new DNS resolution */
        smp = sample_fetch_as_type(px, sess, s, SMP_OPT_DIR_REQ|SMP_OPT_FINAL, rule->arg.dns.expr, SMP_T_STR);
        if (smp == NULL)
-               return ACT_RET_CONT;
+               goto end;
 
        fqdn = smp->data.u.str.area;
        if (action_prepare_for_resolution(s, fqdn) == -1)
-               return ACT_RET_CONT; /* on error, ignore the action */
+               goto end; /* on error, ignore the action */
 
        s->dns_ctx.parent = rule;
+
+       HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
+       locked = 1;
+
        dns_link_resolution(s, OBJ_TYPE_STREAM, 0);
 
        /* Check if there is a fresh enough response in the cache of our associated resolution */
        req = s->dns_ctx.dns_requester;
        if (!req || !req->resolution) {
                dns_trigger_resolution(s->dns_ctx.dns_requester);
-               return ACT_RET_YIELD;
+               ret = ACT_RET_YIELD;
+               goto end;
        }
-       res       = req->resolution;
-       resolvers = res->resolvers;
+       res = req->resolution;
 
        exp = tick_add(res->last_resolution, resolvers->hold.valid);
        if (resolvers->t && res->status == RSLV_STATUS_VALID && tick_isset(res->last_resolution)
@@ -2476,7 +2489,12 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
        }
 
        dns_trigger_resolution(s->dns_ctx.dns_requester);
-       return ACT_RET_YIELD;
+       ret = ACT_RET_YIELD;
+
+  end:
+       if (locked)
+               HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
+       return ret;
 }
 
 static void release_dns_action(struct act_rule *rule)

diff --git a/src/stream.c b/src/stream.c
index 287f40647c1faa256ab96e46d6bdb966fec7b4c4..c8078f5ad8c54bf81d435fc0af15d309499f08ca 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -627,9 +627,13 @@ static void stream_free(struct stream *s)
        }
 
        if (s->dns_ctx.dns_requester) {
+               __decl_thread(struct dns_resolvers *resolvers = s->dns_ctx.parent->arg.dns.resolvers);
+
+               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);
+               HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
 
                pool_free(dns_requester_pool, s->dns_ctx.dns_requester);
                s->dns_ctx.dns_requester = NULL;