]> granicus.if.org Git - sudo/commitdiff
Instead of using a array to store received signals, open a pipe and
authorTodd C. Miller <Todd.Miller@courtesan.com>
Fri, 10 Sep 2010 17:19:26 +0000 (13:19 -0400)
committerTodd C. Miller <Todd.Miller@courtesan.com>
Fri, 10 Sep 2010 17:19:26 +0000 (13:19 -0400)
have the signal handler write the signal number to one end and
select() on the other end.  This makes it possible to handle signals
similar to I/O without race conditions.

--HG--
branch : 1.7

exec.c
exec_pty.c
list.c
list.h
sudo_exec.h

diff --git a/exec.c b/exec.c
index 784f90ab40ff80020dc18f2ea12105076042b5c9..86bb7fb8f3707a357514e087f2b88dce35aa32b7 100644 (file)
--- a/exec.c
+++ b/exec.c
 #include "sudo.h"
 #include "sudo_exec.h"
 
-/* shared with exec_pty.c */
-sig_atomic_t recvsig[NSIG];
-void handler __P((int s));
+/* Shared with exec_pty.c for use with handler(). */
+int signal_pipe[2];
+
+#ifdef _PATH_SUDO_IO_LOGDIR
+/* We keep a tailq of signals to forward to child. */
+struct sigforward {
+    struct sigforward *prev, *next;
+    int signo;
+};
+TQ_DECLARE(sigforward)
+static struct sigforward_list sigfwd_list;
+static void forward_signals __P((int fd));
+static void schedule_signal __P((int signo));
+static int log_io;
+#endif /* _PATH_SUDO_IO_LOGDIR */
+
+static int handle_signals __P((int fd, pid_t child,
+    struct command_status *cstat));
 
 /*
  * Like execve(2) but falls back to running through /bin/sh
@@ -118,6 +133,8 @@ static int fork_cmnd(path, argv, envp, sv, rbac_enabled)
     case 0:
        /* child */
        close(sv[0]);
