From: djm <djm@openbsd.org>
Date: Wed, 30 Mar 2022 21:10:25 +0000 (+0000)
Subject: fix poll() spin when a channel's output fd closes without data in the
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=85f274b89bba9ab4c400b94f990d97c9578e8498;p=openbsd

fix poll() spin when a channel's output fd closes without data in the
channel buffer. Introduce more exact packing of channel fds into the
pollfd array. fixes bz3405 and bz3411; ok deraadt@ markus@
---

diff --git a/usr.bin/ssh/channels.c b/usr.bin/ssh/channels.c
index d6048b34dda..9491c85c11b 100644
--- a/usr.bin/ssh/channels.c
+++ b/usr.bin/ssh/channels.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.c,v 1.414 2022/03/15 05:27:37 djm Exp $ */
+/* $OpenBSD: channels.c,v 1.415 2022/03/30 21:10:25 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -420,21 +420,25 @@ channel_close_fd(struct ssh *ssh, Channel *c, int *fdp)
 		c->io_want &= ~SSH_CHAN_IO_RFD;
 		c->io_ready &= ~SSH_CHAN_IO_RFD;
 		c->rfd = -1;
+		c->pfds[0] = -1;
 	}
 	if (*fdp == c->wfd) {
 		c->io_want &= ~SSH_CHAN_IO_WFD;
 		c->io_ready &= ~SSH_CHAN_IO_WFD;
 		c->wfd = -1;
+		c->pfds[1] = -1;
 	}
 	if (*fdp == c->efd) {
 		c->io_want &= ~SSH_CHAN_IO_EFD;
 		c->io_ready &= ~SSH_CHAN_IO_EFD;
 		c->efd = -1;
+		c->pfds[2] = -1;
 	}
 	if (*fdp == c->sock) {
 		c->io_want &= ~SSH_CHAN_IO_SOCK;
 		c->io_ready &= ~SSH_CHAN_IO_SOCK;
 		c->sock = -1;
+		c->pfds[3] = -1;
 	}
 
 	ret = close(fd);
@@ -2433,10 +2437,13 @@ dump_channel_poll(const char *func, const char *what, Channel *c,
     u_int pollfd_offset, struct pollfd *pfd)
 {
 #ifdef DEBUG_CHANNEL_POLL
-	debug3("%s: channel %d: rfd r%d w%d e%d s%d pfd[%u].fd=%d "
-	    "io_want 0x%02x pfd.ev 0x%02x io_ready 0x%02x pfd.rev 0x%02x",
-	    func, c->self, c->rfd, c->wfd, c->efd, c->sock, pollfd_offset,
-	    pfd->fd, c->io_want, pfd->events, c->io_ready, pfd->revents);
+	debug3("%s: channel %d: %s r%d w%d e%d s%d c->pfds [ %d %d %d %d ] "
+	    "io_want 0x%02x io_ready 0x%02x pfd[%u].fd=%d "
+	    "pfd.ev 0x%02x pfd.rev 0x%02x", func, c->self, what,
+	    c->rfd, c->wfd, c->efd, c->sock,
+	    c->pfds[0], c->pfds[1], c->pfds[2], c->pfds[3],
+	    c->io_want, c->io_ready,
+	    pollfd_offset, pfd->fd, pfd->events, pfd->revents);
 #endif
 }
 
@@ -2445,7 +2452,7 @@ static void
 channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
     struct pollfd *pfd, u_int npfd)
 {
-	u_int p = *next_pollfd;
+	u_int ev, p = *next_pollfd;
 
 	if (c == NULL)
 		return;
@@ -2454,7 +2461,7 @@ channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
 		fatal_f("channel %d: bad pfd offset %u (max %u)",
 		    c->self, p, npfd);
 	}
-	c->pollfd_offset = -1;
+	c->pfds[0] = c->pfds[1] = c->pfds[2] = c->pfds[3] = -1;
 	/*
 	 * prepare c->rfd
 	 *
@@ -2463,69 +2470,82 @@ channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
 	 * IO too.
 	 */
 	if (c->rfd != -1) {
-		if (c->pollfd_offset == -1)
-			c->pollfd_offset = p;
-		pfd[p].fd = c->rfd;
-		pfd[p].events = 0;
+		ev = 0;
 		if ((c->io_want & SSH_CHAN_IO_RFD) != 0)
-			pfd[p].events |= POLLIN;
+			ev |= POLLIN;
 		/* rfd == wfd */
-		if (c->wfd == c->rfd &&
-		    (c->io_want & SSH_CHAN_IO_WFD) != 0)
-			pfd[p].events |= POLLOUT;
+		if (c->wfd == c->rfd) {
+			if ((c->io_want & SSH_CHAN_IO_WFD) != 0)
+				ev |= POLLOUT;
+		}
 		/* rfd == efd */
-		if (c->efd == c->rfd &&
-		    (c->io_want & SSH_CHAN_IO_EFD_R) != 0)
-			pfd[p].events |= POLLIN;
-		if (c->efd == c->rfd &&
-		    (c->io_want & SSH_CHAN_IO_EFD_W) != 0)
-			pfd[p].events |= POLLOUT;
+		if (c->efd == c->rfd) {
+			if ((c->io_want & SSH_CHAN_IO_EFD_R) != 0)
+				ev |= POLLIN;
+			if ((c->io_want & SSH_CHAN_IO_EFD_W) != 0)
+				ev |= POLLOUT;
+		}
 		/* rfd == sock */
-		if (c->sock == c->rfd &&
-		    (c->io_want & SSH_CHAN_IO_SOCK_R) != 0)
-			pfd[p].events |= POLLIN;
-		if (c->sock == c->rfd &&
-		    (c->io_want & SSH_CHAN_IO_SOCK_W) != 0)
-			pfd[p].events |= POLLOUT;
-		dump_channel_poll(__func__, "rfd", c, p, &pfd[p]);
-		p++;
-	}
-	/* prepare c->wfd (if not already handled above) */
+		if (c->sock == c->rfd) {
+			if ((c->io_want & SSH_CHAN_IO_SOCK_R) != 0)
+				ev |= POLLIN;
+			if ((c->io_want & SSH_CHAN_IO_SOCK_W) != 0)
+				ev |= POLLOUT;
+		}
+		/* Pack a pfd entry if any event armed for this fd */
+		if (ev != 0) {
+			c->pfds[0] = p;
+			pfd[p].fd = c->rfd;
+			pfd[p].events = ev;
+			dump_channel_poll(__func__, "rfd", c, p, &pfd[p]);
+			p++;
+		}
+	}
+	/* prepare c->wfd if wanting IO and not already handled above */
 	if (c->wfd != -1 && c->rfd != c->wfd) {
-		if (c->pollfd_offset == -1)
-			c->pollfd_offset = p;
-		pfd[p].fd = c->wfd;
-		pfd[p].events = 0;
-		if ((c->io_want & SSH_CHAN_IO_WFD) != 0)
-			pfd[p].events = POLLOUT;
-		dump_channel_poll(__func__, "wfd", c, p, &pfd[p]);
-		p++;
-	}
-	/* prepare c->efd (if not already handled above) */
+		ev = 0;
+		if ((c->io_want & SSH_CHAN_IO_WFD))
+			ev |= POLLOUT;
+		/* Pack a pfd entry if any event armed for this fd */
+		if (ev != 0) {
+			c->pfds[1] = p;
+			pfd[p].fd = c->wfd;
+			pfd[p].events = ev;
+			dump_channel_poll(__func__, "wfd", c, p, &pfd[p]);
+			p++;
+		}
+	}
+	/* prepare c->efd if wanting IO and not already handled above */
 	if (c->efd != -1 && c->rfd != c->efd) {
-		if (c->pollfd_offset == -1)
-			c->pollfd_offset = p;
-		pfd[p].fd = c->efd;
-		pfd[p].events = 0;
+		ev = 0;
 		if ((c->io_want & SSH_CHAN_IO_EFD_R) != 0)
-			pfd[p].events |= POLLIN;
+			ev |= POLLIN;
 		if ((c->io_want & SSH_CHAN_IO_EFD_W) != 0)
-			pfd[p].events |= POLLOUT;
-		dump_channel_poll(__func__, "efd", c, p, &pfd[p]);
-		p++;
+			ev |= POLLOUT;
+		/* Pack a pfd entry if any event armed for this fd */
+		if (ev != 0) {
+			c->pfds[2] = p;
+			pfd[p].fd = c->efd;
+			pfd[p].events = ev;
+			dump_channel_poll(__func__, "efd", c, p, &pfd[p]);
+			p++;
+		}
 	}
