Merge ~rafaeldtinoco/ubuntu/+source/haproxy:lp1894879-focal into ubuntu/+source/haproxy:ubuntu/focal-devel

[Rafael David Tinoco]

BUG: https://bugs.launchpad.net/ubuntu/+source/haproxy/+bug/1894879

[Rafael David Tinoco (rafaeldtinoco)]

$ git push pkg upload/2.0.13-2ubuntu0.1
Counting objects: 17, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (17/17), done.
Writing objects: 100% (17/17), 6.47 KiB | 602.00 KiB/s, done.
Total 17 (delta 9), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/haproxy
 * [new tag] upload/2.0.13-2ubuntu0.1 -> upload/2.0.13-2ubuntu0.1

$ debdiff *.dsc 2>&1 | diffstat -l | grep -v gpg
changelog
control
patches/lp1894879-BUG-CRITICAL-dns-Make-the-do-resolve-action-thread-safe.patch
patches/lp1894879-BUG-MEDIUM-dns-Dont-yield-in-do-resolve-action-on-a-final.patch
patches/lp1894879-BUG-MEDIUM-dns-Release-answer-items-when-a-DNS-resolution-is-freed.patch
patches/series

$ dput ubuntu haproxy_2.0.13-2ubuntu0.1_source.changes
Checking signature on .changes
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading haproxy_2.0.13-2ubuntu0.1.dsc: done.
  Uploading haproxy_2.0.13-2ubuntu0.1.debian.tar.xz: done.
  Uploading haproxy_2.0.13-2ubuntu0.1_source.buildinfo: done.
  Uploading haproxy_2.0.13-2ubuntu0.1_source.changes: done.
Successfully uploaded packages.

