From 326495744f06a0ab18ee0d16f87b3fe91cac92fb Mon Sep 17 00:00:00 2001 From: Darren Tucker Date: Fri, 25 Oct 2024 19:01:02 +1100 Subject: Simplify pselect shim and remove side effects. Instead of maintaing state (pipe descriptors, signal handlers) across pselect-on-select invocations, set up and restore them each call. This prevents outside factors (eg a closefrom or signal handler installation) from potentially causing problems. This does result in a drop in throughput of a couple of percent on geriatric platforms without a native pselect due to the extra overhead. Tweaks & ok djm@ --- openbsd-compat/bsd-pselect.c | 106 +++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 55 deletions(-) diff --git a/openbsd-compat/bsd-pselect.c b/openbsd-compat/bsd-pselect.c index b36320863..26bdc3e08 100644 --- a/openbsd-compat/bsd-pselect.c +++ b/openbsd-compat/bsd-pselect.c @@ -40,75 +40,47 @@ #include #include "log.h" -#include "misc.h" /* for set_nonblock */ #ifndef HAVE_SIGHANDLER_T typedef void (*sighandler_t)(int); #endif static sighandler_t saved_sighandler[_NSIG]; +static int notify_pipe[2]; /* 0 = read end, 1 = write end */ /* - * Set up the descriptors. Because they are close-on-exec, in the case - * where sshd's re-exec fails notify_pipe will still point to a descriptor - * that was closed by the exec attempt but if that descriptor has been - * reopened then we'll attempt to use that. Ensure that notify_pipe is - * outside of the range used by sshd re-exec but within NFDBITS (so we don't - * need to expand the fd_sets). + * Because the debugging for this is so noisy, we only output on the first + * call, and suppress it thereafter. */ -#define REEXEC_MIN_FREE_FD (STDERR_FILENO + 4) -static int -pselect_notify_setup_fd(int *fd) +static int suppress_debug; + +static void +pselect_set_nonblock(int fd) { - int r; + int val; - if ((r = fcntl(*fd, F_DUPFD, REEXEC_MIN_FREE_FD)) < 0 || - fcntl(r, F_SETFD, FD_CLOEXEC) < 0 || r >= FD_SETSIZE) - return -1; - (void)close(*fd); - return (*fd = r); + if ((val = fcntl(fd, F_GETFL)) == -1 || + fcntl(fd, F_SETFL, val|O_NONBLOCK) == -1) + error_f("fcntl: %s", strerror(errno)); } /* * we write to this pipe if a SIGCHLD is caught in order to avoid - * the race between select() and child_terminated + * the race between select() and child_terminated. */ -static pid_t notify_pid; -static int notify_pipe[2]; -static void +static int pselect_notify_setup(void) { - static int initialized; - - if (initialized && notify_pid == getpid()) - return; - if (notify_pid == 0) - debug3_f("initializing"); - else { - debug3_f("pid changed, reinitializing"); - if (notify_pipe[0] != -1) - close(notify_pipe[0]); - if (notify_pipe[1] != -1) - close(notify_pipe[1]); - } if (pipe(notify_pipe) == -1) { error("pipe(notify_pipe) failed %s", strerror(errno)); - } else if (pselect_notify_setup_fd(¬ify_pipe[0]) == -1 || - pselect_notify_setup_fd(¬ify_pipe[1]) == -1) { - error("fcntl(notify_pipe, ...) failed %s", strerror(errno)); - close(notify_pipe[0]); - close(notify_pipe[1]); - } else { - set_nonblock(notify_pipe[0]); - set_nonblock(notify_pipe[1]); - notify_pid = getpid(); - debug3_f("pid %d saved %d pipe0 %d pipe1 %d", getpid(), - notify_pid, notify_pipe[0], notify_pipe[1]); - initialized = 1; - return; + notify_pipe[0] = notify_pipe[1] = -1; + return -1; } - notify_pipe[0] = -1; /* read end */ - notify_pipe[1] = -1; /* write end */ + pselect_set_nonblock(notify_pipe[0]); + pselect_set_nonblock(notify_pipe[1]); + if (!suppress_debug) + debug3_f("pipe0 %d pipe1 %d", notify_pipe[0], notify_pipe[1]); + return 0; } static void pselect_notify_parent(void) @@ -132,6 +104,8 @@ pselect_notify_done(fd_set *readset) debug2_f("reading"); FD_CLR(notify_pipe[0], readset); } + (void)close(notify_pipe[0]); + (void)close(notify_pipe[1]); } /*ARGSUSED*/ @@ -167,26 +141,29 @@ pselect(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, if (mask == NULL) /* no signal mask, just call select */ return select(nfds, readfds, writefds, exceptfds, tvp); - /* For each signal we're unmasking, install our handler if needed. */ + /* For each signal unmasked, save old handler and install ours. */ for (sig = 0; sig < _NSIG; sig++) { + saved_sighandler[sig] = NULL; if (sig == SIGKILL || sig == SIGSTOP || sigismember(mask, sig)) continue; if (sigaction(sig, NULL, &sa) == 0 && sa.sa_handler != SIG_IGN && sa.sa_handler != SIG_DFL) { unmasked = 1; - if (sa.sa_handler == pselect_sig_handler) - continue; sa.sa_handler = pselect_sig_handler; if (sigaction(sig, &sa, &osa) == 0) { - debug3_f("installing signal handler for %s, " - "previous %p", strsignal(sig), - osa.sa_handler); + if (!suppress_debug) + debug3_f("installed signal handler for" + " %s, previous 0x%p", + strsignal(sig), osa.sa_handler); saved_sighandler[sig] = osa.sa_handler; } } } if (unmasked) { - pselect_notify_setup(); + if ((ret = pselect_notify_setup()) == -1) { + saved_errno = ENOMEM; + goto out; + } pselect_notify_prepare(readfds); nfds = MAX(nfds, notify_pipe[0] + 1); } @@ -199,6 +176,25 @@ pselect(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds, if (unmasked) pselect_notify_done(readfds); + + out: + /* Restore signal handlers. */ + for (sig = 0; sig < _NSIG; sig++) { + if (saved_sighandler[sig] == NULL) + continue; + if (sigaction(sig, NULL, &sa) == 0) { + sa.sa_handler = saved_sighandler[sig]; + if (sigaction(sig, &sa, NULL) == 0) { + if (!suppress_debug) + debug3_f("restored signal handler for " + "%s", strsignal(sig)); + } else { + error_f("failed to restore signal handler for " + "%s: %s", strsignal(sig), strerror(errno)); + } + } + } + suppress_debug = 1; errno = saved_errno; return ret; } -- cgit v1.2.3