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 _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"