From owner-svn-src-head@FreeBSD.ORG Sun Aug 24 13:13:28 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BA11461B; Sun, 24 Aug 2014 13:13:28 +0000 (UTC) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mailhost.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4B59E3B51; Sun, 24 Aug 2014 13:13:28 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 90E1F358C64; Sun, 24 Aug 2014 15:13:24 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id 7360628494; Sun, 24 Aug 2014 15:13:24 +0200 (CEST) Date: Sun, 24 Aug 2014 15:13:24 +0200 From: Jilles Tjoelker To: Konstantin Belousov Subject: Re: svn commit: r265003 - head/secure/usr.sbin/sshd Message-ID: <20140824131324.GA37218@stack.nl> References: <53F4BC9B.3090405@FreeBSD.org> <53F4BEB1.6070000@FreeBSD.org> <53F4C022.5050804@FreeBSD.org> <20140821080541.GE2737@kib.kiev.ua> <53F5D42E.9080908@FreeBSD.org> <20140821123246.GH2737@kib.kiev.ua> <20140822134353.GA21891@stack.nl> <20140822153139.GN2737@kib.kiev.ua> <20140823175244.GA29648@stack.nl> <20140823184221.GV2737@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140823184221.GV2737@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Roger Pau Monn? , src-committers@freebsd.org, Bryan Drewery X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 24 Aug 2014 13:13:28 -0000 On Sat, Aug 23, 2014 at 09:42:21PM +0300, Konstantin Belousov wrote: > On Sat, Aug 23, 2014 at 07:52:44PM +0200, Jilles Tjoelker wrote: > > On Fri, Aug 22, 2014 at 06:31:39PM +0300, Konstantin Belousov wrote: > > > On Fri, Aug 22, 2014 at 03:43:53PM +0200, Jilles Tjoelker wrote: > > > > This is good and necessary for SA_SIGINFO (because of the type of the > > > > SIG_DFL and SIG_IGN constants, and because POSIX says so in the > > > > description of SA_RESETHAND in the sigaction() page). However, there > > > > seems no reason to clear the other flags, which have no effect when the > > > > disposition is SIG_DFL or SIG_IGN but have historically always been > > > > preserved. (Slight exception: if kernel code erroneously loops on > > > > ERESTART, it will eat CPU time iff SA_RESTART is set, independent of the > > > > signal's disposition.) > > > Well, I already committed the patch with several bugs fixed comparing > > > with what was mailed, before your feedback arrived. > > > Do you consider it is important enough to revert the resetting of other > > > flags ? In particular, your note about the traditional historic > > > behaviour makes me wonder. > > I consider it important enough. Clearing the other flags is not > > POSIX-compliant and might break applications. For example, I can imagine > > an application modifying a struct sigaction with sa_handler == SIG_DFL > > from a sigaction() call. > This feels somewhat strange to me. E.g., I can easily imagine an > implementation which relies on some code executing in the process > user context for default action on some signal. Having the flags, > like SA_ONSTACK or SA_NODEFER to influence the handler is weird. > Such implementation is not unix, but I think it is quite possible > that cygwin or interix do core dumping in userspace. The implementation of SA_ONSTACK and the like could just as well be tied to a user-specified handler function. Anyway, this imaginable application is slightly dumb, since it is either blindly using SA_* flags from its parent or asking the kernel for things it already knows. The latter might be done for "modularity" reasons. > > > I do not see why SA_SIGINFO is so special that it must be reset, > > > while other flags are not. The absence of the cases where the > > > default/ignored disposition is affected by the flags seems rather > > > arbitrary. > > The difference is that SA_SIGINFO changes the disposition field from > > sa_handler to sa_sigaction, and it is not unambiguously clear how > > SIG_DFL and SIG_IGN are represented in sa_sigaction. Note that > > sa_handler and sa_sigaction may or may not share storage, and > > implementations may or may not support (void (*)(int, siginfo_t *, void > > *))SIG_DFL. > > For example, when I wrote system() using posix_spawn() in > > https://lists.freebsd.org/pipermail/freebsd-hackers/2012-July/040065.html > > I needed to know whether SIGINT and SIGQUIT were ignored or not. I wrote > > > > ] if ((intact.sa_flags & SA_SIGINFO) != 0 || > > ] intact.sa_handler != SIG_IGN) > > ] (void)sigaddset(&defmask, SIGINT); > > ] if ((quitact.sa_flags & SA_SIGINFO) != 0 || > > ] quitact.sa_handler != SIG_IGN) > > ] (void)sigaddset(&defmask, SIGQUIT); > > > > in the assumption that there is always an actual handler if SA_SIGINFO > > is set. I did not really like this code and eventually system() was > > changed to use vfork() directly instead of posix_spawn(), but I think it > > is correct. > If the implementation provides separate storage for sa_handler and > sa_sigaction, then isn't it more correct to assume that sa_handler is > NULL when sa_sigaction is valid ? In other words, simple > sa.sa_handler != SIG_IGN > test would be more reasonable. > I see that the SUSv4 is worded in a way that SA_SIGINFO always > accompanies sa_sigaction. Are there any implementations which > separate the storage for sa_handler and sa_sigaction ? SUSv4 doesn't allow making any such assumption. The check for (sa.sa_handler != SIG_IGN) will work fine on most implementations but an application doing it is not POSIX-compliant. I don't know of implementations that do not put sa_handler and sa_sigaction in a union. > > Note that this means that it is sometimes necessary to install a handler > > function that will never be called. Per POSIX, SA_SIGINFO must be set > > for sigwaitinfo() to be guaranteed to return siginfo_t data, and the > > only way to do this is to specify a handler function, even though it > > will never be called because the signal is masked. (A never-called > > handler function also needs to be specified when using sigwait-like > > functions with signals that default to ignore.) > > The other flags do not affect the representation of the disposition, and > > can therefore remain set without problem. > Anyway, below is the patch with reverts the behaviour WRT flags other > than SA_SIGINFO. I like the compactness of sigact_flag_test() calls, > so I kept the function, despite it is only used in non-trivial ways > for SA_SIGINFO flag test in kern_sigaction(). > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > index 8810bf3..f73c801 100644 > --- a/sys/kern/kern_sig.c > +++ b/sys/kern/kern_sig.c > @@ -625,9 +625,14 @@ static bool > sigact_flag_test(struct sigaction *act, int flag) > { > > - return ((act->sa_flags & flag) != 0 && > - (__sighandler_t *)act->sa_sigaction != SIG_IGN && > - (__sighandler_t *)act->sa_sigaction != SIG_DFL); > + /* > + * SA_SIGINFO is reset when signal disposition is set to > + * ignore or default. Other flags are kept according to user > + * settings. > + */ > + return ((act->sa_flags & flag) != 0 && (flag != SA_SIGINFO || > + ((__sighandler_t *)act->sa_sigaction != SIG_IGN && > + (__sighandler_t *)act->sa_sigaction != SIG_DFL))); > } > > /* > @@ -916,7 +921,6 @@ siginit(p) > for (i = 1; i <= NSIG; i++) { > if (sigprop(i) & SA_IGNORE && i != SIGCONT) { > SIGADDSET(ps->ps_sigignore, i); > - SIGADDSET(ps->ps_sigintr, i); > } > } > mtx_unlock(&ps->ps_mtx); > @@ -936,10 +940,6 @@ sigdflt(struct sigacts *ps, int sig) > SIGADDSET(ps->ps_sigignore, sig); > ps->ps_sigact[_SIG_IDX(sig)] = SIG_DFL; > SIGDELSET(ps->ps_siginfo, sig); > - SIGADDSET(ps->ps_sigintr, sig); > - SIGDELSET(ps->ps_sigonstack, sig); > - SIGDELSET(ps->ps_sigreset, sig); > - SIGDELSET(ps->ps_signodefer, sig); > } > > /* Looks good to me. -- Jilles Tjoelker