Idletimeout patch, third attempt

Jani Jaakkola jjaakkol at cs.Helsinki.FI
Tue Aug 21 07:56:52 EST 2001


Here is my third attempt at the idletimeout patch. I tried to address
the points which Marcus Friedl brought up.

It is actually bigger than the previous patches, but not as intrusive.
It is big because it moves some stuff from serverloop.c to packet.c.

- I moved all the logic to packet.c. This means that I also had to move
  the actual select() call, which used to be in serverloop.c to packet.c.
  Now serverloop.c only uses packet.c and not the other way around, so
  dependencies are cleaner.

- I implemented packet_select() in packet.c. This makes possible
  to catch idletimeouts not only when when server is sitting in
  wait_until_can_do_something() but also when server is
  waiting for completion of full packet in packet_read() or waiting
  for completion of writing full packet in packet_write_wait(). With
  previous patches sshd could (in theory at least) hang on packet_read()
  or packet_write_wait() even if idletimeout or clientalive was set.

- with packet_select() ssh-client side idletimeout could be easily
  implemented, if anybody would need a feature like that.

- Counting of idle timeouts starts now immediately after authorization
  has been successfull, not when sshd first enters
  wait_until_can_do_something()

- IMHO, the patch is much cleaner. max_time_seconds variable has been
  removed.

- It fixes two places, where select() might have been called after
  EAGAIN or EINTR, but the fd_sets would not have been reinitialized.

- If select returns error other than EAGAIN or EINTR, it exits with
  fatal(). Before the error would either be ignored or just logged,
  but the program would have continued like nothing would have happened.
  The manual says, that after select() error (like ENOMEM) the values
  of fd_sets are undefined. I guess that in some perverse situation
  sshd could also have got stuck in infinite select() loop.

Again, I have actually done some testing (but not much) and have found
no problems.

This patch is also available from:
http://www.cs.helsinki.fi/u/jjaakkol/idletimeout.patch

- Jani
-------------- next part --------------
diff -ru openssh-2.9p2.orig/CREDITS openssh-2.9p2/CREDITS
--- openssh-2.9p2.orig/CREDITS	Mon Apr 16 03:41:46 2001
+++ openssh-2.9p2/CREDITS	Wed Aug 15 22:00:02 2001
@@ -42,6 +42,7 @@
 IWAMURO Motonori <iwa at mmp.fujitsu.co.jp> - bugfixes
 Jani Hakala <jahakala at cc.jyu.fi> - Patches
 Jarno Huuskonen <jhuuskon at hytti.uku.fi> - Bugfixes
+Jani Jaakkola <jjaakkol at cs.helsinki.fi> - IdleTimeOut
 Jim Knoble <jmknoble at jmknoble.cx> - Many patches
 Jonchen (email unknown) - the original author of PAM support of SSH
 Juergen Keil <jk at tools.de> - scp bugfixing
diff -ru openssh-2.9p2.orig/clientloop.c openssh-2.9p2/clientloop.c
--- openssh-2.9p2.orig/clientloop.c	Fri Apr 20 15:50:51 2001
+++ openssh-2.9p2/clientloop.c	Mon Aug 20 22:20:38 2001
@@ -1250,3 +1250,4 @@
 	else
 		client_init_dispatch_15();
 }
+
diff -ru openssh-2.9p2.orig/packet.c openssh-2.9p2/packet.c
--- openssh-2.9p2.orig/packet.c	Fri Apr  6 02:26:33 2001
+++ openssh-2.9p2/packet.c	Tue Aug 21 00:04:48 2001
@@ -121,10 +121,91 @@
 /* True if SSH2 packet format is used */
 int use_ssh2_packet_format = 0;
 
+static time_t idletime_last=0; /* The last time something happened
+				* for idletimeout. */
+static int idletimeout=0;      /* The current idletimeout */
+
 /* Session key information for Encryption and MAC */
 Newkeys *newkeys[MODE_MAX];
 
 void
