From 10596c5a763716368579b6313953ae0861b03ad4 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Wed, 9 Nov 2022 20:20:26 +0100 Subject: [PATCH] server: Avoid deadlock in daemon_log(). Currently both the generic signal handler in signal.c and the signal handler for the stat command handler in command.c call daemon_log() via PARA_EMERG_LOG(). This is problematic because daemon_log() takes the log mutex and the signal might arrive while daemon_log() is executing. If this race condition is hit, the process deadlocks because daemon_log() tries to acquire a mutex which it already holds. All three types of server processes (main, afs and command handler) are susceptible to this bug, but regardless of which process happens to hit the race window, the server process hangs waiting on the mutex, and no longer accepts connections. Fix this by removing the problematic log call in the generic case and by printing it out of interrupt context in the command handler case. This bug was introduced together with the log mutex five years ago. Fixes: ced0c17d1a3ee0336dc7b35e69faff131dabecac --- command.c | 14 +++++++++----- signal.c | 11 +++++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/command.c b/command.c index 7b3d6faf..0c483bef 100644 --- a/command.c +++ b/command.c @@ -52,12 +52,14 @@ extern struct misc_meta_data *mmd; int send_afs_status(struct command_context *cc, int parser_friendly); static bool subcmd_should_die; +/* + * Don't call PARA_XXX_LOG() here as we might already hold the log mutex. See + * generic_signal_handler() for details. + */ static void command_handler_sighandler(int s) { - if (s != SIGTERM) - return; - PARA_EMERG_LOG("terminating on signal %d\n", SIGTERM); - subcmd_should_die = true; + if (s == SIGTERM) + subcmd_should_die = true; } /* @@ -543,8 +545,10 @@ static int com_stat(struct command_context *cc, struct lls_parse_result *lpr) * open a race window similar to the one described above. */ pselect(1, NULL, NULL, NULL, &ts, &set); - if (subcmd_should_die) + if (subcmd_should_die) { + PARA_EMERG_LOG("terminating on SIGTERM\n"); goto out; + } ret = -E_SERVER_CRASH; if (getppid() == 1) goto out; diff --git a/signal.c b/signal.c index 32d6ab66..9b328caf 100644 --- a/signal.c +++ b/signal.c @@ -72,10 +72,13 @@ static void generic_signal_handler(int s) errno = save_errno; return; } - if (ret < 0) - PARA_EMERG_LOG("%s\n", strerror(errno)); - else - PARA_EMERG_LOG("short write to signal pipe\n"); + /* + * This is a fatal error which should never happen. We must not call + * PARA_XXX_LOG() here because this might acquire the log mutex which + * is already taken by the main program if the interrupt occurs while a + * log message is being printed. The mutex will not be released as long + * as this signal handler is running, so a deadlock ensues. + */ exit(EXIT_FAILURE); } -- 2.39.2