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

[Konstantin Belousov]

On Thu, Nov 20, 2014 at 09:32:19PM +0300, Eygene Ryabinkin wrote:
> Good day.
> 
> Was hacking on Squid that used to dump core when signal 15 was
> delivered to it via 'squid -k shutdown' and found out that the reason
> for core dumps is that thr_sighandler() is _sometimes_ passed 0x10001
> as the value of "info" argument despite that _sigaction() always arms
> flags for __sys_sigaction() with SA_SIGINFO.
> 
> But looking at handle_signal() I had discovered that if libthr client
> wants no SA_SIGINFO, then actp->sa_sigaction() will be called, though
> specs,
>   http://pubs.opengroup.org/onlinepubs/009695399/functions/sigaction.html
> say that in this case sa_handler should be used.  And this is consistent
> with the non-threaded case.
The sa_sigaction and sa_handler live in the same bits, look at the
union in the definition of the struct sigaction in sys/signal.h. It is
the same pointer, cast to different functions signature depending on
SA_SIGSINFO.

> 
> Moreover, the called handler is passed 4 extra arguments,
> {{{
>   info->si_code, (struct sigcontext *)ucp, info->si_addr, (__sighandler_t *)sigfunc);
> }}}
The additional arguments for sa_handler is traditional BSD behaviour.
All ABIs allow the undeclared arguments after the required, so it works.

> and for my poor Squid this is the root of the problem, because 0x10001
> gets dereferenced (as info->SOMETHING), so SIGSEGV gets delivered
> immediately.  By the way, the issue with seamonkey (but not Python) at
>   https://lists.freebsd.org/pipermail/freebsd-current/2013-June/042199.html
>   https://lists.freebsd.org/pipermail/freebsd-current/2013-June/042210.html
> has the same pattern: inside one invocation of signal handler we see
> SIGSEGV that can be triggered by this very problem: frame #5 handles
> initial signal, while frame #4 is in-kernel stuff for SIGSEGV and
> #3 is its userspace counterpart that takes us into libthr once again,
> but this time for the different signal, I guess.  And this one goes
> with proper "info", so no more harm is inflicted by libthr's code.
Except the same pattern, which really boils down to the fact that
fault happens in the signal handler code, I do not see anything similar.

> 
> 
> The origin of 0x10001 as info isn't yet clear to me, though I have
> a feeling that it is SI_USER that is slipping somewhere to the wrong
> place.  Will dig further.
It is not clear to me what your problem is, at all.  Compile the faulting
program with the debugging information, possibly compile 
libc/libthr/ld-elf.so with debugging as well, and obtain backtrace
from the coredump.

I briefly looked at the squid 3.4.9 source code, and one outstanding
thing there is the use of streams in the signal handler. Since signals
are async, and there is no masking of signals around << calls in the
code, I would be not much surprised that the cause is the interruption
of another <<.

> 
> 
> Anyway, it seems like that the currend code inside handle_signal()
> should be modified with the patch like
>   http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-sighandler-invocation.patch
> 
I do not see how the patch could have any effect on the issue mentioned,
and it definitely introduces ABI incompatibility (bug) by removing the
traditional arguments.

> Could anyone, please, review it and, probably, come up with the idea of
> why the current code is such as it is.  Revision when it first appeared
> is 212076,
>   https://svnweb.freebsd.org/base/head/lib/libthr/thread/thr_sig.c?r1=211737&r2=212076&view=patch
> 
> Thanks.
> 
> 
> PS: I am not subscribed to the list, so, please, keep me CC'ed.
> -- 
> Eygene Ryabinkin                                        ,,,^..^,,,
> [ Life's unfair - but root password helps!           | codelabs.ru ]
> [ 82FE 06BC D497 C0DE 49EC  4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]