From 47d84679d6fe71f56d3a013578007dff92ff72db Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Thu, 22 May 2014 02:02:39 +0200 Subject: Fix waitpid race condition Doing a waitpid for all processes in the SIGCHLD handler could sometimes steal a signal from a fastd_shell_command_exec_sync call. To fix this, don't reap the children in the SIGCHLD handler anymore, but create a reaper thread for each asynchronous shell command. --- src/fastd.c | 1 - src/shell.c | 57 +++++++++++++++++++++++++++++++++++---------------------- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/fastd.c b/src/fastd.c index 0d487bd..016e5f1 100644 --- a/src/fastd.c +++ b/src/fastd.c @@ -78,7 +78,6 @@ static void on_sigusr1(int signo UNUSED) { } static void on_sigchld(int signo UNUSED) { - while (waitpid(-1, NULL, WNOHANG) > 0) {} } static void init_signals(void) { diff --git a/src/shell.c b/src/shell.c index 9d03fbc..8542bf9 100644 --- a/src/shell.c +++ b/src/shell.c @@ -29,6 +29,7 @@ #include #include +#include typedef struct shell_env_entry { @@ -103,18 +104,15 @@ static void shell_command_setenv(pid_t pid, const fastd_shell_env_t *env) { } } -static bool shell_command_do_exec(const fastd_shell_command_t *command, const fastd_shell_env_t *env, pid_t *pid_ret) { +static bool shell_command_do_exec(const fastd_shell_command_t *command, const fastd_shell_env_t *env, pid_t *pid) { pid_t parent = getpid(); - pid_t pid = fork(); - if (pid < 0) { + *pid = fork(); + if (*pid < 0) { pr_error_errno("shell_command_do_exec: fork"); return false; } - else if (pid > 0) { - if (pid_ret) - *pid_ret = pid; - + else if (*pid > 0) { return true; } @@ -127,12 +125,6 @@ static bool shell_command_do_exec(const fastd_shell_command_t *command, const fa shell_command_setenv(parent, env); - /* unblock SIGCHLD */ - sigset_t set; - sigemptyset(&set); - sigaddset(&set, SIGCHLD); - pthread_sigmask(SIG_UNBLOCK, &set, NULL); - execl("/bin/sh", "sh", "-c", command->command, (char*)NULL); _exit(127); } @@ -141,12 +133,6 @@ bool fastd_shell_command_exec_sync(const fastd_shell_command_t *command, const f if (!fastd_shell_command_isset(command)) return true; - /* block SIGCHLD */ - sigset_t set, oldset; - sigemptyset(&set); - sigaddset(&set, SIGCHLD); - pthread_sigmask(SIG_BLOCK, &set, &oldset); - pid_t pid; if (!shell_command_do_exec(command, env, &pid)) return false; @@ -154,8 +140,6 @@ bool fastd_shell_command_exec_sync(const fastd_shell_command_t *command, const f int status; pid_t err = waitpid(pid, &status, 0); - pthread_sigmask(SIG_SETMASK, &oldset, NULL); - if (err <= 0) { pr_error_errno("fastd_shell_command_exec_sync: waitpid"); return false; @@ -174,6 +158,35 @@ bool fastd_shell_command_exec_sync(const fastd_shell_command_t *command, const f return true; } +static void * reap_child(void *arg) { + pid_t *pid = arg; + + pid_t err = waitpid(*pid, NULL, 0); + + pr_debug("child process %u finished", (unsigned)*pid); + + if (err <= 0) + pr_error_errno("reap_child: waitpid"); + + free(pid); + + return NULL; +} + +static void shell_command_exec_async(const fastd_shell_command_t *command, const fastd_shell_env_t *env) { + pid_t *pid = malloc(sizeof(*pid)); + if (!shell_command_do_exec(command, env, pid)) + return; + + pthread_t thread; + if ((errno = pthread_create(&thread, NULL, reap_child, pid)) != 0) { + pr_error_errno("unable to create reaper thread"); + free(pid); + return; + } + + pthread_detach(thread); +} void fastd_shell_command_exec(const fastd_shell_command_t *command, const fastd_shell_env_t *env) { if (!fastd_shell_command_isset(command)) @@ -182,5 +195,5 @@ void fastd_shell_command_exec(const fastd_shell_command_t *command, const fastd_ if (command->sync) fastd_shell_command_exec_sync(command, env, NULL); else - shell_command_do_exec(command, env, NULL); + shell_command_exec_async(command, env); } -- cgit v1.2.3