[CFR][PATCH] Call proper signal handler from libthr for non-SA_SIGINFO case

[Konstantin Belousov]

On Fri, Nov 21, 2014 at 07:07:14PM +0300, Eygene Ryabinkin wrote:
> The problem is that libthr's _sigaction() proxies sa_flags, so if
> SA_RESETHAND was specified by the caller, it will also be passed to
> the kernel when thr_sighandler() is installed.  And since sa_flags are
> equipped with SA_SIGINFO inside _sigaction(), thr_sighandler() and
> handle_signal() always expect to be called with POSIX SA_SIGINFO
> semantics.
> 
> This isn't the case for SA_RESETHAND, because sigdflt() from
> kern/kern_sig.c will be called before mach-dependent sv_sendsig
> vector, so
> {{{
> SIGDELSET(ps->ps_siginfo, sig)
> }}}
> will be issued.  Thus, any code inside mach-dependent handler won't
> see ps_siginfo, so will always use traditional semantics.  This
> explains why I see 0x10001 as "siginfo_t *info": it is just "int code"
> for the traditional case and the signal in question is really produced
> by userland, so it is SI_USER.
Great, thank you for tracking this down.

> 
> So, it looks like a kernel bug (since if we request SA_SIGINFO, we
> should get the proper handler to be called even for the SA_RESETHAND
> case).  I see two possibilities:
> 
>  - invoke SA_RESETHAND processing inside mach-dependent code; that's
>    a kind of ugly and makes mach-specific code to deal with the
>    generic signal handling logics;
> 
>  - pass information about SA_SIGINFO "out-of-band" (not in
>    ps->ps_siginfo).
> 
> We can't postpone sigdflt() to be called after signal being delivered,
> since spec requires it to be done before calling user-space handler.
No, userspace is not called from sv_sendsig(), it simply cannot work
because nested signals would cause nesting on the kernel stack frame.
sv_sendsig() only prepares the usermode signal stack frame and arranges
the saved thread CPU state so that on return from kernel to usermode,
the signal handler is executed.

And, amusingly, the only thing which sv_sendsig() methods need and which
is also touched by sigdflt(), is ps_siginfo.  Simply rearranging the
order of calls should be enough.  I put the patch at the end of message,
it worked for me.

> 
> Had created a patch that adds 4th argument to sv_sendsig and fixes
> this problem:
>   http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-with-RESETHAND.diff
> This changes internal KABI, but hopefully sv_sendsig is an internal
> kernel interface that isn't used by anything else outside kern_sig.c.
> 
> It works fine with virgin libthr and solves Squid restart problem.
> Will try to install it to more test machines and see if it will work
> as expected.  Seems like a kernel regression test like the attached
> one will also be handy.

There are people who develop the regression suite(s) for FreeBSD.
Might be, they would note your test, may be, you should somehow contact
them and propose the inclusion of the test into suite.  I believe
it is freebsd-testing at f.o.  The test would require adoption to the
framework.

diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
index 5cdc2ce..2c3cc80 100644
--- a/sys/kern/kern_sig.c
+++ b/sys/kern/kern_sig.c
@@ -2863,14 +2863,14 @@ postsig(sig)
 		kern_sigprocmask(td, SIG_BLOCK, &mask, NULL,
 		    SIGPROCMASK_PROC_LOCKED | SIGPROCMASK_PS_LOCKED);
 
-		if (SIGISMEMBER(ps->ps_sigreset, sig))
-			sigdflt(ps, sig);
 		td->td_ru.ru_nsignals++;
 		if (p->p_sig == sig) {
 			p->p_code = 0;
 			p->p_sig = 0;
 		}
 		(*p->p_sysent->sv_sendsig)(action, &ksi, &returnmask);
+		if (SIGISMEMBER(ps->ps_sigreset, sig))
+			sigdflt(ps, sig);
 	}
 	return (1);
 }