openssh unix dev - Apr 2006 - bug & patch in ServerAliveInterval (openssh 4.3-p2)

Hi openssh-unix-dev subscribers :)

I have found that ServerAliveInterval & ServerAliveCountMax have
some bug. Basically the ssh-alive check function (that verify the
peer is alive) is called only if no data at all gets into ssh (when it 
should work only for server channel).I am pretty sure developers know
about this ..anyway I have tried to fix this issue. Here is the patch:

diff -rNu openssh-4.3p2/clientloop.c openssh-4.3p2-alive-fixed/clientloop.c
--- openssh-4.3p2/clientloop.c  2005-12-31 07:22:32.000000000 +0200
+++ openssh-4.3p2-alive-fixed/clientloop.c      2006-04-22 19:32:17.000000000
+0300
@@ -456,7 +456,7 @@
  client_wait_until_can_do_something(fd_set **readsetp, fd_set **writesetp,
      int *maxfdp, u_int *nallocp, int rekeying)
  {
-       struct timeval tv, *tvp;
+       static struct timeval tv, *tvp;
         int ret;

         /* Add any selections by the channel mechanism. */
@@ -508,12 +508,16 @@

         if (options.server_alive_interval == 0 || !compat20)
                 tvp = NULL;
-       else {
+       else if(tv.tv_sec == 0 && tv.tv_usec == 0) {
                 tv.tv_sec = options.server_alive_interval;
                 tv.tv_usec = 0;
                 tvp = &tv;
         }
         ret = select((*maxfdp)+1, *readsetp, *writesetp, NULL, tvp);
+       if(FD_ISSET(connection_out, *readsetp)) {
+               tv.tv_sec = 0;
+               tv.tv_usec = 0;
+       }
         if (ret < 0) {
                 char buf[100];

On Sat, Apr 22, 2006 at 08:02:47PM +0300, catamus wrote:
> 
> I have found that ServerAliveInterval & ServerAliveCountMax have
> some bug. Basically the ssh-alive check function (that verify the
> peer is alive) is called only if no data at all gets into ssh (when it 
> should work only for server channel).I am pretty sure developers know
> about this ..anyway I have tried to fix this issue. Here is the patch:
Could you please give an example of the circumstances under which this
might happen?  Repeatedly banging on the keyboard generating traffic
on stdin?

I'm not sure the patch is quite right, though.
> +       if(FD_ISSET(connection_out, *readsetp)) {
> +               tv.tv_sec = 0;
> +               tv.tv_usec = 0;
This doesn't look right... it's checking if the connection out is
readable?

Maybe that should be this?

	if (FD_ISSET(connection_in, *readsetp) ||
	    FD_ISSET(connection_out, *writesetp))

-- 
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.

Hi All.

Markus pointed out that the same problem applies to sshd and that the
earlier change when applied to sshd could still be fooled under some
conditions.

Below is a slightly different method that seems to work OK for both
client and server.

Index: clientloop.c
==================================================================RCS file:
/cvs/src/usr.bin/ssh/clientloop.c,v
retrieving revision 1.162
diff -u -p -r1.162 clientloop.c
--- clientloop.c	2006/04/20 09:27:09	1.162
+++ clientloop.c	2006/05/06 05:05:47
@@ -466,6 +466,8 @@ client_wait_until_can_do_something(fd_se
     int *maxfdp, u_int *nallocp, int rekeying)
 {
 	struct timeval tv, *tvp;
+	static time_t lastalive;
+	time_t now;
 	int ret;
 
 	/* Add any selections by the channel mechanism. */
@@ -540,8 +542,15 @@ client_wait_until_can_do_something(fd_se
 		snprintf(buf, sizeof buf, "select: %s\r\n", strerror(errno));
 		buffer_append(&stderr_buffer, buf, strlen(buf));
 		quit_pending = 1;
-	} else if (ret == 0)
-		server_alive_check();
+	} else if (compat20 && options.server_alive_interval) {
+		now = time(NULL);
+		if (lastalive == 0 || FD_ISSET(connection_in, *readsetp)) {
+			lastalive = now;
+		} else if (lastalive + options.server_alive_interval <= now) {
+			lastalive = now;
+			server_alive_check();
+		}
+	}
 }
 
 static void
Index: serverloop.c
==================================================================RCS file:
/cvs/src/usr.bin/ssh/serverloop.c,v
retrieving revision 1.135
diff -u -p -r1.135 serverloop.c
--- serverloop.c	2006/03/25 18:30:55	1.135
+++ serverloop.c	2006/05/06 05:05:47
@@ -258,6 +258,8 @@ wait_until_can_do_something(fd_set **rea
     u_int *nallocp, u_int max_time_milliseconds)
 {
 	struct timeval tv, *tvp;
+	static time_t lastalive;
+	time_t now;
 	int ret;
 	int client_alive_scheduled = 0;
 
@@ -342,8 +344,15 @@ wait_until_can_do_something(fd_set **rea
 		memset(*writesetp, 0, *nallocp);
 		if (errno != EINTR)
 			error("select: %.100s", strerror(errno));
-	} else if (ret == 0 && client_alive_scheduled)
-		client_alive_check();
+	} else if (compat20 && options.client_alive_interval) {
+		now = time(NULL);
+		if (lastalive == 0 || FD_ISSET(connection_in, *readsetp)) {
+			lastalive = now;
+		} else if (lastalive + options.client_alive_interval <= now) {
+			lastalive = now;
+			client_alive_check();
+		}
+	}
 
 	notify_done(*readsetp);
 }

-- 
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.