fix signal races when master process is used

In the absence of a pselect/ppoll-like version of waitpid;
we must use a selfwake descriptor (pipe or eventfd) to
wake the master up whenever a signal is received.

So wait on the selfwake descriptor and always run waitpid
with WNOHANG in a loop to ensure all children are reaped.

The: mog_intr_disable(); waitpid(); mog_intr_enable()
sequence was completely stupid I can't believe I wrote it.

4 files changed, 63 insertions, 34 deletions

diff --git a/cmogstored.c b/cmogstored.c
index 1421afb..adad681 100644
--- a/cmogstored.c
+++ b/cmogstored.c
@@ -11,6 +11,7 @@ _Noreturn void cmogstored_exit(void);
 static char summary[] = THIS " -- "DESC;
 const char *argp_program_bug_address = PACKAGE_BUGREPORT;
 const char *argp_program_version = THIS" "PACKAGE_VERSION;
+static struct mog_fd *master_selfwake;
 static sig_atomic_t sigchld_hit;
 static sig_atomic_t do_exit;
 static sig_atomic_t do_upgrade;
@@ -333,12 +334,14 @@ static void wakeup_noop(int signum)
 static void master_wakeup_handler(int signum)
 {
         switch (signum) {
+        case SIGCHLD: sigchld_hit = 1; break;
         case SIGUSR2: do_upgrade = 1; break;
         case SIGQUIT:
         case SIGTERM:
         case SIGINT:
                 do_exit = signum;
         }
+        mog_selfwake_trigger(master_selfwake);
 }
 
 static void siginit(void (*wakeup_handler)(int))
@@ -372,16 +375,22 @@ static void process_died(pid_t pid, int status);
 
 static void sigchld_handler(void)
 {
-        int status;
-        pid_t pid = waitpid(-1, &status, WNOHANG);
-
-        if (pid > 0)
-                process_died(pid, status);
-        if (pid < 0) {
-                switch (errno) {
-                case ECHILD: sigchld_hit = 0; break;
-                case EINTR: return;
-                default: die_errno("waitpid");
+        sigchld_hit = 0;
+
+        for (;;) {
+                int status;
+                pid_t pid = waitpid(-1, &status, WNOHANG);
+
+                if (pid > 0) {
+                        process_died(pid, status);
+                } else if (pid == 0) {
+                        return;
+                } else {
+                        switch (errno) {
+                        case EINTR: sigchld_hit = 1; return; /* retry later */
+                        case ECHILD: return;
+                        default: die_errno("waitpid");
+                        }
                 }
         }
 }
@@ -404,14 +413,14 @@ static void main_worker_loop(const pid_t parent)
         mog_cancel_disable(); /* mog_idleq_wait() now relies on this */
         while (parent == 0 || parent == getppid()) {
                 mog_notify_wait(have_mgmt);
+                if (sigchld_hit)
+                        sigchld_handler();
                 if (do_upgrade)
                         upgrade_handler();
                 if (do_exit)
                         cmogstored_exit();
                 if (have_mgmt)
                         mog_mnt_refresh();
-                if (sigchld_hit)
-                        sigchld_handler();
                 else if (have_mgmt && !iostat_running && !do_exit)
                         /*
                          * maybe iostat was not installed/available/usable at
@@ -521,37 +530,23 @@ static void process_died(pid_t pid, int status)
 static void run_master(void)
 {
         unsigned id;
+        size_t running = worker_processes;
 
+        master_selfwake = mog_selfwake_new();
         siginit(master_wakeup_handler);
 
         for (id = 0; id < worker_processes; id++)
                 fork_worker(id);
 
-        while (! do_exit || mog_kill_each_worker(SIGQUIT) > 0) {
-                int status = 0;
-                pid_t pid;
-
-                if (do_upgrade)
-                        upgrade_handler();
-                mog_intr_enable();
-                pid = waitpid(-1, &status, 0);
-                PRESERVE_ERRNO( mog_intr_disable() );
-
-                if (pid > 0)
-                        process_died(pid, status);
-                if (pid < 0) {
-                        switch (errno) {
-                        case EINTR: break;
-                        case ECHILD: return;
-                        default: syslog(LOG_WARNING, "waitpid failed: %m");
-                        }
-                }
+        while (running > 0) {
+                mog_selfwake_wait(master_selfwake);
+                if (sigchld_hit)
+                        sigchld_handler();
                 if (do_upgrade)
                         upgrade_handler();
+                if (do_exit)
+                        running = mog_kill_each_worker(SIGQUIT);
         }
-        /* upgrade on our way out */
-        if (do_upgrade)
-                upgrade_handler();
 }
 
 int main(int argc, char *argv[], char *envp[])
diff --git a/notify.h b/notify.h
index 0e0eb53..1888fda 100644
--- a/notify.h
+++ b/notify.h
@@ -24,6 +24,8 @@ struct mog_selfpipe {
 
 extern struct mog_queue *mog_notify_queue;
 
+/* selfwake*.[ch] */
 struct mog_fd * mog_selfwake_new(void);
 void mog_selfwake_trigger(struct mog_fd *);
 void mog_selfwake_drain(struct mog_fd *);
+void mog_selfwake_wait(struct mog_fd *);
diff --git a/selfwake_eventfd.h b/selfwake_eventfd.h
index baca39f..836d037 100644
--- a/selfwake_eventfd.h
+++ b/selfwake_eventfd.h
@@ -5,6 +5,8 @@
 #if defined(HAVE_SYS_EVENTFD_H) && defined(HAVE_EVENTFD)
 #include <sys/eventfd.h>
 #if defined(EFD_CLOEXEC) && defined(EFD_NONBLOCK)
+#include <sys/poll.h>
+
 static ssize_t selfwake_write(struct mog_fd *mfd)
 {
         static const uint64_t buf = 1;
@@ -32,5 +34,21 @@ static ssize_t selfwake_drain(struct mog_fd *mfd)
 
         return read(mfd->fd, &buf, sizeof(buf));
 }
+
+/* this allows interrupts */
+void mog_selfwake_wait(struct mog_fd *mfd)
+{
+        sigset_t set;
+        struct pollfd pfd = { .fd = mfd->fd, .events = POLLIN };
+        int r;
+
+        CHECK(int, 0, sigemptyset(&set));
+
+        r = ppoll(&pfd, 1, NULL, &set);
+        if ((r < 0) && (errno != ENOMEM))
+                assert(errno == EINTR && "BUG in using ppoll");
+
+        mog_selfwake_drain(mfd);
+}
 #endif /* EFD_CLOEXEC+EFD_NONBLOCK */
 #endif /* HAVE_SYS_EVENTFD_H  */
diff --git a/selfwake_pipe.h b/selfwake_pipe.h
index 763332a..f61d755 100644
--- a/selfwake_pipe.h
+++ b/selfwake_pipe.h
@@ -32,4 +32,18 @@ static ssize_t selfwake_drain(struct mog_fd *mfd)
         char buf[64];
         return read(mfd->fd, buf, sizeof(buf));
 }
+
+/* this allows interrupts */
+void mog_selfwake_wait(struct mog_fd *mfd)
+{
+        ssize_t r;
+
+        /* poll() on a pipe does not work on some *BSDs, so just block */
+        mog_set_nonblocking(mfd->fd, false);
+        mog_intr_enable();
+        r = selfwake_drain(mfd);
+        mog_intr_disable();
+        mog_set_nonblocking(mfd->fd, true);
+        mog_selfwake_drain(mfd);
+}
 #endif /* !HAVE_EVENTFD */