SIGCHLD race *trivial* patch

[Nicolas Williams]

Yes, this is a patch against an older version of OpenSSH with other
stuff anyways, BUT, it's so TRIVIAL(*), that you can see how it would
apply to newer versions (which I've not tried).

Here's the gist: server_loop2() has a race condition with respect to
reception of SIGCHLD and checking/setting child_terminated. This patch
does two things: wait_until_can_do_something() adds a 1 second timeout
to select() IF AND ONLY IF (!channel_still_open) AND, server_loop2()
breaks out of its loop when there are no sessions left.

Blocking SIGCHLD before select()ing would not fix the problem, nor would
that be very portable.

So, summary of changes:

 - session.h

   Added prototype for session_still_used() boolean function.

 - session.c

   Added session_still_used() boolean function.

 - serverloop.c

   Added this bit of code to wait_until_can_do_something():

      if (!channel_still_open())
              max_time_milliseconds = 1000;

   before select()ing.

   Added this bit of code to server_loop2():

                if (child_terminated) {
                        while ((pid = waitpid(-1, &status, WNOHANG)) > 0)
                                session_close_by_pid(pid, status);
-                       child_terminated = 0;
+                       if (session_still_used())
+                               child_terminated = 0;
+                    if (child_terminated && !channel_still_open())
+                       break;

   so that child_terminated is not reset after handling SIGCHLD *UNLESS*
   there are no sessions left open.

Also, for server_loop(), perhaps it too should have a check to break
out of its loop if the child is terminated and there are no channels
still open.

(*) I hope :)

Cheers,

Nico


Index: 2_9_p2_w_gss_krb5_named_keys.10/session.h
--- 2_9_p2_w_gss_krb5_named_keys.10/session.h Tue, 26 Jun 2001 16:27:13 -0400 willian (OpenSSH/i/13_session.h 1.2 644)
+++ 2_9_p2_w_gss_krb5_named_keys.10(w)/session.h Thu, 25 Oct 2001 14:58:32 -0400 willian (OpenSSH/i/13_session.h 1.2 644)
@@ -28,6 +28,7 @@
 
 void	do_authenticated(Authctxt *ac);
 
+int	session_still_used();
 int	session_open(int id);
 void	session_input_channel_req(int id, void *arg);
 void	session_close_by_pid(pid_t pid, int status);
Index: 2_9_p2_w_gss_krb5_named_keys.10/session.c
--- 2_9_p2_w_gss_krb5_named_keys.10/session.c Tue, 26 Jun 2001 16:27:13 -0400 willian (OpenSSH/i/14_session.c 1.3 644)
+++ 2_9_p2_w_gss_krb5_named_keys.10(w)/session.c Thu, 25 Oct 2001 15:04:21 -0400 willian (OpenSSH/i/14_session.c 1.3 644)
@@ -1533,18 +1533,18 @@
 	exit(1);
 }
 
+static int did_init_sessions = 0;
 Session *
 session_new(void)
 {
 	int i;
-	static int did_init = 0;
-	if (!did_init) {
+	if (!did_init_sessions) {
 		debug("session_new: init");
 		for(i = 0; i < MAX_SESSIONS; i++) {
 			sessions[i].used = 0;
 			sessions[i].self = i;
 		}
-		did_init = 1;
+		did_init_sessions = 1;
 	}
 	for(i = 0; i < MAX_SESSIONS; i++) {
 		Session *s = &sessions[i];
@@ -1622,6 +1622,27 @@
 	error("session_by_pid: unknown pid %d", pid);
 	session_dump();
 	return NULL;
+}
+
+int
+session_still_used()
+{
+	int i;
+	if (!did_init_sessions) {
+		debug("session_new: init");
+		for(i = 0; i < MAX_SESSIONS; i++) {
+			sessions[i].used = 0;
+			sessions[i].self = i;
+		}
+		did_init_sessions = 1;
+	}
+	debug("session_still_used");
+	for(i = 0; i < MAX_SESSIONS; i++) {
+		Session *s = &sessions[i];
+		if (s->used)
+			return 1;
+	}
+	return 0;
 }
 
 int
Index: 2_9_p2_w_gss_krb5_named_keys.10/serverloop.c
--- 2_9_p2_w_gss_krb5_named_keys.10/serverloop.c Thu, 03 May 2001 16:12:13 -0400 jd (OpenSSH/i/16_serverloop 1.1 644)
+++ 2_9_p2_w_gss_krb5_named_keys.10(w)/serverloop.c Thu, 25 Oct 2001 15:11:49 -0400 willian (OpenSSH/i/16_serverloop 1.1 644)
@@ -259,18 +259,25 @@
 		if (max_time_milliseconds == 0 || client_alive_scheduled)
 			max_time_milliseconds = 100;
 
+	if (!channel_still_open())
+		max_time_milliseconds = 1000;
+
 	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;
+		debug3("select timeout is: tv.tv_sec=%d, tv.tv_usec=%d", (int) max_time_milliseconds /
+			1000, (int) 1000 * (max_time_milliseconds % 1000));
 	}
 	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);
+	debug("select() returned %d, child_terminated=%d, channel_still_open() returned %d", ret,
+		child_terminated, channel_still_open());
 
 	if (ret == -1) {
 		if (errno != EINTR)
@@ -590,6 +597,8 @@
 		/* Sleep in select() until we can do something. */
 		wait_until_can_do_something(&readset, &writeset, &max_fd,
 		    max_time_milliseconds);
+		debug("wait_until_can_do_something returned; child_terminated=%d, channel_still_open=%d",
+			child_terminated, channel_still_open());
 
 		/* Process any channel events. */
 		channel_after_select(readset, writeset);
@@ -599,6 +608,11 @@
 
 		/* Process output to the client and to program stdin. */
 		process_output(writeset);
+
+		/* Don't know if this is needed -- if it is, uncomment 
+		if (!channel_still_open() && child_terminated)
+			break;
+		*/
 	}
 	if (readset)
 		xfree(readset);
@@ -721,7 +735,10 @@
 		if (child_terminated) {
 			while ((pid = waitpid(-1, &status, WNOHANG)) > 0)
 				session_close_by_pid(pid, status);
-			child_terminated = 0;
+			if (session_still_used())
+				child_terminated = 0;
+		     if (child_terminated && !channel_still_open())
+			break;
 		}
 		if (!rekeying)
 			channel_after_select(readset, writeset);

--
-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-

Visit our website at http://www.ubswarburg.com

This message contains confidential information and is intended only 
for the individual named.  If you are not the named addressee you 
should not disseminate, distribute or copy this e-mail.  Please 
notify the sender immediately by e-mail if you have received this 
e-mail by mistake and delete this e-mail from your system.

E-mail transmission cannot be guaranteed to be secure or error-free 
as information could be intercepted, corrupted, lost, destroyed, 
arrive late or incomplete, or contain viruses.  The sender therefore 
does not accept liability for any errors or omissions in the contents 
of this message which arise as a result of e-mail transmission.  If 
verification is required please request a hard-copy version.  This 
message is provided for informational purposes and should not be 
construed as a solicitation or offer to buy or sell any securities or 
related financial instruments.