From 0ce0301d8184fcebb1aafda396d6a9ae71bf1301 Mon Sep 17 00:00:00 2001 From: Andre Noll Date: Sat, 12 Aug 2017 23:27:55 +0200 Subject: [PATCH] server: Let stat command handler perform cleanup on signals. The stat command is special because it is the only command which may run for a very long time, and because it catches SIGUSR1 which is sent by the server to notify the command handler process when the status items have changed and the command handler should update the client. The stat command handler catches neither SIGINT nor SIGTERM, though, so these signals terminate the process. This happens when para_server is started without --daemon and CTRL+C is pressed (causing SIGINT to be sent to the all processes in the foreground process group) or when the server is about to exit, because in this case the main process sends SIGTERM to all child processes. If the command handler process terminates because it received SIGINT or SIGTERM, valgrind reports various memory leaks. Although the leaks are harmless, they clutter the valgrind output and make real memory leaks hard to spot. This commit changes com_stat() to ignore SIGINT and catch SIGTERM in addition to SIGUSR1. We don't need to care about SIGINT in com_stat() because in the above scenario, the main process sends SIGTERM to all command handlers after it received SIGINT. With the patch applied, if com_stat() received SIGTERM, it returns and the normal cleanup on exit is performed. We have to employ pselect(2) to avoid race conditions. --- command.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/command.c b/command.c index 84b7d943..a72a524f 100644 --- a/command.c +++ b/command.c @@ -49,9 +49,14 @@ static const char * const server_command_perms_txt[] = {LSG_SERVER_CMD_AUX_INFOS extern int mmd_mutex; extern struct misc_meta_data *mmd; int send_afs_status(struct command_context *cc, int parser_friendly); +static bool subcmd_should_die; -static void dummy(__a_unused int s) +static void command_handler_sighandler(int s) { + if (s != SIGTERM) + return; + PARA_EMERG_LOG("terminating on signal %d\n", SIGTERM); + subcmd_should_die = true; } /* @@ -492,9 +497,21 @@ static int com_stat(struct command_context *cc, struct lls_parse_result *lpr) bool parser_friendly = SERVER_CMD_OPT_GIVEN(STAT, PARSER_FRIENDLY, lpr) > 0; uint32_t num = SERVER_CMD_UINT32_VAL(STAT, NUM, lpr); + const struct timespec ts = {.tv_sec = 50, .tv_nsec = 0}; - para_sigaction(SIGUSR1, dummy); + para_sigaction(SIGINT, SIG_IGN); + para_sigaction(SIGUSR1, command_handler_sighandler); + para_sigaction(SIGTERM, command_handler_sighandler); + /* + * Simply checking subcmd_should_die is racy because a signal may + * arrive after the check but before the subsequent call to sleep(3). + * If this happens, sleep(3) would not be interrupted by the signal. + * To avoid this we block SIGTERM here and allow it to arrive only + * while we sleep. + */ + para_block_signal(SIGTERM); for (;;) { + sigset_t set; /* * Copy the mmd structure to minimize the time we hold the mmd * lock. @@ -517,7 +534,15 @@ static int com_stat(struct command_context *cc, struct lls_parse_result *lpr) ret = 1; if (num > 0 && !--num) goto out; - sleep(50); + sigemptyset(&set); /* empty set means: unblock all signals */ + /* + * pselect(2) allows to atomically unblock signals, then go to + * sleep. Calling sigprocmask(2) followed by sleep(3) would + * open a race window similar to the one described above. + */ + pselect(1, NULL, NULL, NULL, &ts, &set); + if (subcmd_should_die) + goto out; ret = -E_SERVER_CRASH; if (getppid() == 1) goto out; -- 2.30.2