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.
(cherry picked from commit 5098a08c2fafb0d9513996729d2a30c9785378f3)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 99f4623952cbbad2bcae451abdd0f3133bcbe75c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6e5861d72fe1e3c9d34b591d6f77ffd28ddde197)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/src/dns.c b/src/dns.c
index 282fa92..40e29ad 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -2162,14 +2162,23 @@
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. */
@@ -2211,29 +2220,33 @@
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_ERR;
+ 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)
@@ -2242,7 +2255,12 @@
}
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;
}