From 0139e0a7511581c43fb86ada8d675ac636d1b082 Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Tue, 15 Jun 2021 12:01:29 +0200 Subject: [PATCH] BUG/MEDIUM: server/cli: Fix ABBA deadlock when fqdn is set from the CLI To perform servers resolution, the resolver's lock is first acquired then the server's lock when necessary. However, when the fqdn is set via the CLI, the opposite is performed. So, it is possible to experience an ABBA deadlock. To fix this bug, the server's lock is acquired and released for each subcommand of "set server" with an exception when the fqdn is set. The resolver's lock is first acquired. Of course, this means we must be sure to have a resolver to lock. This patch must be backported as far as 1.8. (cherry picked from commit c7b391aed21f384ac38b9f2f98239c5f1cdd1895) Signed-off-by: Christopher Faulet (cherry picked from commit 77df12ca694b168326c29efa2580a95427ad644e) Signed-off-by: Christopher Faulet --- reg-tests/server/cli_set_fdqn.vtc | 2 +- src/server.c | 40 +++++++++++++++++++++++++++---------- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/reg-tests/server/cli_set_fdqn.vtc b/reg-tests/server/cli_set_fdqn.vtc index b3b00c3..7dd97ec 100644 --- a/reg-tests/server/cli_set_fdqn.vtc +++ b/reg-tests/server/cli_set_fdqn.vtc @@ -28,7 +28,7 @@ haproxy h1 -conf { haproxy h1 -cli { send "set server test/www1 fqdn foo.fqdn" - expect ~ "could not update test/www1 FQDN by 'stats socket command'" + expect ~ "set server / fqdn failed because no resolution is configured." send "show servers state test" expect ~ "test 1 www1 ${s1_addr} .* - ${s1_port}" } -wait diff --git a/src/server.c b/src/server.c index eca38ad..5507d20 100644 --- a/src/server.c +++ b/src/server.c @@ -4420,14 +4420,15 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct if (!sv) return 1; - HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); - if (strcmp(args[3], "weight") == 0) { + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); warning = server_parse_weight_change_request(sv, args[4]); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); if (warning) cli_err(appctx, warning); } else if (strcmp(args[3], "state") == 0) { + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); if (strcmp(args[4], "ready") == 0) srv_adm_set_ready(sv); else if (strcmp(args[4], "drain") == 0) @@ -4436,8 +4437,10 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct srv_adm_set_maint(sv); else cli_err(appctx, "'set server state' expects 'ready', 'drain' and 'maint'.\n"); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else if (strcmp(args[3], "health") == 0) { + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); if (sv->track) cli_err(appctx, "cannot change health on a tracking server.\n"); else if (strcmp(args[4], "up") == 0) { @@ -4454,8 +4457,10 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct } else cli_err(appctx, "'set server health' expects 'up', 'stopping', or 'down'.\n"); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else if (strcmp(args[3], "agent") == 0) { + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); if (!(sv->agent.state & CHK_ST_ENABLED)) cli_err(appctx, "agent checks are not enabled on this server.\n"); else if (strcmp(args[4], "up") == 0) { @@ -4468,12 +4473,15 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct } else cli_err(appctx, "'set server agent' expects 'up' or 'down'.\n"); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else if (strcmp(args[3], "agent-addr") == 0) { + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); if (!(sv->agent.state & CHK_ST_ENABLED)) cli_err(appctx, "agent checks are not enabled on this server.\n"); else if (str2ip(args[4], &sv->agent.addr) == NULL) cli_err(appctx, "incorrect addr address given for agent.\n"); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else if (strcmp(args[3], "agent-send") == 0) { if (!(sv->agent.state & CHK_ST_ENABLED)) @@ -4487,18 +4495,21 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct int i = 0; if (strl2irc(args[4], strlen(args[4]), &i) != 0) { cli_err(appctx, "'set server check-port' expects an integer as argument.\n"); - goto out_unlock; + goto out; } if ((i < 0) || (i > 65535)) { cli_err(appctx, "provided port is not valid.\n"); - goto out_unlock; + goto out; } + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); /* prevent the update of port to 0 if MAPPORTS are in use */ if ((sv->flags & SRV_F_MAPPORTS) && (i == 0)) { cli_err(appctx, "can't unset 'port' since MAPPORTS is in use.\n"); - goto out_unlock; + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); + goto out; } sv->check.port = i; + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); cli_msg(appctx, LOG_NOTICE, "health check port updated.\n"); } else if (strcmp(args[3], "addr") == 0) { @@ -4506,7 +4517,7 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct char *port = NULL; if (strlen(args[4]) == 0) { cli_err(appctx, "set server / addr requires an address and optionally a port.\n"); - goto out_unlock; + goto out; } else { addr = args[4]; @@ -4514,21 +4525,31 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct if (strcmp(args[5], "port") == 0) { port = args[6]; } + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); warning = update_server_addr_port(sv, addr, port, "stats socket command"); if (warning) cli_msg(appctx, LOG_WARNING, warning); srv_clr_admin_flag(sv, SRV_ADMF_RMAINT); + HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); } else if (strcmp(args[3], "fqdn") == 0) { if (!*args[4]) { cli_err(appctx, "set server / fqdn requires a FQDN.\n"); - goto out_unlock; + goto out; + } + if (!sv->resolvers) { + cli_err(appctx, "set server / fqdn failed because no resolution is configured.\n"); + goto out; } + HA_SPIN_LOCK(DNS_LOCK, &sv->resolvers->lock); + HA_SPIN_LOCK(SERVER_LOCK, &sv->lock); /* ensure runtime resolver will process this new fqdn */ if (sv->flags & SRV_F_NO_RESOLUTION) { sv->flags &= ~SRV_F_NO_RESOLUTION; } - warning = update_server_fqdn(sv, args[4], "stats socket command", 0); + warning = update_server_fqdn(sv, args[4], "stats socket command", 1); + HA_SPIN_UNLOCK(SERVER_UNLOCK, &sv->lock); + HA_SPIN_UNLOCK(DNS_LOCK, &sv->resolvers->lock); if (warning) cli_msg(appctx, LOG_WARNING, warning); } @@ -4537,8 +4558,7 @@ static int cli_parse_set_server(char **args, char *payload, struct appctx *appct "'set server ' only supports 'agent', 'health', 'state'," " 'weight', 'addr', 'fqdn' and 'check-port'.\n"); } - out_unlock: - HA_SPIN_UNLOCK(SERVER_LOCK, &sv->lock); + out: return 1; } -- 1.7.10.4