Attachment 3519 Details for Bug 3280 – restore non-blocking status for stdio file descriptors

[patch] restore non-blocking status for stdio file descriptors

bz3280.diff (text/plain), 11.79 KB, created by Damien Miller on 2021-05-14 15:06:59 AEST

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2021-05-14 15:06:59 AEST

Size: 11.79 KB

patch

obsolete

>commit e9756bb6e8df9eb99ebed6a4a3d98aaaf37c62c1
>Author: Damien Miller <djm@mindrot.org>
>Date:   Fri May 14 14:52:30 2021 +1000
>
>    restore blocking status for non-TTY stdio fds
>
>diff --git a/channels.c b/channels.c
>index ebeb829..b11c65d 100644
>--- a/channels.c
>+++ b/channels.c
>@@ -300,6 +300,7 @@ channel_register_fds(struct ssh *ssh, Channel *c, int rfd, int wfd, int efd,
>     int extusage, int nonblock, int is_tty)
> {
> 	struct ssh_channels *sc = ssh->chanctxt;
>+	int nb;
> 
> 	/* Update the maximum file descriptor value. */
> 	sc->channel_max_fd = MAXIMUM(sc->channel_max_fd, rfd);
>@@ -323,7 +324,30 @@ channel_register_fds(struct ssh *ssh, Channel *c, int rfd, int wfd, int efd,
> 		debug2("channel %d: rfd %d isatty", c->self, c->rfd);
> 
> 	/* enable nonblocking mode */
>-	if (nonblock) {
>+	c->restore_block = 0;
>+	if (nonblock == CHANNEL_NONBLOCK_STDIO) {
>+		/*
>+		 * Special handling for stdio file descriptors: do not set
>+		 * non-blocking mode if they are TTYs. Otherwise prepare to
>+		 * restore their blocking state on exit to avoid interfering
>+		 * with other programs that follow.
>+		 */
>+		if (rfd != -1 && !isatty(rfd) &&
>+		    (nb = fcntl(rfd, F_GETFL)) == 0) {
>+			c->restore_block |= 1;
>+			set_nonblock(rfd);
>+		}
>+		if (wfd != -1 && !isatty(wfd) &&
>+		    (nb = fcntl(wfd, F_GETFL)) != -1) {
>+			c->restore_block |= 1<<1;
>+			set_nonblock(wfd);
>+		}
>+		if (efd != -1 && !isatty(efd) &&
>+		    (nb = fcntl(efd, F_GETFL)) != -1) {
>+			c->restore_block |= 1<<2;
>+			set_nonblock(efd);
>+		}
>+	} else if (nonblock) {
> 		if (rfd != -1)
> 			set_nonblock(rfd);
> 		if (wfd != -1)
>@@ -378,6 +402,7 @@ channel_new(struct ssh *ssh, char *ctype, int type, int rfd, int wfd, int efd,
> 		fatal_fr(r, "sshbuf_set_max_size");
> 	c->ostate = CHAN_OUTPUT_OPEN;
> 	c->istate = CHAN_INPUT_OPEN;
>+	c->restore_block = 0;
> 	channel_register_fds(ssh, c, rfd, wfd, efd, extusage, nonblock, 0);
> 	c->self = found;
> 	c->type = type;
>@@ -412,17 +437,23 @@ channel_find_maxfd(struct ssh_channels *sc)
> }
> 
> int
>-channel_close_fd(struct ssh *ssh, int *fdp)
>+channel_close_fd(struct ssh *ssh, Channel *c, int *fdp)
> {
> 	struct ssh_channels *sc = ssh->chanctxt;
>-	int ret = 0, fd = *fdp;
>+	int ret, fd = *fdp;
> 
>-	if (fd != -1) {
>-		ret = close(fd);
>-		*fdp = -1;
>-		if (fd == sc->channel_max_fd)
>-			channel_find_maxfd(sc);
>-	}
>+	if (fd == -1)
>+		return 0;
>+
>+	if ((*fdp == c->rfd && (c->restore_block & 1) != 0) ||
>+	   (*fdp == c->wfd && (c->restore_block & (1<<1)) != 0) ||
>+	   (*fdp == c->efd && (c->restore_block & (1<<2)) != 0))
>+		(void)fcntl(*fdp, F_SETFL, 0);	/* restore blocking */
>+
>+	ret = close(fd);
>+	*fdp = -1;
>+	if (fd == sc->channel_max_fd)
>+		channel_find_maxfd(sc);
> 	return ret;
> }
> 
>@@ -432,13 +463,13 @@ channel_close_fds(struct ssh *ssh, Channel *c)
> {
> 	int sock = c->sock, rfd = c->rfd, wfd = c->wfd, efd = c->efd;
> 
>-	channel_close_fd(ssh, &c->sock);
>+	channel_close_fd(ssh, c, &c->sock);
> 	if (rfd != sock)
>-		channel_close_fd(ssh, &c->rfd);
>+		channel_close_fd(ssh, c, &c->rfd);
> 	if (wfd != sock && wfd != rfd)
>-		channel_close_fd(ssh, &c->wfd);
>+		channel_close_fd(ssh, c, &c->wfd);
> 	if (efd != sock && efd != rfd && efd != wfd)
>-		channel_close_fd(ssh, &c->efd);
>+		channel_close_fd(ssh, c, &c->efd);
> }
> 
> static void
>@@ -692,7 +723,7 @@ channel_stop_listening(struct ssh *ssh)
> 			case SSH_CHANNEL_X11_LISTENER:
> 			case SSH_CHANNEL_UNIX_LISTENER:
> 			case SSH_CHANNEL_RUNIX_LISTENER:
>-				channel_close_fd(ssh, &c->sock);
>+				channel_close_fd(ssh, c, &c->sock);
> 				channel_free(ssh, c);
> 				break;
> 			}
>@@ -1481,7 +1512,8 @@ channel_decode_socks5(Channel *c, struct sshbuf *input, struct sshbuf *output)
> 
> Channel *
> channel_connect_stdio_fwd(struct ssh *ssh,
>-    const char *host_to_connect, u_short port_to_connect, int in, int out)
>+    const char *host_to_connect, u_short port_to_connect,
>+    int in, int out, int nonblock)
> {
> 	Channel *c;
> 
>@@ -1489,7 +1521,7 @@ channel_connect_stdio_fwd(struct ssh *ssh,
> 
> 	c = channel_new(ssh, "stdio-forward", SSH_CHANNEL_OPENING, in, out,
> 	    -1, CHAN_TCP_WINDOW_DEFAULT, CHAN_TCP_PACKET_DEFAULT,
>-	    0, "stdio-forward", /*nonblock*/0);
>+	    0, "stdio-forward", nonblock);
> 
> 	c->path = xstrdup(host_to_connect);
> 	c->host_port = port_to_connect;
>@@ -1639,7 +1671,7 @@ channel_post_x11_listener(struct ssh *ssh, Channel *c,
> 	if (c->single_connection) {
> 		oerrno = errno;
> 		debug2("single_connection: closing X11 listener.");
>-		channel_close_fd(ssh, &c->sock);
>+		channel_close_fd(ssh, c, &c->sock);
> 		chan_mark_dead(ssh, c);
> 		errno = oerrno;
> 	}
>@@ -2028,7 +2060,7 @@ channel_handle_efd_write(struct ssh *ssh, Channel *c,
> 		return 1;
> 	if (len <= 0) {
> 		debug2("channel %d: closing write-efd %d", c->self, c->efd);
>-		channel_close_fd(ssh, &c->efd);
>+		channel_close_fd(ssh, c, &c->efd);
> 	} else {
> 		if ((r = sshbuf_consume(c->extended, len)) != 0)
> 			fatal_fr(r, "channel %i: consume", c->self);
>@@ -2054,7 +2086,7 @@ channel_handle_efd_read(struct ssh *ssh, Channel *c,
> 		return 1;
> 	if (len <= 0) {
> 		debug2("channel %d: closing read-efd %d", c->self, c->efd);
>-		channel_close_fd(ssh, &c->efd);
>+		channel_close_fd(ssh, c, &c->efd);
> 	} else if (c->extended_usage == CHAN_EXTENDED_IGNORE)
> 		debug3("channel %d: discard efd", c->self);
> 	else if ((r = sshbuf_put(c->extended, buf, len)) != 0)
>diff --git a/channels.h b/channels.h
>index b318974..a58c7ab 100644
>--- a/channels.h
>+++ b/channels.h
>@@ -63,6 +63,11 @@
> 
> #define CHANNEL_CANCEL_PORT_STATIC	-1
> 
>+/* nonblocking flags for channel_new */
>+#define CHANNEL_NONBLOCK_LEAVE	0 /* don't modify non-blocking state */
>+#define CHANNEL_NONBLOCK_SET	1 /* set non-blocking state */
>+#define CHANNEL_NONBLOCK_STDIO	2 /* set non-blocking and restore on close */
>+
> /* TCP forwarding */
> #define FORWARD_DENY		0
> #define FORWARD_REMOTE		(1)
>@@ -136,6 +141,7 @@ struct Channel {
> 				 * to a matching pre-select handler.
> 				 * this way post-select handlers are not
> 				 * accidentally called if a FD gets reused */
>+	int	restore_block;	/* fd mask to restore blocking status */
> 	struct sshbuf *input;	/* data read from socket, to be sent over
> 				 * encrypted connection */
> 	struct sshbuf *output;	/* data received over encrypted connection for
>@@ -263,7 +269,7 @@ void	 channel_register_filter(struct ssh *, int, channel_infilter_fn *,
> void	 channel_register_status_confirm(struct ssh *, int,
> 	    channel_confirm_cb *, channel_confirm_abandon_cb *, void *);
> void	 channel_cancel_cleanup(struct ssh *, int);
>-int	 channel_close_fd(struct ssh *, int *);
>+int	 channel_close_fd(struct ssh *, Channel *, int *);
> void	 channel_send_window_changes(struct ssh *);
> 
> /* mux proxy support */
>@@ -310,7 +316,7 @@ Channel	*channel_connect_to_port(struct ssh *, const char *, u_short,
> 	    char *, char *, int *, const char **);
> Channel *channel_connect_to_path(struct ssh *, const char *, char *, char *);
> Channel	*channel_connect_stdio_fwd(struct ssh *, const char*,
>-	    u_short, int, int);
>+	    u_short, int, int, int);
> Channel	*channel_connect_by_listen_address(struct ssh *, const char *,
> 	    u_short, char *, char *);
> Channel	*channel_connect_by_listen_path(struct ssh *, const char *,
>diff --git a/clientloop.c b/clientloop.c
>index 2c17cfc..c4b8451 100644
>--- a/clientloop.c
>+++ b/clientloop.c
>@@ -1397,14 +1397,6 @@ client_loop(struct ssh *ssh, int have_pty, int escape_char_arg,
> 	if (have_pty)
> 		leave_raw_mode(options.request_tty == REQUEST_TTY_FORCE);
> 
>-	/* restore blocking io */
>-	if (!isatty(fileno(stdin)))
>-		unset_nonblock(fileno(stdin));
>-	if (!isatty(fileno(stdout)))
>-		unset_nonblock(fileno(stdout));
>-	if (!isatty(fileno(stderr)))
>-		unset_nonblock(fileno(stderr));
>-
> 	/*
> 	 * If there was no shell or command requested, there will be no remote
> 	 * exit status to be returned.  In that case, clear error code if the
>diff --git a/mux.c b/mux.c
>index 55dad8c..6c8b2a7 100644
>--- a/mux.c
>+++ b/mux.c
>@@ -439,14 +439,6 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid,
> 	if (cctx->want_tty && tcgetattr(new_fd[0], &cctx->tio) == -1)
> 		error_f("tcgetattr: %s", strerror(errno));
> 
>-	/* enable nonblocking unless tty */
>-	if (!isatty(new_fd[0]))
>-		set_nonblock(new_fd[0]);
>-	if (!isatty(new_fd[1]))
>-		set_nonblock(new_fd[1]);
>-	if (!isatty(new_fd[2]))
>-		set_nonblock(new_fd[2]);
>-
> 	window = CHAN_SES_WINDOW_DEFAULT;
> 	packetmax = CHAN_SES_PACKET_DEFAULT;
> 	if (cctx->want_tty) {
>@@ -456,7 +448,7 @@ mux_master_process_new_session(struct ssh *ssh, u_int rid,
> 
> 	nc = channel_new(ssh, "session", SSH_CHANNEL_OPENING,
> 	    new_fd[0], new_fd[1], new_fd[2], window, packetmax,
>-	    CHAN_EXTENDED_WRITE, "client-session", /*nonblock*/0);
>+	    CHAN_EXTENDED_WRITE, "client-session", CHANNEL_NONBLOCK_STDIO);
> 
> 	nc->ctl_chan = c->self;		/* link session -> control channel */
> 	c->remote_id = nc->self;	/* link control -> session channel */
>@@ -1012,13 +1004,8 @@ mux_master_process_stdio_fwd(struct ssh *ssh, u_int rid,
> 		}
> 	}
> 
>-	/* enable nonblocking unless tty */
>-	if (!isatty(new_fd[0]))
>-		set_nonblock(new_fd[0]);
>-	if (!isatty(new_fd[1]))
>-		set_nonblock(new_fd[1]);
>-
>-	nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1]);
>+	nc = channel_connect_stdio_fwd(ssh, chost, cport, new_fd[0], new_fd[1],
>+	    CHANNEL_NONBLOCK_STDIO);
> 	free(chost);
> 
> 	nc->ctl_chan = c->self;		/* link session -> control channel */
>diff --git a/nchan.c b/nchan.c
>index a775b71..76d1588 100644
>--- a/nchan.c
>+++ b/nchan.c
>@@ -382,7 +382,7 @@ chan_shutdown_write(struct ssh *ssh, Channel *c)
> 			    c->istate, c->ostate, strerror(errno));
> 		}
> 	} else {
>-		if (channel_close_fd(ssh, &c->wfd) < 0) {
>+		if (channel_close_fd(ssh, c, &c->wfd) < 0) {
> 			logit_f("channel %d: close() failed for "
> 			    "fd %d [i%d o%d]: %.100s", c->self, c->wfd,
> 			    c->istate, c->ostate, strerror(errno));
>@@ -405,7 +405,7 @@ chan_shutdown_read(struct ssh *ssh, Channel *c)
> 			    c->istate, c->ostate, strerror(errno));
> 		}
> 	} else {
>-		if (channel_close_fd(ssh, &c->rfd) < 0) {
>+		if (channel_close_fd(ssh, c, &c->rfd) < 0) {
> 			logit_f("channel %d: close() failed for "
> 			    "fd %d [i%d o%d]: %.100s", c->self, c->rfd,
> 			    c->istate, c->ostate, strerror(errno));
>@@ -424,7 +424,7 @@ chan_shutdown_extended_read(struct ssh *ssh, Channel *c)
> 	debug_f("channel %d: (i%d o%d sock %d wfd %d efd %d [%s])",
> 	    c->self, c->istate, c->ostate, c->sock, c->rfd, c->efd,
> 	    channel_format_extended_usage(c));
>-	if (channel_close_fd(ssh, &c->efd) < 0) {
>+	if (channel_close_fd(ssh, c, &c->efd) < 0) {
> 		logit_f("channel %d: close() failed for "
> 		    "extended fd %d [i%d o%d]: %.100s", c->self, c->efd,
> 		    c->istate, c->ostate, strerror(errno));
>diff --git a/ssh.c b/ssh.c
>index 610d8c8..32c5a04 100644
>--- a/ssh.c
>+++ b/ssh.c
>@@ -1855,9 +1855,10 @@ ssh_init_stdio_forwarding(struct ssh *ssh)
> 
> 	if ((in = dup(STDIN_FILENO)) == -1 ||
> 	    (out = dup(STDOUT_FILENO)) == -1)
>-		fatal("channel_connect_stdio_fwd: dup() in/out failed");
>+		fatal_f("dup() in/out failed");
> 	if ((c = channel_connect_stdio_fwd(ssh, options.stdio_forward_host,
>-	    options.stdio_forward_port, in, out)) == NULL)
>+	    options.stdio_forward_port, in, out,
>+	    CHANNEL_NONBLOCK_STDIO)) == NULL)
> 		fatal_f("channel_connect_stdio_fwd failed");
> 	channel_register_cleanup(ssh, c->self, client_cleanup_stdio_fwd, 0);
> 	channel_register_open_confirm(ssh, c->self, ssh_stdio_confirm, NULL);
>@@ -2053,14 +2054,6 @@ ssh_session2_open(struct ssh *ssh)
> 	if (in == -1 || out == -1 || err == -1)
> 		fatal("dup() in/out/err failed");
> 
>-	/* enable nonblocking unless tty */
>-	if (!isatty(in))
>-		set_nonblock(in);
>-	if (!isatty(out))
>-		set_nonblock(out);
>-	if (!isatty(err))
>-		set_nonblock(err);
>-
> 	window = CHAN_SES_WINDOW_DEFAULT;
> 	packetmax = CHAN_SES_PACKET_DEFAULT;
> 	if (tty_flag) {
>@@ -2070,7 +2063,7 @@ ssh_session2_open(struct ssh *ssh)
> 	c = channel_new(ssh,
> 	    "session", SSH_CHANNEL_OPENING, in, out, err,
> 	    window, packetmax, CHAN_EXTENDED_WRITE,
>-	    "client-session", /*nonblock*/0);
>+	    "client-session", CHANNEL_NONBLOCK_STDIO);
> 
> 	debug3_f("channel_new: %d", c->self);
>

Flags:

dtucker: ok+

Actions: View | Diff

Attachments on bug 3280: 3510 | 3511 | 3519