+packet_set_idletimeout(int max_idle_seconds) 
+{
+        idletimeout=max_idle_seconds;
+        if (max_idle_seconds>0) {
+	        /* Initialize */
+	        time(&idletime_last);
+        }
+}
+
+/* Called by whenever packets are sent or received.
+ * This function decides on which packets idletimeout should
+ * be reset */
+void 
+idletimeout_check(int type) 
+{
+        /* No-op, if idletimeouts are not configured */
+        if (idletimeout==0) return;
+
+	/* The following packets reset idletimeout on input or output.
+	 * Note that only actual data resets idletimeout, control packets 
+	 * do not. */
+	switch(type) {
+	case SSH_MSG_CHANNEL_DATA:
+	case SSH_CMSG_STDIN_DATA:
+	case SSH_SMSG_STDOUT_DATA:
+	case SSH_SMSG_STDERR_DATA:
+	case SSH2_MSG_CHANNEL_DATA:
+	case SSH2_MSG_CHANNEL_EXTENDED_DATA:
+	    time(&idletime_last);
+	}
+}
+
+int     
+packet_select(int maxfds,
+	      fd_set *readset, fd_set *writeset, fd_set *exceptset,
+	      int max_time_milliseconds)
+{
+        struct timeval tv, *tvp=NULL;
+	int ret;
+
+	if (idletimeout>0) {
+	        /* Count the time to idletimeout */
+	        time_t diff=time(NULL)-idletime_last;
+		if (diff>=idletimeout)
+		        tv.tv_sec=1;
+		else
+    		        tv.tv_sec=idletimeout-diff+1;
+		tv.tv_usec=0;
+		tvp = &tv;
+		debug("idletimeout after %ld seconds",tv.tv_sec);
+	}
+	/* If a timeout value was given and the timeout happens before
+	 * idletimeout, use it */
+	if (max_time_milliseconds>0 &&
+	    (tvp==NULL || max_time_milliseconds/1000<tv.tv_sec)) {
+	        tv.tv_sec = max_time_milliseconds / 1000;
+		tv.tv_usec = 1000 * (max_time_milliseconds % 1000);
+		tvp = &tv;
+	        debug("timeout after %d milliseconds",max_time_milliseconds);
+	}
+	if (tvp==NULL) debug("no timeout");
+
+	ret = select( maxfds, readset, writeset, exceptset, tvp );
+	if (ret<0 && errno!=EAGAIN && errno!=EINTR) {
+	        fatal("select: %.100s", strerror(errno));
+	}
+
+	/* Disconnect on idletimeout */
+	if (idletimeout>0 && ret==0 && 
+	    time(NULL)-idletime_last>idletimeout) {
+	        packet_disconnect("Idletimeout.");
+		idletimeout=0;
+	}
+	return ret;
+}
+
+void
 packet_set_ssh2_format(void)
 {
 	DBG(debug("use_ssh2_packet_format"));
@@ -383,6 +464,7 @@
 		packet_start2(type);
 	else
 		packet_start1(type);
+        idletimeout_check(type);
 }
 
 /* Appends a character to the packet data. */
@@ -708,20 +790,21 @@
 		/* If we got a packet, return it. */
 		if (type != SSH_MSG_NONE) {
 			xfree(setp);
+			idletimeout_check(type);
 			return type;
 		}
 		/*
 		 * Otherwise, wait for some data to arrive, add it to the
 		 * buffer, and try again.
 		 */
-		memset(setp, 0, howmany(connection_in + 1, NFDBITS) *
-		    sizeof(fd_mask));
-		FD_SET(connection_in, setp);
-
-		/* Wait for some data to arrive. */
-		while (select(connection_in + 1, setp, NULL, NULL, NULL) == -1 &&
-		    (errno == EAGAIN || errno == EINTR))
-			;
+		do {
+   		        memset(setp, 0, howmany(connection_in + 1, NFDBITS) *
+			       sizeof(fd_mask));
+		        FD_SET(connection_in, setp);
+			
+			/* Wait for some data to arrive. */
+		} while (packet_select(connection_in + 1,
+				       setp, NULL, NULL, 0) == -1);
 
 		/* Read data from the socket. */
 		len = read(connection_in, buf, sizeof(buf));
