svn commit: r327495 - head/usr.sbin/rpcbind

[Bruce Evans]

On Tue, 2 Jan 2018, Conrad Meyer wrote:

> Log:
>  rpcbind: Fix race in signal termination

I have yet to see any application that was correctly converted from using
unsafe signal handlers to safe signal handlers that just set a flag.  That
is without even noticing this race problem before.

>  If a signal was delivered while the main thread was not in poll(2) and after
>  check was performed, we could reenter poll and never detect termination. Fix
>  this with the pipefd trick.  (This race was introduced very recently, in
>  r327482.)

poll() at least returns if the signal occurs after it is called.  Most other
syscalls related to i/o restart after a syscall when SA_RESTART is configured
for the syscall.  SA_RESTART is the default for BSD since handling EINTR after
any syscall is too hard.  Simple conversions from using unsafe conversions
break i/o by not turning off SA_RESTART (top(1) and rpcbind(8) are examples
-- details below).  Unsimple conversions do turn off SA_RESTART but this is
complicated and tends to give bugs (ping(8) is an example).  This race bug
shows that even turning off SA_RESTART doesn't help.  Since it doesn't help,
it just tends to gives bugs from its complications and shouldn't be used.

Today I learned from the POSIX list that the pselect() function is designed
to avoid this bug.  It takes a signal mask arg (like msleep() in the kernel).
The flag should only be checked while signals are masked, and signals must
not be unmasked before making any syscall that might sleep, but applications
rarely bother to mask signals before checking the flag, and there aren't
many i/o functions that take a signal mask.

I haven't noticed any function related to poll() or kevent that takes a
signal mask.

Programs broken by buggy conversion:
- top(1).  Run it interactively and type 's' followed by any non-null
   valid input (say '1').  Then try to abort it using ^C or ^\.  Neither
   works, because read() restarts.  You have to enter a newline to get
   read() to return.  The flag is then checked and top exits with the
   bogus exit status 0 and no message.  top still uses plain signal()
   so gets the default (SA_RESTART on BSD systems and possibly bugs
   from !SA_RESTART on non-BSD systems).  Races on all systems when the
   signal arrives between checking the flag and calling read().
- rpcbind(8).  This is not interactive and normally doesn't use ttys
   which might block.  However, the -d flag makes it do fprintf() to
   stderr.  This may block forever (e.g., for flow control), and you
   would like to be able to kill it using a signal.  But write() will
   restart.  rpcbind also uses plain signal() and doesn't know anything
   about SA_RESTART.
- ping(8).  This does know about SA_RESTART.  However, turning off
   SA_RESTART gives lots of EINTRs that few BSD programs and libraries
   handle properly and fewer libraries document their handling.  This
   caused hangs for some unreachable addresses.  The resolver library
   handled EINTR (from select() IIRC) by restarting, so ^C didn't work.
   It is unclear if either returning or restarting after EINTR is correct
   in a library function.  This was fixed long ago and I didn't know of
   any other similar bugs in ping.  It would be hard to check all syscalls
   in ping and library functions that it calls.  Now I know that there
   must be lots of races because turning off SA_RESTART doesn't really work.

I used to think that correct conversion required only the following modest
changes:
- don't change SA_RESTART or otherwise fiddle with signal handling global
- find all syscalls in the program and libraries that might block, and add
   turn off SA_RESTART around them.  Possibly also change other signal handling
   locally.

But this doesn't work since the race can be lost before making the syscalls.

Next try: add timeouts using alarm() or itimers to every syscall that might
block.  I think this fixes the race before calling syscalls too.

But this is too hard for most programs.  I think it is best to try to write
safe signal handlers.  Unfortunately, APIs support this negatively.  perror()
is broken as designed since it uses stdio, so it is unsafe in signal
handlers.  The err() family copies this bug.  Even *s*printf() is not
required to be safe in signal handlers.  I would fix the latter first.  Then
try to change the API of warn() and warnx() to be safe.  err() can't be
change since it has to keep calling exit(), but it is easy to use the safe
warn() followed by _exit() stdio is avoided, and important to know that it
is avoided.

> ...
> Modified: head/usr.sbin/rpcbind/rpcb_svc_com.c
> ==============================================================================
> --- head/usr.sbin/rpcbind/rpcb_svc_com.c	Tue Jan  2 16:50:57 2018	(r327494)
> +++ head/usr.sbin/rpcbind/rpcb_svc_com.c	Tue Jan  2 17:25:13 2018	(r327495)
> ....
> @@ -1130,23 +1133,26 @@ my_svc_run(void)
> 			fprintf(stderr, ">\n");
> 		}
> #endif
> -		switch (poll_ret = poll(pollfds, nfds, 30 * 1000)) {
> +		poll_ret = poll(pollfds, nfds, 30 * 1000);

I think this is too specialized and complicated.  poll() already has a
timeout arg, so it doesn't need the separate timeout that seems to be
needed for the general case.  30 seconds is just too long.

Plain select() also has a timeout, and its granularity is much lower than
poll()'s, so pselect() isn't essential for fixing the race.  You just have
to use a small timeout which reduces select() to busy-waiting if it is too
small.

1 millisecond is usually too small, but 1 second seems reasonable for
most cases.  The timeout is only used after rarely-lost races unless it
is small.

> Modified: head/usr.sbin/rpcbind/rpcbind.c
> ==============================================================================
> --- head/usr.sbin/rpcbind/rpcbind.c	Tue Jan  2 16:50:57 2018	(r327494)
> +++ head/usr.sbin/rpcbind/rpcbind.c	Tue Jan  2 17:25:13 2018	(r327495)
> ...
> @@ -761,8 +774,13 @@ rbllist_add(rpcprog_t prog, rpcvers_t vers, struct net
> static void
> terminate(int signum)
> {
> +	char c = '\0';
> +	ssize_t wr;
>
> 	doterminate = signum;
> +	wr = write(terminate_wfd, &c, 1);

Not async-signal-safe.  Acccesses in signal handlers to objects with
static storage duration give undefined behaviour except for assignment to
objects of type volatile sig_atomic_t, but the access to terminate_wfd is
a read and the type is plain int.  Using " " for the string would be
equally unsafe (very safe in practice :-), but the function already does
extra work to create the character array at runtime to avoid this.

> +	if (wr < 1)
> +		_exit(2);

Best to not check for write errors, since the error handling of using _exit()
is worse than none.  It loses stdio flushing to handle an almost-harmless
error.  The main problem with keeping everything in a safe handler is that it
is impossible to keep stdio flushing there and we would prefer to not lost
the stdio flushing.

> }

Bruce