-	/* prepare c->sock (if not already handled above) */
+	/* prepare c->sock if wanting IO and not already handled above */
 	if (c->sock != -1 && c->rfd != c->sock) {
-		if (c->pollfd_offset == -1)
-			c->pollfd_offset = p;
-		pfd[p].fd = c->sock;
-		pfd[p].events = 0;
+		ev = 0;
 		if ((c->io_want & SSH_CHAN_IO_SOCK_R) != 0)
-			pfd[p].events |= POLLIN;
+			ev |= POLLIN;
 		if ((c->io_want & SSH_CHAN_IO_SOCK_W) != 0)
-			pfd[p].events |= POLLOUT;
-		dump_channel_poll(__func__, "sock", c, p, &pfd[p]);
-		p++;
+			ev |= POLLOUT;
+		/* Pack a pfd entry if any event armed for this fd */
+		if (ev != 0) {
+			c->pfds[3] = p;
+			pfd[p].fd = c->sock;
+			pfd[p].events = 0;
+			dump_channel_poll(__func__, "sock", c, p, &pfd[p]);
+			p++;
+		}
 	}
 	*next_pollfd = p;
 }
@@ -2572,13 +2592,15 @@ channel_prepare_poll(struct ssh *ssh, struct pollfd **pfdp, u_int *npfd_allocp,
 }
 
 static void