diff --git a/debian/changelog b/debian/changelog
index 5c9fe8b..35cd7fb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+haproxy (2.0.13-2ubuntu0.1) focal; urgency=medium
+
+  * Backport dns related fixes from git to resolve crashes when
+    using do-resolve action (LP: #1894879)
+    - BUG/CRITICAL: dns: Make the do-resolve action thread safe
+    - BUG/MEDIUM: dns: Release answer items when a DNS resolution is freed
+    - BUG/MEDIUM: dns: Don't yield in do resolve action on a final
+
+ -- Simon Deziel <[e-mail]>  Tue, 08 Sep 2020 17:16:14 +0000
+
 haproxy (2.0.13-2) unstable; urgency=medium
 
   * d/dconv: replace cgi.escape by html.escape. Closes: #951416.
diff --git a/debian/control b/debian/control
index cc08457..05a48d5 100644
--- a/debian/control
+++ b/debian/control
@@ -1,7 +1,8 @@
 Source: haproxy
 Section: net
 Priority: optional
-Maintainer: Debian HAProxy Maintainers <[e-mail]>
+Maintainer: Ubuntu Developers <[e-mail]>
+XSBC-Original-Maintainer: Debian HAProxy Maintainers <[e-mail]>
 Uploaders: Apollon Oikonomopoulos <[e-mail]>,
            Prach Pongpanich <[e-mail]>,
            Vincent Bernat <[e-mail]>
diff --git a/debian/patches/lp1894879-BUG-CRITICAL-dns-Make-the-do-resolve-action-thread-safe.patch b/debian/patches/lp1894879-BUG-CRITICAL-dns-Make-the-do-resolve-action-thread-safe.patch
new file mode 100644
index 0000000..e9e6edc
--- /dev/null
+++ b/debian/patches/lp1894879-BUG-CRITICAL-dns-Make-the-do-resolve-action-thread-safe.patch
@@ -0,0 +1,132 @@
+From ef131aee357478c45d547abcb0ab21c2a191f578 Mon Sep 17 00:00:00 2001
+From: Christopher Faulet <[e-mail]>
+Date: Wed, 22 Jul 2020 11:46:32 +0200
+Subject: [PATCH] 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 <[e-mail]>
+(cherry picked from commit 99f4623952cbbad2bcae451abdd0f3133bcbe75c)
+Signed-off-by: Christopher Faulet <[e-mail]>
+(cherry picked from commit 6e5861d72fe1e3c9d34b591d6f77ffd28ddde197)
+Signed-off-by: Christopher Faulet <[e-mail]>
+---
+ src/dns.c    |   36 +++++++++++++++++++++++++++---------
+ src/stream.c |    4 ++++
+ 2 files changed, 31 insertions(+), 9 deletions(-)
+
+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 @@ 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. */
+@@ -2211,29 +2220,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_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 @@ 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;
+ }
+ 
+ 
+diff --git a/src/stream.c b/src/stream.c
+index 080189e..2eb7cfa 100644
+--- a/src/stream.c
++++ b/src/stream.c
+@@ -435,9 +435,13 @@ static void stream_free(struct stream *s)
+ 	}
+ 
+ 	if (s->dns_ctx.dns_requester) {
++		__decl_hathreads(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;
+-- 
+1.7.10.4
+
diff --git a/debian/patches/lp1894879-BUG-MEDIUM-dns-Dont-yield-in-do-resolve-action-on-a-final.patch b/debian/patches/lp1894879-BUG-MEDIUM-dns-Dont-yield-in-do-resolve-action-on-a-final.patch
new file mode 100644
index 0000000..34a6a37
--- /dev/null
+++ b/debian/patches/lp1894879-BUG-MEDIUM-dns-Dont-yield-in-do-resolve-action-on-a-final.patch
@@ -0,0 +1,106 @@
+From 74d704f2f36945d60f1ff7ea75dbfe3f40508861 Mon Sep 17 00:00:00 2001
+From: Christopher Faulet <[e-mail]>
+Date: Tue, 28 Jul 2020 10:21:54 +0200
+Subject: [PATCH] BUG/MEDIUM: dns: Don't yield in do-resolve action on a final
+ evaluation
+
+When an action is evaluated, flags are passed to know if it is the first call
+(ACT_OPT_FIRST) and if it must be the last one (ACT_OPT_FINAL). For the
+do-resolve DNS action, the ACT_OPT_FINAL flag must be handled because the
+action may yield. It must never yield when this flag is set. Otherwise, it may
+lead to a wakeup loop of the stream because the inspected-delay of a tcp-request
+content ruleset was reached without stopping the rules evaluation.
+
+This patch is related to the issue #222. It must be backported as far as 2.0.
+
+(cherry picked from commit 385101e53816dc1b7bc1fc957adc512ce8a07cb4)
+Signed-off-by: Christopher Faulet <[e-mail]>
+(cherry picked from commit 5c038f759959adf95b4b347aba9d97e60ab87e93)
+Signed-off-by: Christopher Faulet <[e-mail]>
+(cherry picked from commit af018c4865400dda4553a732df4c43751a4ff88c)
+Signed-off-by: Christopher Faulet <[e-mail]>
+---
+ src/dns.c |   39 +++++++++++++++++++++------------------
+ 1 file changed, 21 insertions(+), 18 deletions(-)
+
+diff --git a/src/dns.c b/src/dns.c
+index 18e64d9..095040e 100644
+--- a/src/dns.c
++++ b/src/dns.c
+@@ -2182,10 +2182,8 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
+ 			locked = 1;
+ 		}
+ 
+-		if (resolution->step == RSLV_STEP_RUNNING) {
+-			ret = ACT_RET_YIELD;
+-			goto end;
+-		}
++		if (resolution->step == RSLV_STEP_RUNNING)
++			goto yield;
+ 		if (resolution->step == RSLV_STEP_NONE) {
+ 			/* We update the variable only if we have a valid response. */
+ 			if (resolution->status == RSLV_STATUS_VALID) {
+@@ -2219,14 +2217,7 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
+ 			}
+ 		}
+ 
+-		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);
+-
+-		pool_free(dns_requester_pool, s->dns_ctx.dns_requester);
+-		s->dns_ctx.dns_requester = NULL;
+-
+-		goto end;
++		goto release_requester;
+ 	}
+ 
+ 	/* need to configure and start a new DNS resolution */
+@@ -2247,26 +2238,38 @@ enum act_return dns_action_do_resolve(struct act_rule *rule, struct proxy *px,
+ 
+ 	/* 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);
+-		ret = ACT_RET_YIELD;
+-		goto end;
+-	}
++	if (!req || !req->resolution)
++		goto release_requester; /* on error, ignore the action */
+ 	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)
+-		       && !tick_is_expired(exp, now_ms)) {
++	    && !tick_is_expired(exp, now_ms)) {
+ 		goto use_cache;
+ 	}
+ 
+ 	dns_trigger_resolution(s->dns_ctx.dns_requester);
++
++  yield:
++	if (flags & ACT_FLAG_FINAL)
++		goto release_requester;
+ 	ret = ACT_RET_YIELD;
+ 
+   end:
+ 	if (locked)
+ 		HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
+ 	return ret;
++
++  release_requester:
++	free(s->dns_ctx.hostname_dn);
++	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);
++		pool_free(dns_requester_pool, s->dns_ctx.dns_requester);
++		s->dns_ctx.dns_requester = NULL;
++	}
++	goto end;
+ }
+ 
+ 
+-- 
+1.7.10.4
+
diff --git a/debian/patches/lp1894879-BUG-MEDIUM-dns-Release-answer-items-when-a-DNS-resolution-is-freed.patch b/debian/patches/lp1894879-BUG-MEDIUM-dns-Release-answer-items-when-a-DNS-resolution-is-freed.patch
new file mode 100644
index 0000000..f83de45
--- /dev/null
+++ b/debian/patches/lp1894879-BUG-MEDIUM-dns-Release-answer-items-when-a-DNS-resolution-is-freed.patch
@@ -0,0 +1,51 @@
+From 39eb766825d8aad09946dfc284d4a73f610ebd64 Mon Sep 17 00:00:00 2001
+From: Christopher Faulet <[e-mail]>
+Date: Wed, 22 Jul 2020 15:55:49 +0200
+Subject: [PATCH] BUG/MEDIUM: dns: Release answer items when a DNS resolution
+ is freed
+
+When a DNS resolution is freed, the remaining items in .ar_list and .answer_list
+are also released. It must be done to avoid a memory leak. And it is the last
+chance to release these objects. I've honestly no idea if there is a better
+place to release them earlier. But at least, there is no more leak.
+
+This patch should solve the issue #222. It must be backported, at least, as far
+as 2.0, and probably, with caution, as far as 1.8 or 1.7.
+
+(cherry picked from commit 010ab35a9118daf17a670fb2b42e40447f967f7c)
+Signed-off-by: Willy Tarreau <[e-mail]>
+(cherry picked from commit c58ac80d00284886b108b209a5bf993de5ab38ed)
+Signed-off-by: Christopher Faulet <[e-mail]>
+(cherry picked from commit 81120e6ea286ae3f2566959167fb56a7d1f0de19)
+Signed-off-by: Christopher Faulet <[e-mail]>
+---
+ src/dns.c |    6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/src/dns.c b/src/dns.c
+index 40e29ad..18e64d9 100644
+--- a/src/dns.c
++++ b/src/dns.c
+@@ -1336,6 +1336,7 @@ static struct dns_resolution *dns_pick_resolution(struct dns_resolvers *resolver
+ static void dns_free_resolution(struct dns_resolution *resolution)
+ {
+ 	struct dns_requester *req, *reqback;
++	struct dns_answer_item *item, *itemback;
+ 
+ 	/* clean up configuration */
+ 	dns_reset_resolution(resolution);
+@@ -1347,6 +1348,11 @@ static void dns_free_resolution(struct dns_resolution *resolution)
+ 		req->resolution = NULL;
+ 	}
+ 
++	list_for_each_entry_safe(item, itemback, &resolution->response.answer_list, list) {
++		LIST_DEL(&item->list);
++		pool_free(dns_answer_item_pool, item);
++	}
++
+ 	LIST_DEL(&resolution->list);
+ 	pool_free(dns_resolution_pool, resolution);
+ }
+-- 
+1.7.10.4
+
diff --git a/debian/patches/series b/debian/patches/series
index b57f722..c8fe75d 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -7,3 +7,7 @@ haproxy.service-add-documentation.patch
 
 # applied during the build process:
 # debianize-dconv.patch
+
+lp1894879-BUG-CRITICAL-dns-Make-the-do-resolve-action-thread-safe.patch
+lp1894879-BUG-MEDIUM-dns-Release-answer-items-when-a-DNS-resolution-is-freed.patch
+lp1894879-BUG-MEDIUM-dns-Dont-yield-in-do-resolve-action-on-a-final.patch