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

[Eygene Ryabinkin]

Konstantin, good day.

Fri, Nov 21, 2014 at 10:32:27PM +0200, Konstantin Belousov wrote:
> On Fri, Nov 21, 2014 at 10:55:55PM +0300, Eygene Ryabinkin wrote:
> > If there is no change in behaviour that will arise from rearranging
> > the order of calls to mach-dependent and mach-independent code,
> > I'd go a bit firther and unify some repeated code,
> >   http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-with-RESETHAND-v2.diff
> > 
> > Works for me too, just tested with the same Squid installation.
>
> This looks correct, but is much more delicate change.
> In particular, the signal mask copied to usermode as part of
> ucontext, for restoration at sigreturn(2), seems to be correct in
> both cases, but in the postsig() case, where sendsig_common() is put
> after sv_sendsig() call, it depends on the returnmask calculation.

Since in original (unmodified) code returnmask is calculated before
the call to kern_sigprocmask(),
{{{
                if (td->td_pflags & TDP_OLDMASK) {
                        returnmask = td->td_oldsigmask;
                        td->td_pflags &= ~TDP_OLDMASK;
                } else
                        returnmask = td->td_sigmask;

                mask = ps->ps_catchmask[_SIG_IDX(sig)];
                if (!SIGISMEMBER(ps->ps_signodefer, sig))
                        SIGADDSET(mask, 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);
}}}
and kern_sigprocmask() only touches td->td_sigmask, such change should
be safe, because mach-dependent handlers do not use or manipulate
td_sigmask.

This looks logical, because we want to pass current original signal mask
for the thread to sv_sendsig() to make it to be restored after signal
handler's work.  So returnmask is to be calculated before mangling
and should not depend on it.

> Can you test that signal mask after signal return is correct with your
> patch?

Tested with the following code:
  http://codelabs.ru/fbsd/patches/libthr/kern-sig-test-sigmask-restoration.c
All tested masks are restored properly both for modified and unmodified
kernels.

> Two other notes about your patch, which should be changed before it is
> committable.  First, the name sendsig_common() is not telling anything.
> Might be, rename the function to postsig_sigprop() or like.
> 
> In the same vein, comment is too ambitious for small helper.  This is not
> The common code, just a helper to handle thread signal mask and several
> other details of delivery.  Just specifying that this is the thing to
> call after sysent->sv_sendsig() to arrange for proper bookkeeping, is
> enough.

Changed it to sendsig_adjust that should carry more sense.
Also modified comment.

> Second, function needs asserts that process lock and sigact mutex
> (ps_mtx) are locked.

Added them too.  New patch is at
  http://codelabs.ru/fbsd/patches/libthr/11-CURRENT-fix-SIGINFO-processing-with-RESETHAND-v3.diff


Thanks!
-- 
Eygene Ryabinkin                                        ,,,^..^,,,
[ Life's unfair - but root password helps!           | codelabs.ru ]
[ 82FE 06BC D497 C0DE 49EC  4FF0 16AF 9EAE 8152 ECFB | freebsd.org ]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 358 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-threads/attachments/20141127/a570ff6e/attachment.sig>