[openssh-commits] [openssh] 01/03: Simplify pselect shim and remove side effects.
git+noreply at mindrot.org
git+noreply at mindrot.org
Fri Oct 25 19:16:51 AEDT 2024
This is an automated email from the git hooks/post-receive script.
dtucker pushed a commit to branch master
in repository openssh.
commit 326495744f06a0ab18ee0d16f87b3fe91cac92fb
Author: Darren Tucker <dtucker at dtucker.net>
AuthorDate: Fri Oct 25 19:01:02 2024 +1100
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 b3632086..26bdc3e0 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(¬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;
}
--
To stop receiving notification emails like this one, please contact
djm at mindrot.org.
More information about the openssh-commits
mailing list