From 7f8f6cb926f777bc49b24bd783879b288dc390ee Mon Sep 17 00:00:00 2001
From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 10 Nov 2020 14:24:31 +0100
Subject: [PATCH] BUG/MEDIUM: stats: prevent crash if counters not alloc with
 dummy one

Define a per-thread counters allocated with the greatest size of any
stat module counters. This variable is named trash_counters.

When using a proxy without allocated counters, return the trash counters
from EXTRA_COUNTERS_GET instead of a dangling pointer to prevent
segfault.

This is useful for all the proxies used internally and not
belonging to the global proxy list. As these objects does not appears on
the stat report, it does not matter to use the dummy counters.

For this fix to be functional, the extra counters are explicitly
initialized to NULL on proxy/server/listener init functions.

Most notably, the crash has already been detected with the following
vtc:
- reg-tests/lua/txn_get_priv.vtc
- reg-tests/peers/tls_basic_sync.vtc
- reg-tests/peers/tls_basic_sync_wo_stkt_backend.vtc
There is probably other parts that may be impacted (SPOE for example).

This bug was introduced in the current release and do not need to be
backported. The faulty commits are
"MINOR: ssl: count client hello for stats" and
"MINOR: ssl: add counters for ssl sessions".
---
 include/haproxy/stats-t.h |  6 +++++-
 src/listener.c            |  2 ++
 src/proxy.c               |  3 +++
 src/server.c              |  2 ++
 src/stats.c               | 38 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/haproxy/stats-t.h b/include/haproxy/stats-t.h
index af9b9711d7..70d8b489ae 100644
--- a/include/haproxy/stats-t.h
+++ b/include/haproxy/stats-t.h
@@ -509,11 +509,15 @@ enum stats_domain_px_cap {
 	STATS_PX_CAP_MASK = 0xff
 };
 
+extern THREAD_LOCAL void *trash_counters;
+
 #define EXTRA_COUNTERS(name) \
 	struct extra_counters *name
 
 #define EXTRA_COUNTERS_GET(counters, mod) \
-	(void *)((counters)->data + (mod)->counters_off[(counters)->type])
+	(likely(counters) ? \
+		((void *)((counters)->data + (mod)->counters_off[(counters)->type])) : \
+		(trash_counters))
 
 #define EXTRA_COUNTERS_REGISTER(counters, ctype, alloc_failed_label) \
 	do {                                                         \
diff --git a/src/listener.c b/src/listener.c
index 7f038c4a3d..c9f0c2de6a 100644
--- a/src/listener.c
+++ b/src/listener.c
@@ -623,6 +623,8 @@ int create_listeners(struct bind_conf *bc, const struct sockaddr_storage *ss,
 		if (fd != -1)
 			l->rx.flags |= RX_F_INHERITED;
 
+		l->extra_counters = NULL;
+
 		HA_SPIN_INIT(&l->lock);
 		_HA_ATOMIC_ADD(&jobs, 1);
 		_HA_ATOMIC_ADD(&listeners, 1);
diff --git a/src/proxy.c b/src/proxy.c
index e469c76714..08140198f4 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1041,6 +1041,9 @@ void init_new_proxy(struct proxy *p)
 	/* Default to only allow L4 retries */
 	p->retry_type = PR_RE_CONN_FAILED;
 
+	p->extra_counters_fe = NULL;
+	p->extra_counters_be = NULL;
+
 	HA_RWLOCK_INIT(&p->lock);
 }
 
diff --git a/src/server.c b/src/server.c
index 1e9f46ec72..d72e7e069c 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1742,6 +1742,8 @@ struct server *new_server(struct proxy *proxy)
 	srv->agent.proxy = proxy;
 	srv->xprt  = srv->check.xprt = srv->agent.xprt = xprt_get(XPRT_RAW);
 
+	srv->extra_counters = NULL;
+
 	/* please don't put default server settings here, they are set in
 	 * init_default_instance().
 	 */
diff --git a/src/stats.c b/src/stats.c
index ad92d71593..3f314440de 100644
--- a/src/stats.c
+++ b/src/stats.c
@@ -269,6 +269,8 @@ static struct list stats_module_list[STATS_DOMAIN_COUNT] = {
 	LIST_HEAD_INIT(stats_module_list[STATS_DOMAIN_DNS]),
 };
 
+THREAD_LOCAL void *trash_counters;
+
 static inline uint8_t stats_get_domain(uint32_t domain)
 {
 	return domain >> STATS_DOMAIN & STATS_DOMAIN_MASK;
@@ -4548,6 +4550,34 @@ static int allocate_stats_dns_postcheck(void)
 
 REGISTER_CONFIG_POSTPARSER("allocate-stats-dns", allocate_stats_dns_postcheck);
 
+static int allocate_trash_counters(void)
+{
+	struct stats_module *mod;
+	int domains[] = { STATS_DOMAIN_PROXY, STATS_DOMAIN_DNS }, i;
+	size_t max_counters_size = 0;
+
+	/* calculate the greatest counters used by any stats modules */
+	for (i = 0; i < STATS_DOMAIN_COUNT; ++i) {
+		list_for_each_entry(mod, &stats_module_list[domains[i]], list) {
+			max_counters_size = mod->counters_size > max_counters_size ?
+			                    mod->counters_size : max_counters_size;
+		}
+	}
+
+	/* allocate the trash with the size of the greatest counters */
+	if (max_counters_size) {
+		trash_counters = malloc(max_counters_size);
+		if (!trash_counters) {
+			ha_alert("stats: cannot allocate trash counters for statistics\n");
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
+REGISTER_PER_THREAD_ALLOC(allocate_trash_counters);
+
 static void deinit_stats(void)
 {
 	int domains[] = { STATS_DOMAIN_PROXY, STATS_DOMAIN_DNS }, i;
@@ -4565,6 +4595,14 @@ static void deinit_stats(void)
 
 REGISTER_POST_DEINIT(deinit_stats);
 
+static void free_trash_counters(void)
+{
+	if (trash_counters)
+		free(trash_counters);
+}
+
+REGISTER_PER_THREAD_FREE(free_trash_counters);
+
 /* register cli keywords */
 static struct cli_kw_list cli_kws = {{ },{
 	{ { "clear", "counters",  NULL }, "clear counters : clear max statistics counters (add 'all' for all counters)", cli_parse_clear_counters, NULL, NULL },
-- 
2.47.3