SIGCHLD race *trivial* patch

[Nicolas Williams]

SSH_CHANNEL_INPUT_DRAINING and SSH_CHANNEL_OUTPUT_DRAINING channels are
considered "still open" by channel_still_open(), so no, no data can be
lost.

That said, there's no harm in moving the check to the bottom of the
loop.

A new, cleaner patch is attached. Again, against 2.9p2 + stuff, but
conceptually it should apply to any later releases that still have this
bug.

Cheers,

Nico

On Thu, Oct 25, 2001 at 04:30:09PM -0400, Nicolas Williams wrote:
> I've done some testing and no, no data is lost. Look at the new
> condition for breaking out of the server loop: if (there's no channels
> AND there's no sessions).
> 
> Of course, I'm not too familiar with how unsent buffers would be
> handled. Can you explain?
> 
> Perhaps the (no channels open && no sessions in use) check should be
> made at the bottom of the loop, after channel_after_select(),
> process_input() and process_output() have been called.
> 
> Hmmm,
> 
> Nico
> 
> On Thu, Oct 25, 2001 at 03:25:12PM -0500, mouring at etoh.eviladmin.org wrote:
> > 
> > I've not tested this, but have you verified that scp never loses any data?
> > and the 'ssh site date'  NEVER loses data?
> > 
> > - Ben


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.12(w)/session.h Thu, 25 Oct 2001 14:58:32 -0400 willian (OpenSSH/i/13_session.h 1.3 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.12(w)/session.c Thu, 25 Oct 2001 15:04:21 -0400 willian (OpenSSH/i/14_session.c 1.4 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.12(w)/serverloop.c Thu, 25 Oct 2001 16:35:36 -0400 willian (OpenSSH/i/16_serverloop 1.3 644)
@@ -259,6 +259,9 @@
 		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 {
@@ -599,6 +602,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 +729,8 @@
 		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 (!rekeying)
 			channel_after_select(readset, writeset);
@@ -729,6 +738,8 @@
 		if (connection_closed)
 			break;
 		process_output(writeset);
+		if (child_terminated && !channel_still_open())
+			break;
 	}
 	if (readset)
 		xfree(readset);

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