Skip site navigation (1)Skip section navigation (2)
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>