SIGCHLD race *trivial* patch
Nicolas Williams
Nicolas.Williams at ubsw.com
Fri Oct 26 06:30:09 EST 2001
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
>
> On Thu, 25 Oct 2001, Nicolas Williams wrote:
>
> > I forgot to mention the behaviour that this patch fixes: hang-on-exit
> > for SSHv2 sessions. See the other "SIGCHLD race condition?" thread for
> > more details.
> >
> > Cheers,
> >
> > Nico
> >
> >
> > On Thu, Oct 25, 2001 at 03:25:22PM -0400, Nicolas Williams wrote:
> > > 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.
> >
> >
--
-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.
More information about the openssh-unix-dev
mailing list