Instead of using a array to store received signals, open a pipe and

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.

src/exec.c

@@ -60,9 +60,21 @@
 #include "sudo_plugin.h"
 #include "sudo_plugin_int.h"
 
-/* shared with exec_pty.c */
+/* Shared with exec_pty.c for use with handler(). */
-sig_atomic_t recvsig[NSIG];
+int signal_pipe[2];
-void handler(int s);
+
+/* 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 int handle_signals(int fd, pid_t child, int log_io,
+    struct command_status *cstat);
+static void forward_signals(int fd);
+static void schedule_signal(int signo);
 
 /*
  * Like execve(2) but falls back to running through /bin/sh
@@ -113,6 +125,8 @@ static int fork_cmnd(struct command_details *details, char *argv[],
     case 0:
 	/* child */
 	close(sv[0]);
+	close(signal_pipe[0]);
+	close(signal_pipe[1]);
 	fcntl(sv[1], F_SETFD, FD_CLOEXEC);
 	if (exec_setup(details, NULL, -1) == TRUE) {
 	    /* headed for execve() */
@@ -142,10 +156,9 @@ int
 sudo_execve(struct command_details *details, char *argv[], char *envp[],
     struct command_status *cstat)
 {
-    sigaction_t sa;
+    int log_io, maxfd, n, nready, sv[2];
     fd_set *fdsr, *fdsw;
-    int maxfd, n, nready, status, sv[2];
+    sigaction_t sa;
-    int log_io;
     pid_t child;
 
     /* If running in background mode, fork and exit. */
@@ -177,6 +190,13 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
     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);
 
@@ -192,7 +212,7 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
     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
@@ -223,64 +243,39 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
     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 {
-		pid = waitpid(child, &status, WUNTRACED|WNOHANG);
-	    } 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);
+	if (!tq_empty(&sigfwd_list))
+	    FD_SET(sv[0], fdsw);
+	FD_SET(signal_pipe[0], fdsr);
 	if (log_io)
 	    fd_set_iobs(fdsr, fdsw); /* XXX - better name */
-	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 */
-		    if (n == SIGALRM) {
-			terminate_child(child, FALSE);
-		    } else {
-			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");
 	}
+	if (FD_ISSET(sv[0], fdsw)) {
+	    forward_signals(sv[0]);
+	}
+	if (FD_ISSET(signal_pipe[0], fdsr)) {
+	    n = handle_signals(signal_pipe[0], child, log_io, 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);
@@ -302,7 +297,7 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
 		    /* Suspend parent and tell child how to resume on return. */
 		    sudo_debug(8, "child stopped, suspending parent");
 		    n = suspend_parent(WSTOPSIG(cstat->val));
-		    recvsig[n] = TRUE;
+		    schedule_signal(n);
 		    continue;
 		} else {
 		    /* Child exited or was killed, either way we are done. */
@@ -314,23 +309,6 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
 	    }
 	}
 
