Subject: Re: race in select() ?
To: <>
From: David Laight <david@l8s.co.uk>
List: tech-kern
Date: 10/09/2003 22:46:34
> > To avoid the race before select, the sigchld handler adds the bit in allsock
> > and the fd_set we will select on (allsock_select).
> > To avoid the false positive on select return, we check both the fd_set
> > we got from select, and the one we had before copying allsock to
> > allsock_select.
> > I'm not sure if allsock should be declared volatile; could the compiler
> > optimise this with registers load/store and keep values in registers
> > between the 2 copies here:
> >            readable = allsock;
> >            allsock_select = allsock;
> > This would of course require an architecture with enouth registers, or
> > a smaller value for FD_SETSIZE.

I'd go for a volatile.

> > There is still a race left between the copy to readable and the copy to
> > allsock_select, but it's harmless: in this case, we won't handle the
> > descriptor after select returns, and handle it at the next loop.

> > Index: inetd.c
> > ===================================================================
> > RCS file: /cvsroot/src/usr.sbin/inetd/inetd.c,v
> > retrieving revision 1.76
> > diff -u -r1.76 inetd.c
> > --- inetd.c	2002/01/21 14:42:28	1.76
> > +++ inetd.c	2003/10/09 19:39:02
> > @@ -272,7 +272,7 @@
> >  int	lflag;
> >  #endif
> >  int	nsock, maxsock;
> > -fd_set	allsock;
> > +fd_set	allsock, allsock_select;
> >  int	options;
> >  int	timingout;
> >  struct	servent *sp;
> > @@ -532,7 +532,8 @@
> >  		(void) sigsetmask(0L);
> >  	    }
> >  	    readable = allsock;
> > -	    if ((n = select(maxsock + 1, &readable, (fd_set *)0,
> > +	    allsock_select = allsock;
> > +	    if ((n = select(maxsock + 1, &allsock_select, (fd_set *)0,
> >  		(fd_set *)0, (struct timeval *)0)) <= 0) {
> >  		    if (n == -1 && errno != EINTR) {
> >  			syslog(LOG_WARNING, "select: %m");
> > @@ -542,7 +543,8 @@
> >  	    }
> >  	    for (sep = servtab; n && sep; sep = nsep) {
> >  	    nsep = sep->se_next;
> > -	    if (sep->se_fd != -1 && FD_ISSET(sep->se_fd, &readable)) {
> > +	    if (sep->se_fd != -1 && FD_ISSET(sep->se_fd, &readable) &&
> > +		FD_ISSET(sep->se_fd, &allsock_select)) {
> >  		n--;
> >  		if (debug)
> >  			fprintf(stderr, "someone wants %s\n", sep->se_service);
> > @@ -770,6 +772,7 @@
> >  					    sep->se_server, WTERMSIG(status));
> >  				sep->se_wait = 1;
> >  				FD_SET(sep->se_fd, &allsock);
> > +				FD_SET(sep->se_fd, &allsock_select);
> >  				nsock++;
> >  				if (debug)
> >  					fprintf(stderr, "restored %s, fd %d\n",

Grrr... merge two mail items....

> First of all, your "false positive" detection will cause the return from
> select(2) to actually be ignored in the race condition case.  In this case,
> all your patch is really doing is causing us to loop around again and pick up
> the new soeckt.

Looping around in a race condition isn't a problem!
The FD_ISSET(sep->se_fd, &readable) check ensures we ignore anything the
signal handler set in allsock_select AFTER the kernel had updated
allsock_select with the results of the select.

> Secondly, there is a race condition in the creation of "allsock_select".  If
> we read the part of "allsock" containing the file descriptor in question, but
> were interrupted before writing it to allsock_select, then we will lose the
> setting of the bit that was done in the signal handler.

Comparing allsock to allsock_select and redoing the copy if different
will fix that one.

I've also realised that the main programs use of FD_SET() and FD_CLEAR()
can also lead to bit-rot.

Did I hear someone say that a signal handler should only set an entire
volatile global variable to zero....

Writing a byte (ie sep->se_fd) to a pipe starts looking like a good
idea (provided you can convince yourself that the pipe will never get full).

Of course this could still be an entirely different bug!  SVR4 streams
(as in UW2) would fail to wakeup poll if a message arrived at the stream
head in interrupt context and an odd moment.  NetBSD's select/poll looks
better that that did - but could still have a nasty little window.

	David

--
David Laight: david@l8s.co.uk