-fd_ready(Channel *c, u_int p, struct pollfd *pfds, int fd,
+fd_ready(Channel *c, int p, struct pollfd *pfds, u_int npfd, int fd,
     const char *what, u_int revents_mask, u_int ready)
 {
 	struct pollfd *pfd = &pfds[p];
 
 	if (fd == -1)
 		return;
+	if (p == -1 || (u_int)p >= npfd)
+		fatal_f("channel %d: bad pfd %d (max %u)", c->self, p, npfd);
 	dump_channel_poll(__func__, what, c, p, pfd);
 	if (pfd->fd != fd) {
 		fatal("channel %d: inconsistent %s fd=%d pollfd[%u].fd %d "
@@ -2601,11 +2623,12 @@ void
 channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
 {
 	struct ssh_channels *sc = ssh->chanctxt;
-	u_int i, p;
+	u_int i;
+	int p;
 	Channel *c;
 
 #ifdef DEBUG_CHANNEL_POLL
-	for (p = 0; p < npfd; p++) {
+	for (p = 0; p < (int)npfd; p++) {
 		if (pfd[p].revents == 0)
 			continue;
 		debug_f("pfd[%u].fd %d rev 0x%04x",
@@ -2616,13 +2639,8 @@ channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
 	/* Convert pollfd into c->io_ready */
 	for (i = 0; i < sc->channels_alloc; i++) {
 		c = sc->channels[i];
-		if (c == NULL || c->pollfd_offset < 0)
+		if (c == NULL)
 			continue;
-		if ((u_int)c->pollfd_offset >= npfd) {
-			/* shouldn't happen */
-			fatal_f("channel %d: (before) bad pfd %u (max %u)",
-			    c->self, c->pollfd_offset, npfd);
-		}
 		/* if rfd is shared with efd/sock then wfd should be too */
 		if (c->rfd != -1 && c->wfd != -1 && c->rfd != c->wfd &&
 		    (c->rfd == c->efd || c->rfd == c->sock)) {
@@ -2631,56 +2649,52 @@ channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
 			    c->self, c->rfd, c->wfd, c->efd, c->sock);
 		}
 		c->io_ready = 0;
-		p = c->pollfd_offset;
 		/* rfd, potentially shared with wfd, efd and sock */
-		if (c->rfd != -1) {
-			fd_ready(c, p, pfd, c->rfd, "rfd", POLLIN,
-			    SSH_CHAN_IO_RFD);
+		if (c->rfd != -1 && (p = c->pfds[0]) != -1) {
+			fd_ready(c, p, pfd, npfd, c->rfd,
+			    "rfd", POLLIN, SSH_CHAN_IO_RFD);
 			if (c->rfd == c->wfd) {
-				fd_ready(c, p, pfd, c->wfd, "wfd/r", POLLOUT,
-				    SSH_CHAN_IO_WFD);
+				fd_ready(c, p, pfd, npfd, c->wfd,
+				    "wfd/r", POLLOUT, SSH_CHAN_IO_WFD);
 			}
 			if (c->rfd == c->efd) {
-				fd_ready(c, p, pfd, c->efd, "efdr/r", POLLIN,
-				    SSH_CHAN_IO_EFD_R);
-				fd_ready(c, p, pfd, c->efd, "efdw/r", POLLOUT,
-				    SSH_CHAN_IO_EFD_W);
+				fd_ready(c, p, pfd, npfd, c->efd,
+				    "efdr/r", POLLIN, SSH_CHAN_IO_EFD_R);
+				fd_ready(c, p, pfd, npfd, c->efd,
+				    "efdw/r", POLLOUT, SSH_CHAN_IO_EFD_W);
 			}
 			if (c->rfd == c->sock) {
-				fd_ready(c, p, pfd, c->sock, "sockr/r", POLLIN,
-				    SSH_CHAN_IO_SOCK_R);
-				fd_ready(c, p, pfd, c->sock, "sockw/r", POLLOUT,
-				    SSH_CHAN_IO_SOCK_W);
+				fd_ready(c, p, pfd, npfd, c->sock,
+				    "sockr/r", POLLIN, SSH_CHAN_IO_SOCK_R);
+				fd_ready(c, p, pfd, npfd, c->sock,
+				    "sockw/r", POLLOUT, SSH_CHAN_IO_SOCK_W);
 			}
-			p++;
+			dump_channel_poll(__func__, "rfd", c, p, pfd);
 		}
 		/* wfd */
-		if (c->wfd != -1 && c->wfd != c->rfd) {
-			fd_ready(c, p, pfd, c->wfd, "wfd", POLLOUT,
-			    SSH_CHAN_IO_WFD);
-			p++;
+		if (c->wfd != -1 && c->wfd != c->rfd &&
+		    (p = c->pfds[1]) != -1) {
+			fd_ready(c, p, pfd, npfd, c->wfd,
+			    "wfd", POLLOUT, SSH_CHAN_IO_WFD);
+			dump_channel_poll(__func__, "wfd", c, p, pfd);
 		}
 		/* efd */
-		if (c->efd != -1 && c->efd != c->rfd) {
-			fd_ready(c, p, pfd, c->efd, "efdr", POLLIN,
-			    SSH_CHAN_IO_EFD_R);
-			fd_ready(c, p, pfd, c->efd, "efdw", POLLOUT,
-			    SSH_CHAN_IO_EFD_W);
-			p++;
+		if (c->efd != -1 && c->efd != c->rfd &&
+		    (p = c->pfds[2]) != -1) {
+			fd_ready(c, p, pfd, npfd, c->efd,
+			    "efdr", POLLIN, SSH_CHAN_IO_EFD_R);
+			fd_ready(c, p, pfd, npfd, c->efd,
+			    "efdw", POLLOUT, SSH_CHAN_IO_EFD_W);
+			dump_channel_poll(__func__, "efd", c, p, pfd);
 		}
 		/* sock */
-		if (c->sock != -1 && c->sock != c->rfd) {
-			fd_ready(c, p, pfd, c->sock, "sockr", POLLIN,
-			    SSH_CHAN_IO_SOCK_R);
-			fd_ready(c, p, pfd, c->sock, "sockw", POLLOUT,
-			    SSH_CHAN_IO_SOCK_W);
-			p++;
-		}
-
-		if (p > npfd) {
-			/* shouldn't happen */
-			fatal_f("channel %d: (after) bad pfd %u (max %u)",
-			    c->self, c->pollfd_offset, npfd);
+		if (c->sock != -1 && c->sock != c->rfd &&
+		    (p = c->pfds[3]) != -1) {
+			fd_ready(c, p, pfd, npfd, c->sock,
+			    "sockr", POLLIN, SSH_CHAN_IO_SOCK_R);
+			fd_ready(c, p, pfd, npfd, c->sock,
+			    "sockw", POLLOUT, SSH_CHAN_IO_SOCK_W);
+			dump_channel_poll(__func__, "sock", c, p, pfd);
 		}
 	}
 	channel_handler(ssh, CHAN_POST, NULL);
diff --git a/usr.bin/ssh/channels.h b/usr.bin/ssh/channels.h
index b515f83cb9f..871fda0a8f4 100644
--- a/usr.bin/ssh/channels.h
+++ b/usr.bin/ssh/channels.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: channels.h,v 1.141 2022/01/22 00:49:34 djm Exp $ */
+/* $OpenBSD: channels.h,v 1.142 2022/03/30 21:10:25 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -138,7 +138,7 @@ struct Channel {
 	int     sock;		/* sock fd */
 	u_int	io_want;	/* bitmask of SSH_CHAN_IO_* */
 	u_int	io_ready;	/* bitmask of SSH_CHAN_IO_* */
-	int	pollfd_offset;	/* base offset into pollfd array (or -1) */
+	int	pfds[4];	/* pollfd entries for rfd/wfd/efd/sock */
 	int     ctl_chan;	/* control channel (multiplexed connections) */
 	int     isatty;		/* rfd is a tty */
 	int	client_tty;	/* (client) TTY has been requested */