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