suggested fix for the sigchld race

[Nicolas Williams]

On Wed, Oct 31, 2001 at 05:42:01PM +0100, Markus Friedl wrote:
> comments?

Works. The SIGCHLD/select() race is still present in 3.0.2p1. This
patch should be applied.

The patch applies cleanly but for the bit that patches the signal
handler - the change from signal(...) to mysignal() confuses patch.


> alternatives: sigsetjmp(ugly) and pselect(not portable, available)
> drawback: additional filedescriptors.

sigsetjmp() is not ugly (this is the sort of problem it's meant to be
there for). But it's not as elegant as the pipe trick and probably much
less portable. I prefer the the pipe trick.

I've never used pselect().

So I second the use of the pipe trick to prevent the SIGCHLD/select()
race condition.

I'll file a bug report.

Thanks! Cheers,

Nico


> Index: serverloop.c
> ===================================================================
> RCS file: /home/markus/cvs/ssh/serverloop.c,v
> retrieving revision 1.82
> diff -u -r1.82 serverloop.c
> --- serverloop.c	10 Oct 2001 22:18:47 -0000	1.82
> +++ serverloop.c	11 Oct 2001 18:06:33 -0000
> @@ -92,6 +92,45 @@
>  /* prototypes */
>  static void server_init_dispatch(void);
>  
> +/*
> + * we write to this pipe if a SIGCHLD is caught in order to avoid
> + * the race between select() and child_terminated
> + */
> +static int notify_pipe[2];
> +static void
> +notify_setup(void)
> +{
> +	if (pipe(notify_pipe) < 0) {
> +		error("pipe(notify_pipe) failed %s", strerror(errno));
> +		notify_pipe[0] = -1;	/* read end */
> +		notify_pipe[1] = -1;	/* write end */
> +	} else {
> +		set_nonblock(notify_pipe[0]);
> +		set_nonblock(notify_pipe[1]);
> +	}
> +}
> +static void
> +notify_parent(void)
> +{
> +	if (notify_pipe[1] != -1)
> +		write(notify_pipe[1], "", 1);
> +}
> +static void
> +notify_prepare(fd_set *readset)
> +{
> +	if (notify_pipe[0] != -1)
> +		FD_SET(notify_pipe[0], readset);
> +}
> +static void
> +notify_done(fd_set *readset)
> +{
> +	char c;
> +
> +	if (notify_pipe[0] != -1 && FD_ISSET(notify_pipe[0], readset))
> +		while (read(notify_pipe[0], &c, 1) != -1)
> +			debug2("notify_done: reading");
> +}
> +
>  static void
>  sigchld_handler(int sig)
>  {
> @@ -99,6 +138,7 @@
>  	debug("Received SIGCHLD.");
>  	child_terminated = 1;
>  	signal(SIGCHLD, sigchld_handler);
> +	notify_parent();
>  	errno = save_errno;
>  }
>  
> @@ -242,6 +282,7 @@
>  		if (fdin != -1 && buffer_len(&stdin_buffer) > 0)
>  			FD_SET(fdin, *writesetp);
>  	}
> +	notify_prepare(*readsetp);
>  
>  	/*
>  	 * If we have buffered packet data going to the client, mark that
> @@ -278,6 +319,8 @@
>  			error("select: %.100s", strerror(errno));
>  	} else if (ret == 0 && client_alive_scheduled)
>  		client_alive_check();
> +
> +	notify_done(*readsetp);
>  }
>  
>  /*
> @@ -467,6 +510,8 @@
>  	connection_in = packet_get_connection_in();
>  	connection_out = packet_get_connection_out();
>  
> +	notify_setup();
> +
>  	previous_stdout_buffer_bytes = 0;
>  
>  	/* Set approximate I/O buffer size. */
> @@ -572,6 +617,7 @@
>  		max_fd = MAX(max_fd, fdin);
>  		max_fd = MAX(max_fd, fdout);
>  		max_fd = MAX(max_fd, fderr);
> +		max_fd = MAX(max_fd, notify_pipe[0]);
>  
>  		/* Sleep in select() until we can do something. */
>  		wait_until_can_do_something(&readset, &writeset, &max_fd,
> @@ -696,7 +742,11 @@
>  	connection_in = packet_get_connection_in();
>  	connection_out = packet_get_connection_out();
>  
> +	notify_setup();
> +
>  	max_fd = MAX(connection_in, connection_out);
> +	max_fd = MAX(max_fd, notify_pipe[0]);
> +
>  	xxx_authctxt = authctxt;
>  
>  	server_init_dispatch();
--
-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.