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@

1 files 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 <unistd.h>
 
 #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(&notify_pipe[0]) == -1 ||
-	    pselect_notify_setup_fd(&notify_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;
 }