From owner-svn-src-all@freebsd.org Sat Feb 3 19:31:30 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 02647EE621D; Sat, 3 Feb 2018 19:31:30 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-io0-f180.google.com (mail-io0-f180.google.com [209.85.223.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 92FDB76F5B; Sat, 3 Feb 2018 19:31:29 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-io0-f180.google.com with SMTP id l17so26403049ioc.3; Sat, 03 Feb 2018 11:31:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:reply-to:in-reply-to:references :from:date:message-id:subject:to:cc; bh=Znj6mo+0x6INbwRZR5Tuja8o+VPGY7CdNJm0jaW1R3k=; b=kx3pIjG4KRV58l9G8ZJQjZ5Xx3+4dVLIU+eTw6pQygMjJkK+lJFXSO9MvKJNK85yic pBcGejIp8dPjpHmMqQIAxeHPPHmlmlHJwHEDJw0MwTOUpUM8ZJCYmp/IRBlUxxWDfVjZ 0KlFJ5u1Fky9s0VnB6dOU7EtsiQe4WMT5X98prmB7n2okNvCBaD80eS/Pt+aXs4GZHZA 5zYUAaSzuczDJPpwXwLCZkXqV0LjuZaPQgSLT49Ps1G7boURadoRZv5LEGHG7468wvca eyKF4NERlKgakMV6Y7Kw2aM9fuSi/4PdP4TpQWsNqzEe32qw5mbk1gkG4MevGOIERclm x+Wg== X-Gm-Message-State: AKwxytcb3aaZc5tBBGcDWYdw0yErLMT717zcP9rNr/y4u6xPP2NAOvtk MHyLSg6AXA7j6j5cV9fVG+Ga+aJe X-Google-Smtp-Source: AH8x225r9N0EHjA0hZKVhkScQ6haxTgGDqn7hlVSZ8l2C6W9NPAmHbMhCDX5lWp+yGsz2eR7tc+Qmg== X-Received: by 10.107.178.70 with SMTP id b67mr47140226iof.139.1517684915391; Sat, 03 Feb 2018 11:08:35 -0800 (PST) Received: from mail-io0-f176.google.com (mail-io0-f176.google.com. [209.85.223.176]) by smtp.gmail.com with ESMTPSA id r137sm2795916ita.23.2018.02.03.11.08.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 03 Feb 2018 11:08:35 -0800 (PST) Received: by mail-io0-f176.google.com with SMTP id l17so26369258ioc.3; Sat, 03 Feb 2018 11:08:34 -0800 (PST) X-Received: by 10.107.35.84 with SMTP id j81mr45492009ioj.226.1517684914385; Sat, 03 Feb 2018 11:08:34 -0800 (PST) MIME-Version: 1.0 Reply-To: cem@freebsd.org Received: by 10.2.95.152 with HTTP; Sat, 3 Feb 2018 11:08:34 -0800 (PST) In-Reply-To: <20180203232725.U1389@besplex.bde.org> References: <201801021725.w02HPDaj068477@repo.freebsd.org> <20180203232725.U1389@besplex.bde.org> From: Conrad Meyer Date: Sat, 3 Feb 2018 11:08:34 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r327495 - head/usr.sbin/rpcbind To: Bruce Evans Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 03 Feb 2018 19:31:30 -0000 On Sat, Feb 3, 2018 at 6:46 AM, Bruce Evans wrote: > On Tue, 2 Jan 2018, Conrad Meyer wrote: >> ... > 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). > > I haven't noticed any function related to poll() or kevent that takes a > signal mask. There is the similar function ppoll(), although it complies only with the Linux standard, not POSIX. With kevent, you can simply mask all (or most) signals and watch on a signal event. Conversion to kevent is more complicated, though. > Programs broken by buggy conversion: > ... > - 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. This was not broken by conversion -- it was already broken in this case. If the signal delivery raced with an fprintf, we ended up invoking the stdio atexit handlers via exit(3) call in terminate(), which of course encountered corrupted state. Now it is broken in a slightly different way, yes, if signal delivery races fprintf *and* fprintf is blocked in flow control. This might happen with a slow serial for stderr but seems extraordinarily unlikely otherwise. > ... > 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. Agreed. The signal abstraction is pretty awful. I think the safest way to handle them is to block them entirely, then watch using kqueue()/kevent(). That way you never have to deal with signal context. But that kind of conversion is more work. You also have to deal with EINTR or be ok with blocking signal handling indefinitely. > 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. It does seem like the printf family of routines should be signal-handler safe. Unfortunately, they are implemented in terms of the unsafe stdio streams :-(. > 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. That sounds nice. I'm on board with that. >> ... Non-functional change highlighted ... > > I think this is too specialized and complicated. > > ... > > 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. Feel free to change it to 1s yourself, if you think it is important. >> 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. The type can be changed to volatile sig_atomic_t if you believe plain int will trigger nonsensical compiler behavior. The value is initialized once, before the signal handler is registered, so unless the compiler does something totally insane it should be fine on all architectures FreeBSD runs on. >> + 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. I don't necessarily agree. If the write fails, we missed the signal telling us to terminate the program and will never exit. That said, how would the write ever fail? Conrad