From 31ddd76fef720c35c22acc39016feea5604e5b39 Mon Sep 17 00:00:00 2001
From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Thu, 10 Jun 2021 15:26:44 +0200
Subject: [PATCH] BUG/MEDIUM: server: extend thread-isolate over much of CLI
 'add server'

Some config parsing handlers were designed to be run at startup on a
single-thread. When executing at runtime for dynamic servers,
thread-safety is not guaranteed. This is the case for example in
srv_parse_id which manipulates backend used_ids tree.

One solution could be to add locks but it might be tricky to found all
affected functions and it can be an easy source of deadlock. The other
solution which has been chosen is to use thread-isolation over almost
all of the cli_parse_add_server CLI handler.

For now this solution is sufficient. If some users make heavy use of the
'add server', hurting the overall performance, it will be necessary to
design a much thinner solution.

This must be backported up to 2.4.
---
 src/server.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/server.c b/src/server.c
index 57b3feecb..14f54857d 100644
--- a/src/server.c
+++ b/src/server.c
@@ -4349,6 +4349,14 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		return 1;
 	}
 
+	/* At this point, some operations might not be thread-safe anymore. This
+	 * might be the case for parsing handlers which were designed to run
+	 * only at the starting stage on single-thread mode.
+	 *
+	 * Activate thread isolation to ensure thread-safety.
+	 */
+	thread_isolate();
+
 	args[1] = sv_name;
 	errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags);
 	if (errcode)
@@ -4412,15 +4420,12 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		goto out;
 	}
 
-	/* Attach the server to the end of the proxy linked list. The proxy
-	 * servers list is currently not protected by a lock, so this requires
-	 * thread_isolate/release.
+	/* Attach the server to the end of the proxy linked list. Note that this
+	 * operation is not thread-safe so this is executed under thread
+	 * isolation.
 	 *
-	 * If a server with the same name is found, reject the new one. This
-	 * operation requires thread-safety and thus cannot be executed at the
-	 * beginning without having server allocation under locks/isolation.
+	 * If a server with the same name is found, reject the new one.
 	 */
-	thread_isolate();
 
 	/* TODO use a double-linked list for px->srv */
 	if (be->srv) {
@@ -4429,7 +4434,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		while (1) {
 			/* check for duplicate server */
 			if (!strcmp(srv->id, next->id)) {
-				thread_release();
 				ha_alert("Already exists a server with the same name in backend.\n");
 				goto out;
 			}
@@ -4455,6 +4459,8 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 	return 0;
 
 out:
+	thread_release();
+
 	if (!usermsgs_empty())
 		cli_err(appctx, usermsgs_str());