util: fix minimal race where we might miss SIGTERMs when forking off an agent

[elogind.git] / src / shared / util.c

diff --git a/src/shared/util.c b/src/shared/util.c
index 2bb3b5ef1c1c2d22b84f02e629ba9dda3f099ba4..cf9d487b829b67087691a43da7959d0637020a10 100644 (file)
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -175,25 +175,24 @@ char* first_word(const char *s, const char *word) {
 }
 
 int close_nointr(int fd) {
-        int r;
-
         assert(fd >= 0);
-        r = close(fd);
-        if (r >= 0)
-                return r;
-        else if (errno == EINTR)
-                /*
-                 * Just ignore EINTR; a retry loop is the wrong
-                 * thing to do on Linux.
-                 *
-                 * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
-                 * https://bugzilla.gnome.org/show_bug.cgi?id=682819
-                 * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
-                 * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
-                 */
+
+        if (close(fd) >= 0)
                 return 0;
-        else
-                return -errno;
+
+        /*
+         * Just ignore EINTR; a retry loop is the wrong thing to do on
+         * Linux.
+         *
+         * http://lkml.indiana.edu/hypermail/linux/kernel/0509.1/0877.html
+         * https://bugzilla.gnome.org/show_bug.cgi?id=682819
+         * http://utcc.utoronto.ca/~cks/space/blog/unix/CloseEINTR
+         * https://sites.google.com/site/michaelsafyan/software-engineering/checkforeintrwheninvokingclosethinkagain
+         */
+        if (errno == EINTR)
+                return 0;
+
+        return -errno;
 }
 
 int safe_close(int fd) {
@@ -938,7 +937,7 @@ int readlink_and_canonicalize(const char *p, char **r) {
 }
 
 int reset_all_signal_handlers(void) {
-        int sig;
+        int sig, r = 0;
 
         for (sig = 1; sig < _NSIG; sig++) {
                 struct sigaction sa = {
@@ -946,16 +945,29 @@ int reset_all_signal_handlers(void) {
                         .sa_flags = SA_RESTART,
                 };
 
+                /* These two cannot be caught... */
                 if (sig == SIGKILL || sig == SIGSTOP)
                         continue;
 
                 /* On Linux the first two RT signals are reserved by
                  * glibc, and sigaction() will return EINVAL for them. */
                 if ((sigaction(sig, &sa, NULL) < 0))
-                        if (errno != EINVAL)
-                                return -errno;
+                        if (errno != EINVAL && r == 0)
+                                r = -errno;
         }
 
+        return r;
+}
+
+int reset_signal_mask(void) {
+        sigset_t ss;
+
+        if (sigemptyset(&ss) < 0)
+                return -errno;
+
+        if (sigprocmask(SIG_SETMASK, &ss, NULL) < 0)
+                return -errno;
+
         return 0;
 }
 
@@ -2592,7 +2604,7 @@ bool hostname_is_set(void) {
         return !isempty(u.nodename) && !streq(u.nodename, "(none)");
 }
 
-static char *lookup_uid(uid_t uid) {
+char *lookup_uid(uid_t uid) {
         long bufsize;
         char *name;
         _cleanup_free_ char *buf = NULL;
@@ -3878,16 +3890,13 @@ void execute_directory(const char *directory, DIR *d, usec_t timeout, char *argv
                 _cleanup_hashmap_free_free_ Hashmap *pids = NULL;
                 _cleanup_closedir_ DIR *_d = NULL;
                 struct dirent *de;
-                sigset_t ss;
 
                 /* We fork this all off from a child process so that
                  * we can somewhat cleanly make use of SIGALRM to set
                  * a time limit */
 
                 reset_all_signal_handlers();
-
-                assert_se(sigemptyset(&ss) == 0);
-                assert_se(sigprocmask(SIG_SETMASK, &ss, NULL) == 0);
+                reset_signal_mask();
 
                 assert_se(prctl(PR_SET_PDEATHSIG, SIGTERM) == 0);
 
@@ -3942,7 +3951,6 @@ void execute_directory(const char *directory, DIR *d, usec_t timeout, char *argv
                                 _exit(EXIT_FAILURE);
                         }
 
-
                         log_debug("Spawned %s as " PID_FMT ".", path, pid);
 
                         r = hashmap_put(pids, UINT_TO_PTR(pid), path);
@@ -5094,9 +5102,9 @@ int fd_inc_rcvbuf(int fd, size_t n) {
 }
 
 int fork_agent(pid_t *pid, const int except[], unsigned n_except, const char *path, ...) {
-        pid_t parent_pid, agent_pid;
-        int fd;
         bool stdout_is_tty, stderr_is_tty;
+        pid_t parent_pid, agent_pid;
+        sigset_t ss, saved_ss;
         unsigned n, i;
         va_list ap;
         char **l;
@@ -5104,16 +5112,25 @@ int fork_agent(pid_t *pid, const int except[], unsigned n_except, const char *pa
         assert(pid);
         assert(path);
 
-        parent_pid = getpid();
-
         /* Spawns a temporary TTY agent, making sure it goes away when
          * we go away */
 
+        parent_pid = getpid();
+
+        /* First we temporarily block all signals, so that the new
+         * child has them blocked initially. This way, we can be sure
+         * that SIGTERMs are not lost we might send to the agent. */
+        assert_se(sigfillset(&ss) >= 0);
+        assert_se(sigprocmask(SIG_SETMASK, &ss, &saved_ss) >= 0);
+
         agent_pid = fork();
-        if (agent_pid < 0)
+        if (agent_pid < 0) {
+                assert_se(sigprocmask(SIG_SETMASK, &saved_ss, NULL) >= 0);
                 return -errno;
+        }
 
         if (agent_pid != 0) {
+                assert_se(sigprocmask(SIG_SETMASK, &saved_ss, NULL) >= 0);
                 *pid = agent_pid;
                 return 0;
         }
@@ -5124,8 +5141,14 @@ int fork_agent(pid_t *pid, const int except[], unsigned n_except, const char *pa
         if (prctl(PR_SET_PDEATHSIG, SIGTERM) < 0)
                 _exit(EXIT_FAILURE);
 
+        /* Make sure we actually can kill the agent, if we need to, in
+         * case somebody invoked us from a shell script that trapped
+         * SIGTERM or so... */
+        reset_all_signal_handlers();
+        reset_signal_mask();
+
         /* Check whether our parent died before we were able
-         * to set the death signal */
+         * to set the death signal and unblock the signals */
         if (getppid() != parent_pid)
                 _exit(EXIT_SUCCESS);
 
@@ -5136,6 +5159,8 @@ int fork_agent(pid_t *pid, const int except[], unsigned n_except, const char *pa
         stderr_is_tty = isatty(STDERR_FILENO);
 
         if (!stdout_is_tty || !stderr_is_tty) {
+                int fd;
+
                 /* Detach from stdout/stderr. and reopen
                  * /dev/tty for them. This is important to
                  * ensure that when systemctl is started via
@@ -7114,10 +7139,10 @@ int unquote_many_words(const char **p, ...) {
                 if (r < 0) {
                         int j;
 
-                        for (j = 0; j < c; j++) {
+                        for (j = 0; j < c; j++)
                                 free(l[j]);
-                                return r;
-                        }
+
+                        return r;
                 }
 
                 if (r == 0)
@@ -7139,3 +7164,24 @@ int unquote_many_words(const char **p, ...) {
 
         return c;
 }
+
+int free_and_strdup(char **p, const char *s) {
+        char *t;
+
+        assert(p);
+
+        /* Replaces a string pointer with an strdup()ed new string,
+         * possibly freeing the old one. */
+
+        if (s) {
+                t = strdup(s);
+                if (!t)
+                        return -ENOMEM;
+        } else
+                t = NULL;
+
+        free(*p);
+        *p = t;
+
+        return 0;
+}