sshd gets stuck: select() in packet_read_seqnr waits indefinitely

[Darren Tucker]

On Thu, Mar 15, 2007 at 02:22:13PM +1100, Darren Tucker wrote:
> On Wed, Mar 14, 2007 at 08:59:04PM -0600, Matt Day wrote:
> > What about the timeout handling in packet_write_wait? Do you really
> > want to proceed with the write() and then possibly loop back around
> > to the select() call?
> 
> Yeah, that's a problem.  On platforms that return EAGAIN rather than
> EWOULDBLOCK it'll just spin through packet_write_poll() repeatedly.
> 
> Probably best to capture the return from select() explicitly test for
> a timeout.

Like so (still untested):

Index: packet.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/packet.c,v
retrieving revision 1.146
diff -u -p -r1.146 packet.c
--- packet.c	17 Jan 2007 00:00:14 -0000	1.146
+++ packet.c	15 Mar 2007 05:51:19 -0000
@@ -136,6 +136,10 @@ static int server_side = 0;
 /* Set to true if we are authenticated. */
 static int after_authentication = 0;
 
+/* Set to the maximum time that we will wait to send or receive a packet */
+static struct timeval packet_wait_tv;
+static struct timeval *packet_wait_tvp = NULL;
+
 /* Session key information for Encryption and MAC */
 Newkeys *newkeys[MODE_MAX];
 static struct packet_state {
@@ -189,6 +193,21 @@ packet_set_connection(int fd_in, int fd_
 	}
 }
 
+void
+packet_set_timeout(int timeout, int count)
+{
+	if (timeout == 0 && count == 0) {
+		packet_wait_tvp = NULL;
+		return;
+	}
+	if (LONG_MAX / count < timeout)
+		packet_wait_tv.tv_sec = LONG_MAX;
+	else
+		packet_wait_tv.tv_sec = timeout * count;
+	packet_wait_tv.tv_usec = 0;
+	packet_wait_tvp = &packet_wait_tv;
+}
+
 /* Returns 1 if remote host is connected via socket, 0 if not. */
 
 int
@@ -888,7 +907,7 @@ packet_send(void)
 int
 packet_read_seqnr(u_int32_t *seqnr_p)
 {
-	int type, len;
+	int type, len, ret;
 	fd_set *setp;
 	char buf[8192];
 	DBG(debug("packet_read()"));
@@ -923,10 +942,15 @@ packet_read_seqnr(u_int32_t *seqnr_p)
 		FD_SET(connection_in, setp);
 
 		/* Wait for some data to arrive. */
-		while (select(connection_in + 1, setp, NULL, NULL, NULL) == -1 &&
+		while ((ret = select(connection_in + 1, setp, NULL, NULL,
+		    packet_wait_tvp)) == -1 &&
 		    (errno == EAGAIN || errno == EINTR))
 			;
-
+		if (ret == 0) {
+			logit("Connection to %.200s timed out",
+			    get_remote_ipaddr());
+			cleanup_exit(255);
+		}
 		/* Read data from the socket. */
 		len = read(connection_in, buf, sizeof(buf));
 		if (len == 0) {
@@ -1441,6 +1465,7 @@ void
 packet_write_wait(void)
 {
 	fd_set *setp;
+	int ret;
 
 	setp = (fd_set *)xcalloc(howmany(connection_out + 1, NFDBITS),
 	    sizeof(fd_mask));
@@ -1449,9 +1474,15 @@ packet_write_wait(void)
 		memset(setp, 0, howmany(connection_out + 1, NFDBITS) *
 		    sizeof(fd_mask));
 		FD_SET(connection_out, setp);
-		while (select(connection_out + 1, NULL, setp, NULL, NULL) == -1 &&
+		while ((ret = select(connection_out + 1, NULL, setp, NULL,
+		    packet_wait_tvp)) == -1 &&
 		    (errno == EAGAIN || errno == EINTR))
 			;
+		if (ret == 0) {
+			logit("Connection to %.200s timed out",
+			    get_remote_ipaddr());
+			cleanup_exit(255);
+		}
 		packet_write_poll();
 	}
 	xfree(setp);
Index: packet.h
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/packet.h,v
retrieving revision 1.47
diff -u -p -r1.47 packet.h
--- packet.h	26 Mar 2006 03:30:02 -0000	1.47
+++ packet.h	15 Mar 2007 04:01:39 -0000
@@ -21,6 +21,7 @@
 #include <openssl/bn.h>
 
 void     packet_set_connection(int, int);
+void     packet_set_timeout(int, int);
 void     packet_set_nonblocking(void);
 int      packet_get_connection_in(void);
 int      packet_get_connection_out(void);
Index: sshconnect.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/sshconnect.c,v
retrieving revision 1.171
diff -u -p -r1.171 sshconnect.c
--- sshconnect.c	23 Oct 2006 17:02:24 -0000	1.171
+++ sshconnect.c	15 Mar 2007 04:04:50 -0000
@@ -158,6 +158,8 @@ ssh_proxy_connect(const char *host, u_sh
 
 	/* Set the connection file descriptors. */
 	packet_set_connection(pout[0], pin[1]);
+	packet_set_timeout(options.server_alive_interval,
+	    options.server_alive_count_max);
 
 	/* Indicate OK return */
 	return 0;
@@ -386,6 +388,8 @@ ssh_connect(const char *host, struct soc
 
 	/* Set the connection. */
 	packet_set_connection(sock, sock);
+	packet_set_timeout(options.server_alive_interval,
+	    options.server_alive_count_max);
 
 	return 0;
 }
Index: sshd.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/sshd.c,v
retrieving revision 1.362
diff -u -p -r1.362 sshd.c
--- sshd.c	25 Feb 2007 09:37:22 -0000	1.362
+++ sshd.c	15 Mar 2007 03:58:53 -0000
@@ -1708,6 +1708,8 @@ main(int ac, char **av)
 	 * not have a key.
 	 */
 	packet_set_connection(sock_in, sock_out);
+	packet_set_timeout(options.client_alive_interval,
+	    options.client_alive_count_max);
 	packet_set_server();
 
 	/* Set SO_KEEPALIVE if requested. */

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.