@@ -974,6 +1057,7 @@
 		    packet_read_poll2(payload_len_ptr):
 		    packet_read_poll1(payload_len_ptr);
 
+		idletimeout_check(type);
 		if(compat20) {
 			int reason;
 			if (type != 0)
@@ -1217,12 +1301,12 @@
 	    sizeof(fd_mask));
 	packet_write_poll();
 	while (packet_have_data_to_write()) {
-		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 &&
-		    (errno == EAGAIN || errno == EINTR))
-			;
+	        do {
+		        memset(setp, 0, howmany(connection_out + 1, NFDBITS) *
+			       sizeof(fd_mask));
+			FD_SET(connection_out, setp);
+		} while (packet_select(connection_out + 1, 
+				       NULL, setp, NULL, 0) == -1);
 		packet_write_poll();
 	}
 	xfree(setp);
diff -ru openssh-2.9p2.orig/packet.h openssh-2.9p2/packet.h
--- openssh-2.9p2.orig/packet.h	Sun Apr 15 02:13:03 2001
+++ openssh-2.9p2/packet.h	Mon Aug 20 23:36:10 2001
@@ -220,4 +220,20 @@
 /* add an ignore message and make sure size (current+ignore) = n*sumlen */
 void	packet_inject_ignore(int sumlen);
 
+/* This sets the maximum idle time before packet_select() automatically
+ * disconnects with packet_disconnect("Idletimeout"). 
+ * Never autodisconnects if set to zero. zero is the default */
+void    packet_set_idletimeout(int max_idle_seconds);
+
+/* This is an quite normal select, except that it implements idletimeouts
+ * set with packet_set_idletimeout().
+ * It also returns exits, if select() returns any other error than AGAIN
+ * or EINTR. So if packet_select returns -1, you can safely reinit fd_sets
+ * and call packet_select again, without checking errno.
+ */
+int     packet_select(int maxfds,
+		      fd_set *readset, fd_set *writeset, fd_set *exceptset,
+		      int max_time_milliseconds);
+
+
 #endif				/* PACKET_H */
diff -ru openssh-2.9p2.orig/servconf.c openssh-2.9p2/servconf.c
--- openssh-2.9p2.orig/servconf.c	Wed Apr 25 15:44:15 2001
+++ openssh-2.9p2/servconf.c	Wed Aug 15 22:10:23 2001
@@ -102,6 +102,7 @@
 	options->client_alive_interval = -1;
 	options->client_alive_count_max = -1;
 	options->pam_authentication_via_kbd_int = -1;
+	options->idletimeout = -1;
 }
 
 void
@@ -210,6 +211,8 @@
 		options->client_alive_count_max = 3;
 	if (options->pam_authentication_via_kbd_int == -1)
 		options->pam_authentication_via_kbd_int = 0;
+	if (options->idletimeout == -1)
+		options->idletimeout=0;
 }
 
 /* Keyword tokens. */
@@ -235,7 +238,8 @@
 	sGatewayPorts, sPubkeyAuthentication, sXAuthLocation, sSubsystem, sMaxStartups,
 	sBanner, sReverseMappingCheck, sHostbasedAuthentication,
 	sHostbasedUsesNameFromPacketOnly, sClientAliveInterval, 
-	sClientAliveCountMax, sPAMAuthenticationViaKbdInt
+	sClientAliveCountMax, sPAMAuthenticationViaKbdInt,
+	sIdleTimeout
 } ServerOpCodes;
 
 /* Textual representation of the tokens. */
@@ -302,6 +306,7 @@
 	{ "clientaliveinterval", sClientAliveInterval },
 	{ "clientalivecountmax", sClientAliveCountMax },
 	{ "PAMAuthenticationViaKbdInt", sPAMAuthenticationViaKbdInt },
+	{ "idletimeout", sIdleTimeout },
 	{ NULL, 0 }
 };
 
@@ -801,7 +806,28 @@
 		case sPAMAuthenticationViaKbdInt:
 			intptr = &options->pam_authentication_via_kbd_int;
 			goto parse_flag;
