From: Todd C. Miller Date: Fri, 10 Sep 2010 15:20:32 +0000 (-0400) Subject: Instead of using a array to store received signals, open a pipe and X-Git-Tag: SUDO_1_8_0~249 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=59399d55c30f46b18568d0e87d29dfc44826e873;p=sudo 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. --- diff --git a/src/exec.c b/src/exec.c index d2e80291c..10ffba531 100644 --- a/src/exec.c +++ b/src/exec.c @@ -60,9 +60,21 @@ #include "sudo_plugin.h" #include "sudo_plugin_int.h" -/* shared with exec_pty.c */ -sig_atomic_t recvsig[NSIG]; -void handler(int s); +/* Shared with exec_pty.c for use with handler(). */ +int signal_pipe[2]; + +/* 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]; - int log_io; + sigaction_t sa; 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; } diff --git a/src/exec_pty.c b/src/exec_pty.c index 0e8adc321..e77b60bc7 100644 --- a/src/exec_pty.c +++ b/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)); diff --git a/src/sudo_exec.h b/src/sudo_exec.h index 5151a2241..309f9c16d 100644 --- a/src/sudo_exec.h +++ b/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 */