Date: Fri, 22 Aug 2014 15:43:53 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Roger Pau Monn? <royger@FreeBSD.org>, src-committers@freebsd.org, Bryan Drewery <bdrewery@FreeBSD.org> Subject: Re: svn commit: r265003 - head/secure/usr.sbin/sshd Message-ID: <20140822134353.GA21891@stack.nl> In-Reply-To: <20140821123246.GH2737@kib.kiev.ua> References: <201404270528.s3R5SEIm054377@svn.freebsd.org> <53F4B381.5010205@FreeBSD.org> <20140820151310.GB2737@kib.kiev.ua> <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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140822134353.GA21891>