svn commit: r265003 - head/secure/usr.sbin/sshd
Jilles Tjoelker
jilles at stack.nl
Fri Aug 22 13:43:57 UTC 2014
On Thu, Aug 21, 2014 at 03:32:46PM +0300, Konstantin Belousov wrote:
> > > I think you mis-interpret the man page statement, it only says that
> > > SA_SIGINFO should not be set in new->sa_flags IMO. But I do not see
> > > much sense in the requirement. Note that we do not test flags for
> > > correctness at all. SUSv4 is also silent on the issue.
> > > If this is important for your case, the following patch prevents
> > > leaking of the flags for ignored of default/action signals. Could
> > > you, please, describe why do you consider this a bug ?
> > IMO, it is an inconsistency to return an invalid old sigaction, I
> > assume that what is returned as the old sigaction should also be valid
> > according to the man page.
> > I realize SUSv4 don't specify such requirement, but it would still be
> > wrong to use SIG_DFL with SA_SIGINFO, since SA_SIGINFO expect the
> > handler to be of the type:
> > void func(int signo, siginfo_t *info, void *context);
> > While SIG_DLF is of type:
> > void func(int signo);
> > There's software out there that (wrongly?) relies on sa_action ==
> > SIG_DFL and (sa_flags & SA_SIGINFO) == 0:
> > http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/libxl_fork.c;h=fa150959adcfa6618342ba1eb0085cbba5f75d0a;hb=HEAD#l338
> > The sa_flags check done here seems too strong in my opinion, but I
> > still think it's right according to the man page.
> Apparently, the original problem requires /bin/sh as the login shell to
> reproduce, while I used zsh on the test box.
> Below is the patch which fixes reset of flags both for sigaction(2)
> and execve(2).
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 561ea0a..4077ec9 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -621,6 +621,15 @@ sig_ffs(sigset_t *set)
> return (0);
> }
>
> +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);
> +}
> +
> /*
> * kern_sigaction
> * sigaction
> @@ -679,7 +688,7 @@ kern_sigaction(td, sig, act, oact, flags)
>
> ps->ps_catchmask[_SIG_IDX(sig)] = act->sa_mask;
> SIG_CANTMASK(ps->ps_catchmask[_SIG_IDX(sig)]);
> - if (act->sa_flags & SA_SIGINFO) {
> + if (sigact_flag_test(act, SA_SIGINFO)) {
> ps->ps_sigact[_SIG_IDX(sig)] =
> (__sighandler_t *)act->sa_sigaction;
> SIGADDSET(ps->ps_siginfo, sig);
> @@ -687,19 +696,19 @@ kern_sigaction(td, sig, act, oact, flags)
> ps->ps_sigact[_SIG_IDX(sig)] = act->sa_handler;
> SIGDELSET(ps->ps_siginfo, sig);
> }
> - if (!(act->sa_flags & SA_RESTART))
> + if (sigact_flag_test(act, SA_RESTART))
> SIGADDSET(ps->ps_sigintr, sig);
> else
> SIGDELSET(ps->ps_sigintr, sig);
> - if (act->sa_flags & SA_ONSTACK)
> + if (sigact_flag_test(act, SA_ONSTACK))
> SIGADDSET(ps->ps_sigonstack, sig);
> else
> SIGDELSET(ps->ps_sigonstack, sig);
> - if (act->sa_flags & SA_RESETHAND)
> + if (sigact_flag_test(act, SA_RESETHAND))
> SIGADDSET(ps->ps_sigreset, sig);
> else
> SIGDELSET(ps->ps_sigreset, sig);
> - if (act->sa_flags & SA_NODEFER)
> + if (sigact_flag_test(act, SA_NODEFER))
> SIGADDSET(ps->ps_signodefer, sig);
> else
> SIGDELSET(ps->ps_signodefer, sig);
> @@ -935,6 +944,11 @@ execsigs(struct proc *p)
> sigqueue_delete_proc(p, sig);
> }
> ps->ps_sigact[_SIG_IDX(sig)] = SIG_DFL;
> + SIGDELSET(ps->ps_siginfo, sig);
> + SIGDELSET(ps->ps_sigintr, sig);
> + SIGDELSET(ps->ps_sigonstack, sig);
> + SIGDELSET(ps->ps_sigreset, sig);
> + SIGDELSET(ps->ps_signodefer, sig);
> }
> /*
> * Reset stack state to the user stack.
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.)
I notice a bug in POSIX here: it should specify that execve() clear
SA_SIGINFO bits when it resets signals to SIG_DFL.
--
Jilles Tjoelker
More information about the svn-src-head
mailing list