upstream: 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@

OpenBSD-Commit-ID: 06740737849c9047785622ad5d472cb6a3907d10
This commit is contained in:
djm@openbsd.org 2022-03-30 21:10:25 +00:00 committed by Damien Miller
parent 8a74a96d25
commit d6556de1db
2 changed files with 118 additions and 104 deletions

View File

@ -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> * Author: Tatu Ylonen <ylo@cs.hut.fi>
* Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@ -432,21 +432,25 @@ channel_close_fd(struct ssh *ssh, Channel *c, int *fdp)
c->io_want &= ~SSH_CHAN_IO_RFD; c->io_want &= ~SSH_CHAN_IO_RFD;
c->io_ready &= ~SSH_CHAN_IO_RFD; c->io_ready &= ~SSH_CHAN_IO_RFD;
c->rfd = -1; c->rfd = -1;
c->pfds[0] = -1;
} }
if (*fdp == c->wfd) { if (*fdp == c->wfd) {
c->io_want &= ~SSH_CHAN_IO_WFD; c->io_want &= ~SSH_CHAN_IO_WFD;
c->io_ready &= ~SSH_CHAN_IO_WFD; c->io_ready &= ~SSH_CHAN_IO_WFD;
c->wfd = -1; c->wfd = -1;
c->pfds[1] = -1;
} }
if (*fdp == c->efd) { if (*fdp == c->efd) {
c->io_want &= ~SSH_CHAN_IO_EFD; c->io_want &= ~SSH_CHAN_IO_EFD;
c->io_ready &= ~SSH_CHAN_IO_EFD; c->io_ready &= ~SSH_CHAN_IO_EFD;
c->efd = -1; c->efd = -1;
c->pfds[2] = -1;
} }
if (*fdp == c->sock) { if (*fdp == c->sock) {
c->io_want &= ~SSH_CHAN_IO_SOCK; c->io_want &= ~SSH_CHAN_IO_SOCK;
c->io_ready &= ~SSH_CHAN_IO_SOCK; c->io_ready &= ~SSH_CHAN_IO_SOCK;
c->sock = -1; c->sock = -1;
c->pfds[3] = -1;
} }
ret = close(fd); ret = close(fd);
@ -2475,10 +2479,13 @@ dump_channel_poll(const char *func, const char *what, Channel *c,
u_int pollfd_offset, struct pollfd *pfd) u_int pollfd_offset, struct pollfd *pfd)
{ {
#ifdef DEBUG_CHANNEL_POLL #ifdef DEBUG_CHANNEL_POLL
debug3("%s: channel %d: rfd r%d w%d e%d s%d pfd[%u].fd=%d " debug3("%s: channel %d: %s r%d w%d e%d s%d c->pfds [ %d %d %d %d ] "
"io_want 0x%02x pfd.ev 0x%02x io_ready 0x%02x pfd.rev 0x%02x", "io_want 0x%02x io_ready 0x%02x pfd[%u].fd=%d "
func, c->self, c->rfd, c->wfd, c->efd, c->sock, pollfd_offset, "pfd.ev 0x%02x pfd.rev 0x%02x", func, c->self, what,
pfd->fd, c->io_want, pfd->events, c->io_ready, pfd->revents); 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 #endif
} }
@ -2487,7 +2494,7 @@ static void
channel_prepare_pollfd(Channel *c, u_int *next_pollfd, channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
struct pollfd *pfd, u_int npfd) struct pollfd *pfd, u_int npfd)
{ {
u_int p = *next_pollfd; u_int ev, p = *next_pollfd;
if (c == NULL) if (c == NULL)
return; return;
@ -2496,7 +2503,7 @@ channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
fatal_f("channel %d: bad pfd offset %u (max %u)", fatal_f("channel %d: bad pfd offset %u (max %u)",
c->self, p, npfd); c->self, p, npfd);
} }
c->pollfd_offset = -1; c->pfds[0] = c->pfds[1] = c->pfds[2] = c->pfds[3] = -1;
/* /*
* prepare c->rfd * prepare c->rfd
* *
@ -2505,69 +2512,82 @@ channel_prepare_pollfd(Channel *c, u_int *next_pollfd,
* IO too. * IO too.
*/ */
if (c->rfd != -1) { if (c->rfd != -1) {
if (c->pollfd_offset == -1) ev = 0;
c->pollfd_offset = p;
pfd[p].fd = c->rfd;
pfd[p].events = 0;
if ((c->io_want & SSH_CHAN_IO_RFD) != 0) if ((c->io_want & SSH_CHAN_IO_RFD) != 0)
pfd[p].events |= POLLIN; ev |= POLLIN;
/* rfd == wfd */ /* rfd == wfd */
if (c->wfd == c->rfd && if (c->wfd == c->rfd) {
(c->io_want & SSH_CHAN_IO_WFD) != 0) if ((c->io_want & SSH_CHAN_IO_WFD) != 0)
pfd[p].events |= POLLOUT; ev |= POLLOUT;
}
/* rfd == efd */ /* rfd == efd */
if (c->efd == c->rfd && if (c->efd == c->rfd) {
(c->io_want & SSH_CHAN_IO_EFD_R) != 0) if ((c->io_want & SSH_CHAN_IO_EFD_R) != 0)
pfd[p].events |= POLLIN; ev |= POLLIN;
if (c->efd == c->rfd && if ((c->io_want & SSH_CHAN_IO_EFD_W) != 0)
(c->io_want & SSH_CHAN_IO_EFD_W) != 0) ev |= POLLOUT;
pfd[p].events |= POLLOUT; }
/* rfd == sock */ /* rfd == sock */
if (c->sock == c->rfd && if (c->sock == c->rfd) {
(c->io_want & SSH_CHAN_IO_SOCK_R) != 0) if ((c->io_want & SSH_CHAN_IO_SOCK_R) != 0)
pfd[p].events |= POLLIN; ev |= POLLIN;
if (c->sock == c->rfd && if ((c->io_want & SSH_CHAN_IO_SOCK_W) != 0)
(c->io_want & SSH_CHAN_IO_SOCK_W) != 0) ev |= POLLOUT;
pfd[p].events |= POLLOUT; }
dump_channel_poll(__func__, "rfd", c, p, &pfd[p]); /* Pack a pfd entry if any event armed for this fd */
p++; 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 not already handled above) */ /* prepare c->wfd if wanting IO and not already handled above */
if (c->wfd != -1 && c->rfd != c->wfd) { if (c->wfd != -1 && c->rfd != c->wfd) {
if (c->pollfd_offset == -1) ev = 0;
c->pollfd_offset = p; if ((c->io_want & SSH_CHAN_IO_WFD))
pfd[p].fd = c->wfd; ev |= POLLOUT;
pfd[p].events = 0; /* Pack a pfd entry if any event armed for this fd */
if ((c->io_want & SSH_CHAN_IO_WFD) != 0) if (ev != 0) {
pfd[p].events = POLLOUT; c->pfds[1] = p;
dump_channel_poll(__func__, "wfd", c, p, &pfd[p]); pfd[p].fd = c->wfd;
p++; pfd[p].events = ev;
dump_channel_poll(__func__, "wfd", c, p, &pfd[p]);
p++;
}
} }
/* prepare c->efd (if not already handled above) */ /* prepare c->efd if wanting IO and not already handled above */
if (c->efd != -1 && c->rfd != c->efd) { if (c->efd != -1 && c->rfd != c->efd) {
if (c->pollfd_offset == -1) ev = 0;
c->pollfd_offset = p;
pfd[p].fd = c->efd;
pfd[p].events = 0;
if ((c->io_want & SSH_CHAN_IO_EFD_R) != 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) if ((c->io_want & SSH_CHAN_IO_EFD_W) != 0)
pfd[p].events |= POLLOUT; ev |= POLLOUT;
dump_channel_poll(__func__, "efd", c, p, &pfd[p]); /* Pack a pfd entry if any event armed for this fd */
p++; 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->sock != -1 && c->rfd != c->sock) {
if (c->pollfd_offset == -1) ev = 0;
c->pollfd_offset = p;
pfd[p].fd = c->sock;
pfd[p].events = 0;
if ((c->io_want & SSH_CHAN_IO_SOCK_R) != 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) if ((c->io_want & SSH_CHAN_IO_SOCK_W) != 0)
pfd[p].events |= POLLOUT; ev |= POLLOUT;
dump_channel_poll(__func__, "sock", c, p, &pfd[p]); /* Pack a pfd entry if any event armed for this fd */
p++; 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; *next_pollfd = p;
} }
@ -2614,13 +2634,15 @@ channel_prepare_poll(struct ssh *ssh, struct pollfd **pfdp, u_int *npfd_allocp,
} }
static void 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) const char *what, u_int revents_mask, u_int ready)
{ {
struct pollfd *pfd = &pfds[p]; struct pollfd *pfd = &pfds[p];
if (fd == -1) if (fd == -1)
return; 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); dump_channel_poll(__func__, what, c, p, pfd);
if (pfd->fd != fd) { if (pfd->fd != fd) {
fatal("channel %d: inconsistent %s fd=%d pollfd[%u].fd %d " fatal("channel %d: inconsistent %s fd=%d pollfd[%u].fd %d "
@ -2643,11 +2665,12 @@ void
channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd) channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
{ {
struct ssh_channels *sc = ssh->chanctxt; struct ssh_channels *sc = ssh->chanctxt;
u_int i, p; u_int i;
int p;
Channel *c; Channel *c;
#ifdef DEBUG_CHANNEL_POLL #ifdef DEBUG_CHANNEL_POLL
for (p = 0; p < npfd; p++) { for (p = 0; p < (int)npfd; p++) {
if (pfd[p].revents == 0) if (pfd[p].revents == 0)
continue; continue;
debug_f("pfd[%u].fd %d rev 0x%04x", debug_f("pfd[%u].fd %d rev 0x%04x",
@ -2658,13 +2681,8 @@ channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
/* Convert pollfd into c->io_ready */ /* Convert pollfd into c->io_ready */
for (i = 0; i < sc->channels_alloc; i++) { for (i = 0; i < sc->channels_alloc; i++) {
c = sc->channels[i]; c = sc->channels[i];
if (c == NULL || c->pollfd_offset < 0) if (c == NULL)
continue; 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 rfd is shared with efd/sock then wfd should be too */
if (c->rfd != -1 && c->wfd != -1 && c->rfd != c->wfd && if (c->rfd != -1 && c->wfd != -1 && c->rfd != c->wfd &&
(c->rfd == c->efd || c->rfd == c->sock)) { (c->rfd == c->efd || c->rfd == c->sock)) {
@ -2673,56 +2691,52 @@ channel_after_poll(struct ssh *ssh, struct pollfd *pfd, u_int npfd)
c->self, c->rfd, c->wfd, c->efd, c->sock); c->self, c->rfd, c->wfd, c->efd, c->sock);
} }
c->io_ready = 0; c->io_ready = 0;
p = c->pollfd_offset;
/* rfd, potentially shared with wfd, efd and sock */ /* rfd, potentially shared with wfd, efd and sock */
if (c->rfd != -1) { if (c->rfd != -1 && (p = c->pfds[0]) != -1) {
fd_ready(c, p, pfd, c->rfd, "rfd", POLLIN, fd_ready(c, p, pfd, npfd, c->rfd,
SSH_CHAN_IO_RFD); "rfd", POLLIN, SSH_CHAN_IO_RFD);
if (c->rfd == c->wfd) { if (c->rfd == c->wfd) {
fd_ready(c, p, pfd, c->wfd, "wfd/r", POLLOUT, fd_ready(c, p, pfd, npfd, c->wfd,
SSH_CHAN_IO_WFD); "wfd/r", POLLOUT, SSH_CHAN_IO_WFD);
} }
if (c->rfd == c->efd) { if (c->rfd == c->efd) {
fd_ready(c, p, pfd, c->efd, "efdr/r", POLLIN, fd_ready(c, p, pfd, npfd, c->efd,
SSH_CHAN_IO_EFD_R); "efdr/r", POLLIN, SSH_CHAN_IO_EFD_R);
fd_ready(c, p, pfd, c->efd, "efdw/r", POLLOUT, fd_ready(c, p, pfd, npfd, c->efd,
SSH_CHAN_IO_EFD_W); "efdw/r", POLLOUT, SSH_CHAN_IO_EFD_W);
} }
if (c->rfd == c->sock) { if (c->rfd == c->sock) {
fd_ready(c, p, pfd, c->sock, "sockr/r", POLLIN, fd_ready(c, p, pfd, npfd, c->sock,
SSH_CHAN_IO_SOCK_R); "sockr/r", POLLIN, SSH_CHAN_IO_SOCK_R);
fd_ready(c, p, pfd, c->sock, "sockw/r", POLLOUT, fd_ready(c, p, pfd, npfd, c->sock,
SSH_CHAN_IO_SOCK_W); "sockw/r", POLLOUT, SSH_CHAN_IO_SOCK_W);
} }
p++; dump_channel_poll(__func__, "rfd", c, p, pfd);
} }
/* wfd */ /* wfd */
if (c->wfd != -1 && c->wfd != c->rfd) { if (c->wfd != -1 && c->wfd != c->rfd &&
fd_ready(c, p, pfd, c->wfd, "wfd", POLLOUT, (p = c->pfds[1]) != -1) {
SSH_CHAN_IO_WFD); fd_ready(c, p, pfd, npfd, c->wfd,
p++; "wfd", POLLOUT, SSH_CHAN_IO_WFD);
dump_channel_poll(__func__, "wfd", c, p, pfd);
} }
/* efd */ /* efd */
if (c->efd != -1 && c->efd != c->rfd) { if (c->efd != -1 && c->efd != c->rfd &&
fd_ready(c, p, pfd, c->efd, "efdr", POLLIN, (p = c->pfds[2]) != -1) {
SSH_CHAN_IO_EFD_R); fd_ready(c, p, pfd, npfd, c->efd,
fd_ready(c, p, pfd, c->efd, "efdw", POLLOUT, "efdr", POLLIN, SSH_CHAN_IO_EFD_R);
SSH_CHAN_IO_EFD_W); fd_ready(c, p, pfd, npfd, c->efd,
p++; "efdw", POLLOUT, SSH_CHAN_IO_EFD_W);
dump_channel_poll(__func__, "efd", c, p, pfd);
} }
/* sock */ /* sock */
if (c->sock != -1 && c->sock != c->rfd) { if (c->sock != -1 && c->sock != c->rfd &&
fd_ready(c, p, pfd, c->sock, "sockr", POLLIN, (p = c->pfds[3]) != -1) {
SSH_CHAN_IO_SOCK_R); fd_ready(c, p, pfd, npfd, c->sock,
fd_ready(c, p, pfd, c->sock, "sockw", POLLOUT, "sockr", POLLIN, SSH_CHAN_IO_SOCK_R);
SSH_CHAN_IO_SOCK_W); fd_ready(c, p, pfd, npfd, c->sock,
p++; "sockw", POLLOUT, SSH_CHAN_IO_SOCK_W);
} dump_channel_poll(__func__, "sock", c, p, pfd);
if (p > npfd) {
/* shouldn't happen */
fatal_f("channel %d: (after) bad pfd %u (max %u)",
c->self, c->pollfd_offset, npfd);
} }
} }
channel_handler(ssh, CHAN_POST, NULL); channel_handler(ssh, CHAN_POST, NULL);

View File

@ -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> * Author: Tatu Ylonen <ylo@cs.hut.fi>
@ -138,7 +138,7 @@ struct Channel {
int sock; /* sock fd */ int sock; /* sock fd */
u_int io_want; /* bitmask of SSH_CHAN_IO_* */ u_int io_want; /* bitmask of SSH_CHAN_IO_* */
u_int io_ready; /* 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 ctl_chan; /* control channel (multiplexed connections) */
int isatty; /* rfd is a tty */ int isatty; /* rfd is a tty */
#ifdef _AIX #ifdef _AIX