Call for testing: OpenSSH 8.7

[Darren Tucker]

On Thu, Aug 19, 2021 at 04:39:51PM +1000, Darren Tucker wrote:
> Actually hold off on that for a bit, I think I know what's going on and
> I'll have a theory and diff to try shortly.

My original theory was that the two sshd processes were sharing the
descriptor and that because notify_done is called unconditionally,
if the "wrong" process scheduled first it'd eat the notification from
the notify_pipe.  Having added some debugging, poking it and looking at
the code I don't think that's possible with the current code: the
listening sshd re-execs itself when it gets a connection and the
descriptors are set close-on-exec although it might be possible if we
add more calls to pselect in the future.

While looking at that I think I found a bug in the pselect setup: on
second and subsequent calls the signal hander shim is not installed (which
is fine, it's already installed) but it also doesn't set the unmasked
flag and hence the notify_pipe is not added to select()'s readset.
This would explain what you're seeing: if the signal is delivered between
the signals being unmasked and the select() being called, the handler
would write to notify_pipe but the select wouldn't be looking for it.

I've added code to handle both of those cases, and some more debugging
in case that doesn't fix it, so if it doesn't please send us the logs
from the patched version.

Thanks.

diff --git a/openbsd-compat/bsd-pselect.c b/openbsd-compat/bsd-pselect.c
index da34b41d..eb6f113c 100644
--- a/openbsd-compat/bsd-pselect.c
+++ b/openbsd-compat/bsd-pselect.c
@@ -73,6 +73,7 @@ notify_setup_fd(int *fd)
  * we write to this pipe if a SIGCHLD is caught in order to avoid
  * the race between select() and child_terminated
  */
+static pid_t notify_pid;
 static int notify_pipe[2];
 static void
 notify_setup(void)
@@ -81,6 +82,15 @@ notify_setup(void)
 
 	if (initialized)
 		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 (notify_setup_fd(&notify_pipe[0]) == -1 ||
@@ -91,6 +101,9 @@ notify_setup(void)
 	} else {
 		set_nonblock(notify_pipe[0]);
 		set_nonblock(notify_pipe[1]);
+		notify_pid = getpid();
+		debug_f("pid %d saved %d pipe0 %d pipe1 %d", getpid(),
+		    notify_pid, notify_pipe[0], notify_pipe[1]);
 		initialized = 1;
 		return;
 	}
@@ -159,15 +172,16 @@ pselect(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 		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 &&
-		    sa.sa_handler != sig_handler) {
+		    sa.sa_handler != SIG_IGN && sa.sa_handler != SIG_DFL) {
+			unmasked = 1;
+			if (sa.sa_handler == sig_handler)
+				continue;
 			sa.sa_handler = sig_handler;
 			if (sigaction(sig, &sa, &osa) == 0) {
 				debug3_f("installing signal handler for %s, "
 				    "previous %p", strsignal(sig),
 				     osa.sa_handler);
 				saved_sighandler[sig] = osa.sa_handler;
-				unmasked = 1;
 			}
 		}
 	}
@@ -183,7 +197,8 @@ pselect(int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	saved_errno = errno;
 	sigprocmask(SIG_SETMASK, &osig, NULL);
 
-	notify_done(readfds);
+	if (unmasked)
+		notify_done(readfds);
 	errno = saved_errno;
 	return ret;
 }

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.