-
+               case sIdleTimeout:
+			arg = strdelim(&cp);
+			if (!arg || *arg == '\0')
+				fatal("%s line %d: Missing IdleTimeout argument",
+					filename,linenum);
+			options->idletimeout=atoi(arg);
+			switch(arg[strlen(arg)-1]) {
+				case 'w': options->idletimeout*=7;
+				case 'd': options->idletimeout*=24;
+				case 'h': options->idletimeout*=60;
+				case 'm': options->idletimeout*=60;
+				case 's': 
+				case '0': case '1': case '2': case '3': 
+				case '4': case '5': case '6': case '7': 
+				case '8': case '9':
+	                               break;
+				default:
+					fatal("%s line %d: Invalid IdleTimeout argument",
+						filename,linenum);
+			}
+			break;
+	
 		default:
 			fatal("%s line %d: Missing handler for opcode %s (%d)",
 			    filename, linenum, arg, opcode);
diff -ru openssh-2.9p2.orig/servconf.h openssh-2.9p2/servconf.h
--- openssh-2.9p2.orig/servconf.h	Wed Apr 25 15:44:16 2001
+++ openssh-2.9p2/servconf.h	Wed Aug 15 22:09:33 2001
@@ -125,6 +125,10 @@
 					 * diconnect the session 
 					 */
 	int	pam_authentication_via_kbd_int;
+	int	idletimeout;		/*
+					 * If nonzero, the number of second
+					 * after which idle connections
+					 * will be terminated */
 }       ServerOptions;
 /*
  * Initializes the server options to special values that indicate that they
diff -ru openssh-2.9p2.orig/serverloop.c openssh-2.9p2/serverloop.c
--- openssh-2.9p2.orig/serverloop.c	Sat Apr 14 02:28:03 2001
+++ openssh-2.9p2/serverloop.c	Mon Aug 20 23:33:30 2001
@@ -190,7 +190,6 @@
 wait_until_can_do_something(fd_set **readsetp, fd_set **writesetp, int *maxfdp,
     u_int max_time_milliseconds)
 {
-	struct timeval tv, *tvp;
 	int ret;
 	int client_alive_scheduled = 0;
 
@@ -259,25 +258,15 @@
 		if (max_time_milliseconds == 0 || client_alive_scheduled)
 			max_time_milliseconds = 100;
 
-	if (max_time_milliseconds == 0)
-		tvp = NULL;
-	else {
-		tv.tv_sec = max_time_milliseconds / 1000;
-		tv.tv_usec = 1000 * (max_time_milliseconds % 1000);
-		tvp = &tv;
-	}
-	if (tvp!=NULL)
-		debug3("tvp!=NULL kid %d mili %d", child_terminated, max_time_milliseconds);
 
-	/* Wait for something to happen, or the timeout to expire. */
-	ret = select((*maxfdp)+1, *readsetp, *writesetp, NULL, tvp);
+	/* Wait for something to happen, or the timeout to expire. 
+	 * packet select also implements server idletimeouts for us. */
+	ret = packet_select((*maxfdp)+1, *readsetp, *writesetp, NULL, 
+			    max_time_milliseconds);
+
+	if (ret == -1)
+	        goto retry_select;
 
-	if (ret == -1) {
-		if (errno != EINTR)
-			error("select: %.100s", strerror(errno));
-		else
-			goto retry_select;
-	}
 	if (ret == 0 && client_alive_scheduled) {
 		/* timeout, check to see how many we have had */
 		client_alive_timeouts++;
diff -ru openssh-2.9p2.orig/session.c openssh-2.9p2/session.c
--- openssh-2.9p2.orig/session.c	Sun Jun 17 06:40:51 2001
+++ openssh-2.9p2/session.c	Mon Aug 20 22:47:16 2001
@@ -170,6 +170,11 @@
 	 * authentication.
 	 */
 	alarm(0);
+	/*
+	 * Now that the login grace alarm is cleared it is time to apply
+	 * idletimeout */
+	packet_set_idletimeout(options.idletimeout);
+
 	if (startup_pipe != -1) {
 		close(startup_pipe);
 		startup_pipe = -1;


More information about the openssh-unix-dev mailing list