SIGCHLD race *trivial* patch

[Nicolas Williams]

Oh, that's too bad. I haven't played with the current CVS.

Nonetheless, you can see what the problem was and how the race was
addressed in our patch. Wether the concept carries over will depend on
how much server_loop2() and friends have changed since 2.9p2.

Specifically,

 - select() must be given a timeout if there are no open channels
 - child_terminated should be true only when there are no more
   sessions in use
 - the main loop should be exited when there are no more sessions in use
   and no more channels open

This way sshd doesn't get stuck in a select() with no timeout, no live
children, and no open file descriptors from its now-dead children.

Since draining channels are considered open no data can be lost.

Nico


On Thu, Oct 25, 2001 at 04:01:52PM -0500, mouring at etoh.eviladmin.org wrote:
> 
> It won't apply anywhere clean to the current CVS tree.. and when I shoe
> horn it.. it provides me with no useful solution.
> 
> - Ben
> 
> On Thu, 25 Oct 2001, Nicolas Williams wrote:
> 
> > 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.
> >
> >
--
-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.