From: Amaury Denoyelle <adenoyelle@haproxy.com>
Date: Tue, 20 Apr 2021 15:09:08 +0000 (+0200)
Subject: BUG/MEDIUM: server: ensure thread-safety of server runtime creation
X-Git-Tag: v2.4-dev17~40
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=cece9186251a052a17ded4891815f657b52014c6;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: server: ensure thread-safety of server runtime creation

cli_parse_add_server can be executed in parallel by several CLI
instances and so must be thread-safe. The critical points of the
function are :
- server duplicate detection
- insertion of the server in the proxy list

The mode of operation has been reversed. The server is first
instantiated and parsed. The duplicate check has been moved at the end
just before the insertion in the proxy list, under the thread isolation.
Thus, the thread safety is guaranteed and server allocation is kept
outside of locks/thread isolation.
---

diff --git a/src/server.c b/src/server.c
index 78620e019a..ac53221cea 100644
--- a/src/server.c
+++ b/src/server.c
@@ -4347,7 +4347,7 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 	if (!*sv_name)
 		return cli_err(appctx, "Require 'backend/server'.");
 
-	get_backend_server(be_name, sv_name, &be, &srv);
+	be = proxy_be_by_name(be_name);
 	if (!be)
 		return cli_err(appctx, "No such backend.");
 
@@ -4356,9 +4356,6 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		return 1;
 	}
 
-	if (srv)
-		return cli_err(appctx, "Already exists a server with the same name in backend.");
-
 	args[1] = sv_name;
 	errcode = _srv_parse_init(&srv, args, &argc, be, parse_flags, &errmsg);
 	if (errcode) {
@@ -4423,17 +4420,33 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		goto out;
 	}
 
-	/* attach the server to the end of proxy linked list
+	/* 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.
 	 *
-	 * The proxy servers list is currently not protected by a lock, so this
-	 * requires thread_isolate/release.
+	 * 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.
 	 */
 	thread_isolate();
+
 	/* TODO use a double-linked list for px->srv */
 	if (be->srv) {
-		struct server *next;
-		for (next = be->srv; next->next; next = next->next)
-			;
+		struct server *next = be->srv;
+
+		while (1) {
+			/* check for duplicate server */
+			if (!strcmp(srv->id, next->id)) {
+				thread_release();
+				cli_err(appctx, "Already exists a server with the same name in backend.");
+				goto out;
+			}
+
+			if (!next->next)
+				break;
+
+			next = next->next;
+		}
 
 		next->next = srv;
 	}
@@ -4441,6 +4454,7 @@ static int cli_parse_add_server(char **args, char *payload, struct appctx *appct
 		srv->next = be->srv;
 		be->srv = srv;
 	}
+
 	thread_release();
 
 	cli_msg(appctx, LOG_INFO, "New server registered.");