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

diff --git a/command.c b/command.c
index 7b3d6faf90849a92ecbafb98538f277cfef5b02a..0c483befe3adfd8899756c99feed7c30b003850d 100644 (file)
--- 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 32d6ab6624e5f493ac393b630a603c0968f11497..9b328caf6550da385eb64c824217da7f3abd4588 100644 (file)
--- 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);
 }