+       close(signal_pipe[0]);
+       close(signal_pipe[1]);
        fcntl(sv[1], F_SETFD, FD_CLOEXEC);
        if (exec_setup(rbac_enabled, user_ttypath, -1) == TRUE) {
            /* headed for execve() */
@@ -152,11 +169,10 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
     int dowait;
     int bgmode;
 {
-    sigaction_t sa;
-    fd_set *fdsr, *fdsw;
-    int maxfd, n, nready, status, sv[2];
+    int maxfd, n, nready, sv[2];
     int rbac_enabled = 0;
-    int log_io;
+    fd_set *fdsr, *fdsw;
+    sigaction_t sa;
     pid_t child;
 
     /* If running in background mode, fork and exit. */
@@ -210,6 +226,13 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
     if (socketpair(PF_UNIX, SOCK_DGRAM, 0, sv) != 0)
        error(1, "cannot create sockets");
 
+    /*
+     * We use a pipe to atomically handle signal notification within
+     * the select() loop.
+     */
+    if (pipe_nonblock(signal_pipe) != 0)
+       error(1, "cannot create pipe");
+
     zero_bytes(&sa, sizeof(sa));
     sigemptyset(&sa.sa_mask);
 
@@ -224,7 +247,7 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
     sigaction(SIGTERM, &sa, NULL);
 
     /* Max fd we will be selecting on. */
-    maxfd = sv[0];
+    maxfd = MAX(sv[0], signal_pipe[0]);
 
     /*
      * Child will run the command in the pty, parent will pass data
@@ -253,72 +276,49 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
     fdsr = (fd_set *)emalloc2(howmany(maxfd + 1, NFDBITS), sizeof(fd_mask));
     fdsw = (fd_set *)emalloc2(howmany(maxfd + 1, NFDBITS), sizeof(fd_mask));
     for (;;) {
-       if (recvsig[SIGCHLD]) {
-           pid_t pid;
-
-           /*
-            * If logging I/O, child is the intermediate process,
-            * otherwise it is the command itself.
-            */
-           recvsig[SIGCHLD] = FALSE;
-           do {
-#ifdef sudo_waitpid
-               pid = sudo_waitpid(child, &status, WUNTRACED|WNOHANG);
-#else
-               pid = wait(&status);
-#endif
-           } while (pid == -1 && errno == EINTR);
-           if (pid == child) {
-               /* If not logging I/O and child has exited we are done. */
-               if (!log_io) {
-                   if (WIFSTOPPED(status)) {
-                       /* Child may not have privs to suspend us itself. */
-                       kill(getpid(), WSTOPSIG(status));
-                   } else {
-                       /* Child has exited, we are done. */
-                       cstat->type = CMD_WSTATUS;
-                       cstat->val = status;
-                       return 0;
-                   }
-               }
-               /* Else we get ECONNRESET on sv[0] if child dies. */
-           }
-       }
-
        zero_bytes(fdsw, howmany(maxfd + 1, NFDBITS) * sizeof(fd_mask));
        zero_bytes(fdsr, howmany(maxfd + 1, NFDBITS) * sizeof(fd_mask));
 
        FD_SET(sv[0], fdsr);
 #ifdef _PATH_SUDO_IO_LOGDIR
+       if (!tq_empty(&sigfwd_list))
+           FD_SET(sv[0], fdsw);
        if (log_io)
            fd_set_iobs(fdsr, fdsw); /* XXX - better name */
 #endif
-       for (n = 0; n < NSIG; n++) {
-           if (recvsig[n] && n != SIGCHLD) {
-               if (log_io) {
-                   FD_SET(sv[0], fdsw);
-                   break;
-               } else {
-                   /* nothing listening on sv[0], send directly */
-                   kill(child, n);
-               }
-           }
-       }
-
-       if (recvsig[SIGCHLD])
-           continue;
        nready = select(maxfd + 1, fdsr, fdsw, NULL, NULL);
        if (nready == -1) {
            if (errno == EINTR)
                continue;
            error(1, "select failed");
        }
+#ifdef _PATH_SUDO_IO_LOGDIR
+       if (FD_ISSET(sv[0], fdsw)) {
+           forward_signals(sv[0]);
+       }
+#endif /* _PATH_SUDO_IO_LOGDIR */
+       if (FD_ISSET(signal_pipe[0], fdsr)) {
+           n = handle_signals(signal_pipe[0], child, cstat);
+           if (n == 0) {
+               /* Child has exited, cstat is set, we are done. */
+               goto done;
+           }
+           if (n == -1) {
+               if (errno == EAGAIN || errno == EINTR)
+                   continue;
+               /* Error reading signal_pipe[0], should not happen. */
+               break;
+           }
+           /* Restart event loop so signals get sent to child immediately. */
+           continue;
+       }
        if (FD_ISSET(sv[0], fdsr)) {
            /* read child status */
            n = recv(sv[0], cstat, sizeof(*cstat), 0);
            if (n == -1) {
                if (errno == EINTR)
                    continue;
+#ifdef _PATH_SUDO_IO_LOGDIR
                /*
                 * If not logging I/O we will receive ECONNRESET when
                 * the command is executed.  It is safe to ignore this.
@@ -328,13 +328,14 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
                    cstat->val = errno;
                    break;
                }
+#endif
            }
 #ifdef _PATH_SUDO_IO_LOGDIR /* XXX */
            if (cstat->type == CMD_WSTATUS) {
                if (WIFSTOPPED(cstat->val)) {
                    /* Suspend parent and tell child how to resume on return. */
                    n = suspend_parent(WSTOPSIG(cstat->val));
-                   recvsig[n] = TRUE;
+                   schedule_signal(n);
                    continue;
                } else {
                    /* Child exited or was killed, either way we are done. */
@@ -349,23 +350,6 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
        }
 
 #ifdef _PATH_SUDO_IO_LOGDIR
-       /* XXX - move this too */
-       if (FD_ISSET(sv[0], fdsw)) {
-           for (n = 0; n < NSIG; n++) {
-               if (!recvsig[n])
-                   continue;
-               recvsig[n] = FALSE;
-               cstat->type = CMD_SIGNO;
-               cstat->val = n;
-               do {
-                   n = send(sv[0], cstat, sizeof(*cstat), 0);
-               } while (n == -1 && errno == EINTR);
-               if (n != sizeof(*cstat)) {
-                   recvsig[n] = TRUE;
-                   break;
-               }
-           }
-       }
        if (perform_io(fdsr, fdsw, cstat) != 0)
            break;
 #endif /* _PATH_SUDO_IO_LOGDIR */
@@ -386,19 +370,188 @@ sudo_execve(path, argv, envp, uid, cstat, dowait, bgmode)
     }
 #endif
 
+done:
     efree(fdsr);
     efree(fdsw);
+#ifdef _PATH_SUDO_IO_LOGDIR
+    while (!tq_empty(&sigfwd_list)) {
+       struct sigforward *sigfwd = tq_first(&sigfwd_list);
+       tq_remove(&sigfwd_list, sigfwd);
+       efree(sigfwd);
+    }
+#endif /* _PATH_SUDO_IO_LOGDIR */
 
     return cstat->type == CMD_ERRNO ? -1 : 0;
 }
 
+/*
+ * Read signals on fd written to by handler().
+ * Returns -1 on error (possibly non-fatal), 0 on child exit, else 1.
+ */
+static int
+handle_signals(fd, child, cstat)
+    int fd;
+    pid_t child;
+    struct command_status *cstat;
+{
+    unsigned char signo;
+    ssize_t nread;
+    int status;
+    pid_t pid;
+
+    for (;;) {
+       /* read signal pipe */
+       nread = read(signal_pipe[0], &signo, sizeof(signo));
+       if (nread <= 0) {
+           /* It should not be possible to get EOF but just in case. */
+           if (nread == 0)
+               errno = ECONNRESET;
+           if (errno != EINTR && errno != EAGAIN) {
+               cstat->type = CMD_ERRNO;
+               cstat->val = errno;
+           }
+           return -1;
+       }
+       if (signo == SIGCHLD) {
+           /*
+            * If logging I/O, child is the intermediate process,
+            * otherwise it is the command itself.
+            */
+           do {
+#ifdef sudo_waitpid
+               pid = sudo_waitpid(child, &status, WUNTRACED|WNOHANG);
+#else
+               pid = wait(&status);
+#endif
+           } while (pid == -1 && errno == EINTR);
+           if (pid == child) {
+               /* If not logging I/O and child has exited we are done. */
+#ifdef _PATH_SUDO_IO_LOGDIR
+               if (!log_io)
+#endif
+               {
+                   if (WIFSTOPPED(status)) {
+                       /* Child may not have privs to suspend us itself. */
+                       kill(getpid(), WSTOPSIG(status));
+                   } else {
+                       /* Child has exited, we are done. */
+                       cstat->type = CMD_WSTATUS;
+                       cstat->val = status;
+                       return 0;
+                   }
+               }
+               /* Else we get ECONNRESET on sv[0] if child dies. */
+           }
+       } else {
+#ifdef _PATH_SUDO_IO_LOGDIR
+           if (log_io) {
+               /* Schedule signo to be forwared to the child. */
+               schedule_signal(signo);
+           } else
+#endif
+           {
+               /* Nothing listening on sv[0], send directly. */
+               kill(child, signo);
+           }
+       }
+    }
+    return 1;
+}
+
+#ifdef _PATH_SUDO_IO_LOGDIR
+/*
+ * Forward signals in sigfwd_list to child listening on fd.
+ */
+static void
+forward_signals(sock)
+    int sock;
+{
+    struct sigforward *sigfwd;
+    struct command_status cstat;
+    ssize_t nsent;
+
+    while (!tq_empty(&sigfwd_list)) {
+       sigfwd = tq_first(&sigfwd_list);
+       cstat.type = CMD_SIGNO;
+       cstat.val = sigfwd->signo;
+       do {
+           nsent = send(sock, &cstat, sizeof(cstat), 0);
+       } while (nsent == -1 && errno == EINTR);
+       tq_remove(&sigfwd_list, sigfwd);
+       efree(sigfwd);
+       if (nsent != sizeof(cstat)) {
+           if (errno == EPIPE) {
+               /* Other end of socket gone, empty out sigfwd_list. */
+               while (!tq_empty(&sigfwd_list)) {
+                   sigfwd = tq_first(&sigfwd_list);
+                   tq_remove(&sigfwd_list, sigfwd);
+                   efree(sigfwd);
+               }
+           }
+           break;
+       }
+    }
+}
+
+/*
+ * Schedule a signal to be forwared.
+ */
+static void
+schedule_signal(signo)
+    int signo;
+{
+    struct sigforward *sigfwd;
+
+    sigfwd = emalloc(sizeof(*sigfwd));
+    sigfwd->prev = sigfwd;
+    sigfwd->next = NULL;
+    sigfwd->signo = signo;
+    tq_append(&sigfwd_list, sigfwd);
+}
+#endif /* _PATH_SUDO_IO_LOGDIR */
+
 /*
  * Generic handler for signals passed from parent -> child.
- * The recvsig[] array is checked in the main event loop.
+ * The other end of signal_pipe is checked in the main event loop.
  */
 void
 handler(s)
     int s;
 {
-    recvsig[s] = TRUE;
+    unsigned char signo = (unsigned char)s;
+
+    /*
+     * The pipe is non-blocking, if we overflow the kernel's pipe
+     * buffer we drop the signal.  This is not a problem in practice.
+     */
+    (void)write(signal_pipe[1], &signo, sizeof(signo));
+}
+
+/*
+ * Open a pipe and make both ends non-blocking.
+ * Returns 0 on success and -1 on error.
+ */
+int
+pipe_nonblock(fds)
+    int fds[2];
+{
+    int flags, rval;
+
+    rval = pipe(fds);
+    if (rval != -1) {
+       flags = fcntl(fds[0], F_GETFL, 0);
+       if (flags != -1 && !ISSET(flags, O_NONBLOCK))
+           rval = fcntl(fds[0], F_SETFL, flags | O_NONBLOCK);
+       if (rval != -1) {
+           flags = fcntl(fds[1], F_GETFL, 0);
+           if (flags != -1 && !ISSET(flags, O_NONBLOCK))
+               rval = fcntl(fds[1], F_SETFL, flags | O_NONBLOCK);
+       }
+       if (rval == -1) {
+           close(fds[0]);
+           close(fds[1]);
+       }
+    }
+
+    return rval;
 }
index 71836f987474e9e65633f34e2268c2f83b4a5368..c0dd068a3b41f7a40acfe5e91651abe163196391 100644 (file)
@@ -448,6 +448,8 @@ fork_pty(path, argv, envp, sv, rbac_enabled, maxfd)
     case 0:
        /* child */
        close(sv[0]);
+       close(signal_pipe[0]);
+       close(signal_pipe[1]);
        fcntl(sv[1], F_SETFD, FD_CLOEXEC);
        if (exec_setup(rbac_enabled, slavename, io_fds[SFD_SLAVE]) == TRUE) {
            /* Close the other end of the stdin/stdout/stderr pipes and exec. */
@@ -699,6 +701,7 @@ exec_monitor(path, argv, envp, backchannel, rbac)
     sigaction_t sa;
     int errpipe[2], maxfd, n, status;
     int alive = TRUE;
+    unsigned char signo;
 
     /* Close unused fds. */
     if (io_fds[SFD_MASTER] != -1)
@@ -706,6 +709,13 @@ exec_monitor(path, argv, envp, backchannel, rbac)
     if (io_fds[SFD_USERTTY] != -1)
        close(io_fds[SFD_USERTTY]);
 
+    /*
+     * We use a pipe to atomically handle signal notification within
+     * the select() loop.
+     */
+    if (pipe_nonblock(signal_pipe) != 0)
+       error(1, "cannot create pipe");
+
     /* Reset SIGWINCH and SIGALRM. */
     zero_bytes(&sa, sizeof(sa));
     sigemptyset(&sa.sa_mask);
@@ -764,6 +774,8 @@ exec_monitor(path, argv, envp, backchannel, rbac)
     if (child == 0) {
        /* We pass errno back to our parent via pipe on exec failure. */
        close(backchannel);
+       close(signal_pipe[0]);
+       close(signal_pipe[1]);
        close(errpipe[0]);
        fcntl(errpipe[1], F_SETFD, FD_CLOEXEC);
 
@@ -797,27 +809,20 @@ exec_monitor(path, argv, envp, backchannel, rbac)
     }
 
     /* Wait for errno on pipe, signal on backchannel or for SIGCHLD */
-    maxfd = MAX(errpipe[0], backchannel);
+    maxfd = MAX(MAX(errpipe[0], signal_pipe[0]), backchannel);
     fdsr = (fd_set *)emalloc2(howmany(maxfd + 1, NFDBITS), sizeof(fd_mask));
     zero_bytes(fdsr, howmany(maxfd + 1, NFDBITS) * sizeof(fd_mask));
     zero_bytes(&cstat, sizeof(cstat));
     tv.tv_sec = 0;
     tv.tv_usec = 0;
     for (;;) {
-       /* Read child status. */
-       if (recvsig[SIGCHLD]) {
-           recvsig[SIGCHLD] = FALSE;
-           alive = handle_sigchld(backchannel, &cstat);
-       }
-
        /* Check for signal on backchannel or errno on errpipe. */
        FD_SET(backchannel, fdsr);
+       FD_SET(signal_pipe[0], fdsr);
        if (errpipe[0] != -1)
            FD_SET(errpipe[0], fdsr);
-       maxfd = MAX(errpipe[0], backchannel);
+       maxfd = MAX(MAX(errpipe[0], signal_pipe[0]), backchannel);
 
-       if (recvsig[SIGCHLD])
-           continue;
        /* If command exited we just poll, there may be data on errpipe. */
        n = select(maxfd + 1, fdsr, NULL, NULL, alive ? NULL : &tv);
        if (n <= 0) {
@@ -828,6 +833,21 @@ exec_monitor(path, argv, envp, backchannel, rbac)
            error(1, "select failed");
        }
 
+       if (FD_ISSET(signal_pipe[0], fdsr)) {
+           /* Read child status. */
+           n = read(signal_pipe[0], &signo, sizeof(signo));
+           if (n == -1) {
+               if (errno == EINTR || errno == EAGAIN)
+                   continue;
+               warning("error reading from signal pipe");
+               goto done;
+           }
+           /* We should only ever get SIGCHLD. */
+           if (signo == SIGCHLD) {
+               alive = handle_sigchld(backchannel, &cstat);
+               continue;
+           }
+       }
        if (errpipe[0] != -1 && FD_ISSET(errpipe[0], fdsr)) {
            /* read errno or EOF from command pipe */
            n = read(errpipe[0], &cstat, sizeof(cstat));
diff --git a/list.c b/list.c
index 60c1138026ac7ad0c84858cbd4ad959820abc385..1d8c2012ba29f603784c3aa299549fa5b16bbcf1 100644 (file)
--- a/list.c
+++ b/list.c
@@ -131,3 +131,33 @@ tq_append(vh, vl)
     l->prev = h->last;
     h->last = tail;
 }
+
+/*
+ * Remove element from the tail_queue
+ */
+void
+tq_remove(vh, vl)
+    void *vh;
+    void *vl;
+{
+    struct list_head_proto *h = (struct list_head_proto *)vh;
+    struct list_proto *l = (struct list_proto *)vl;
+
+    if (h->first == l && h->last == l) {
+       /* Single element in the list. */
+       h->first = NULL;
+       h->last = NULL;
+    } else {
+       /* At least two elements in the list. */
+       if (h->first == l) {
+           h->first = l->next;
+           h->first->prev = h->first;
+       } else if (h->last == l) {
+           h->last = l->prev;
+           h->last->next = NULL;
+       } else {
+           l->prev->next = l->next;
+           l->next->prev = l->prev;
+       }
+    }
+}
diff --git a/list.h b/list.h
index 17aab415b1cc19189260f20e73fe6a1c4b546be9..719afa2f3275b44b397cda5f3473e78bd0c081d8 100644 (file)
--- a/list.h
+++ b/list.h
@@ -77,6 +77,7 @@ struct n/**/_list {                                   \
  */
 void *tq_pop           __P((void *));
 void tq_append         __P((void *, void *));
+void tq_remove         __P((void *, void *));
 void list_append       __P((void *, void *));
 void list2tq           __P((void *, void *));
 
index 6e2691377683def349e9dc950efedff6df32f24b..83671080fed30932f39932acd21e2d30a57e55f0 100644 (file)
@@ -23,6 +23,7 @@
 
 /* exec.c */
 int my_execve __P((const char *path, char *argv[], char *envp[]));
+int pipe_nonblock __P((int fds[2]));
 
 /* exec_pty.c */
 int fork_pty __P((const char *path, char *argv[], char *envp[], int sv[],
@@ -33,6 +34,6 @@ void fd_set_iobs __P((fd_set *fdsr, fd_set *fdsw));
 void handler __P((int s));
 void pty_close __P((struct command_status *cstat));
 void pty_setup __P((uid_t uid));
-extern sig_atomic_t recvsig[NSIG];
+extern int signal_pipe[2];
 
 #endif /* _SUDO_EXEC_H */