-	if (FD_ISSET(sv[0], fdsw)) {
-	    for (n = 0; n < NSIG; n++) {
-		if (!recvsig[n])
-		    continue;
-		recvsig[n] = FALSE;
-		sudo_debug(9, "sending signal %d to child over backchannel", n);
-		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;
     }
@@ -348,18 +326,173 @@ sudo_execve(struct command_details *details, char *argv[], char *envp[],
     }
 #endif
 
+done:
     efree(fdsr);
     efree(fdsw);
+    while (!tq_empty(&sigfwd_list)) {
+	struct sigforward *sigfwd = tq_first(&sigfwd_list);
+	tq_remove(&sigfwd_list, sigfwd);
+	efree(sigfwd);
+    }
 
     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(int fd, pid_t child, int log_io, struct command_status *cstat)
+{
+    unsigned char signo;
+    ssize_t nread;
+    int status;
+    pid_t pid;
+
+    /* 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) {
+	    sudo_debug(9, "error reading signal pipe %s", strerror(errno));
+	    cstat->type = CMD_ERRNO;
+	    cstat->val = errno;
+	}
+	return -1;
+    }
+    sudo_debug(9, "received signal %d", signo);
+    if (signo == SIGCHLD) {
+	/*
+	 * If logging I/O, child is the intermediate process,
+	 * otherwise it is the command itself.
+	 */
+	do {
+	    pid = waitpid(child, &status, WUNTRACED|WNOHANG);
+	} 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. */
+	}
+    } else {
+	if (log_io) {
+	    /* Schedule signo to be forwared to the child. */
+	    schedule_signal(signo);
+	} else {
+	    /* Nothing listening on sv[0], send directly. */
+	    if (signo == SIGALRM) {
+		terminate_child(child, FALSE);
+	    } else {
+		kill(child, signo);
+	    }
+	}
+    }
+    return 1;
+}
+
+/*
+ * Forward signals in sigfwd_list to child listening on fd.
+ */
+static void
+forward_signals(int sock)
+{
+    struct sigforward *sigfwd;
+    struct command_status cstat;
+    ssize_t nsent;
+
+    while (!tq_empty(&sigfwd_list)) {
+	sigfwd = tq_first(&sigfwd_list);
+	sudo_debug(9, "sending signal %d to child over backchannel",
+	    sigfwd->signo);
+	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(int signo)
+{
+    struct sigforward *sigfwd;
+
+    sigfwd = emalloc(sizeof(*sigfwd));
+    sigfwd->prev = sigfwd;
+    sigfwd->next = NULL;
+    sigfwd->signo = signo;
+    tq_append(&sigfwd_list, sigfwd);
+}
+
 /*
  * 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(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(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;
 }

src/exec_pty.c

@@ -567,6 +567,8 @@ fork_pty(struct command_details *details, char *argv[], char *envp[],
     case 0:
 	/* child */
 	close(sv[0]);
+	close(signal_pipe[0]);
+	close(signal_pipe[1]);
 	fcntl(sv[1], F_SETFD, FD_CLOEXEC);
 	if (exec_setup(details, slavename, io_fds[SFD_SLAVE]) == TRUE) {
 	    /* Close the other end of the stdin/stdout/stderr pipes and exec. */
@@ -797,7 +799,7 @@ handle_sigchld(int backchannel, struct command_status *cstat)
  * parent and relays signals from the parent to the command.
  * Returns an error if fork(2) fails, else calls _exit(2).
  */
-int
+static int
 exec_monitor(struct command_details *details, char *argv[], char *envp[],
     int backchannel)
 {
@@ -807,6 +809,7 @@ exec_monitor(struct command_details *details, char *argv[], char *envp[],
     sigaction_t sa;
     int errpipe[2], maxfd, n, status;
     int alive = TRUE;
+    unsigned char signo;
 
     /* Close unused fds. */
     if (io_fds[SFD_MASTER] != -1)
@@ -814,6 +817,13 @@ exec_monitor(struct command_details *details, char *argv[], char *envp[],
     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);
@@ -872,6 +882,8 @@ exec_monitor(struct command_details *details, char *argv[], char *envp[],
     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);
 
@@ -905,27 +917,20 @@ exec_monitor(struct command_details *details, char *argv[], char *envp[],
     }
 
     /* 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) {
@@ -936,6 +941,21 @@ exec_monitor(struct command_details *details, char *argv[], char *envp[],
 	    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));

src/sudo_exec.h

@@ -23,6 +23,7 @@
 
 /* exec.c */
 int my_execve(const char *path, char *const argv[], char *const envp[]);
+int pipe_nonblock(int fds[2]);
 
 /* exec_pty.c */
 int fork_pty(struct command_details *details, char *argv[], char *envp[],
@@ -34,6 +35,6 @@ void handler(int s);
 void pty_close(struct command_status *cstat);
 void pty_setup(uid_t uid);
 void terminate_child(pid_t pid, int use_pgrp);
-extern sig_atomic_t recvsig[NSIG];
+extern int signal_pipe[2];
 
 #endif /* _SUDO_EXEC_H */