From 41306b406363f63408d58eacad7f1ceefa0e6073 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Fri, 10 Sep 2010 13:19:26 -0400 Subject: [PATCH] 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. --HG-- branch : 1.7 --- exec.c | 301 +++++++++++++++++++++++++++++++++++++++------------- exec_pty.c | 40 +++++-- list.c | 30 ++++++ list.h | 1 + sudo_exec.h | 3 +- 5 files changed, 290 insertions(+), 85 deletions(-) diff --git a/exec.c b/exec.c index 784f90ab4..86bb7fb8f 100644 --- a/exec.c +++ b/exec.c @@ -65,9 +65,24 @@ #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; } diff --git a/exec_pty.c b/exec_pty.c index 71836f987..c0dd068a3 100644 --- a/exec_pty.c +++ b/exec_pty.c @@ -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 60c113802..1d8c2012b 100644 --- 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 17aab415b..719afa2f3 100644 --- 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 *)); diff --git a/sudo_exec.h b/sudo_exec.h index 6e2691377..83671080f 100644 --- a/sudo_exec.h +++ b/sudo_exec.h @@ -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 */ -- 2.40.0