svn commit: r265003 - head/secure/usr.sbin/sshd

[Konstantin Belousov]

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.

> 
> > 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 ?

> 
> 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);
 }
 
 /*
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20140823/cb9d2515/attachment.sig>