sshd gets stuck: select() in packet_read_seqnr waits indefinitely

[Matt Day]

On Thu, Mar 15, 2007 at 04:52:42PM +1100, Darren Tucker wrote:
> Like so (still untested):
> 
> [..]

Awesome, thanks!

I applied the patch and ran into one problem during testing: outbound
ssh connections immediately received the new "timed out" error.

The problem was in packet_set_timeout: packet_wait_tvp was only
getting set to NULL when *both* timeout and count were 0. The default
is interval=0 count=3, so it used the timeval with tv_sec = 0.

I fixed that (use || instead of && in packet_set_timeout) and also
made the two new "timed out" error messages unique. That might be
handy someday if one is wondering if it timed out waiting to read
or to write (which I found myself wondering while debugging this).

I have installed these changes on my server and will let you know
when I get confirmation in my logfile that the fix is working.

Thank you very much!
Matt

===================================================================
diff -u -p packet.c.orig packet.c
--- packet.c.orig	Sun Sep 11 09:50:34 2005
+++ packet.c	Thu Mar 15 01:08:22 2007
@@ -122,6 +122,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 {
@@ -175,6 +179,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
@@ -863,7 +882,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()"));
@@ -898,10 +917,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 while waiting to read",
+			    get_remote_ipaddr());
+			cleanup_exit(255);
+		}
 		/* Read data from the socket. */
 		len = read(connection_in, buf, sizeof(buf));
 		if (len == 0) {
@@ -1411,6 +1435,7 @@ void
 packet_write_wait(void)
 {
 	fd_set *setp;
+	int ret;
 
 	setp = (fd_set *)xmalloc(howmany(connection_out + 1, NFDBITS) *
 	    sizeof(fd_mask));
@@ -1419,9 +1444,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 while waiting to write",
+			    get_remote_ipaddr());
+			cleanup_exit(255);
+		}
 		packet_write_poll();
 	}
 	xfree(setp);
===================================================================
diff -u -p packet.h.orig packet.h
--- packet.h.orig	Sun Sep 11 09:50:34 2005
+++ packet.h	Thu Mar 15 00:05:29 2007
@@ -19,6 +19,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);
===================================================================
diff -u -p sshconnect.c.orig sshconnect.c
--- sshconnect.c.orig	Sun Sep 11 09:50:35 2005
+++ sshconnect.c	Thu Mar 15 00:05:29 2007
@@ -139,6 +139,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;
@@ -381,6 +383,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;
 }
===================================================================
diff -u -p sshd.c.orig sshd.c
--- sshd.c.orig	Sun Sep 11 09:50:35 2005
+++ sshd.c	Thu Mar 15 00:05:29 2007
@@ -1643,6 +1643,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. */