Possible bug in libc/sysdeps/linux/sparc/sigaction.c

[Denys Vlasenko]

On Monday 23 November 2009 04:24, Austin Foxley wrote:
> On 07/21/2009 04:53 PM, Denys Vlasenko wrote:
> > Is sparc special in this regard?
> 
> Not special for a good reason, just old. The bug you noted is fixed in 
> master, and will be in the 0.9.30.2 release.

There are a few more problems.

int __libc_sigaction(int sig, const struct sigaction *act, struct sigaction *oact)
{
        int ret;
        struct sigaction kact, koact;
        unsigned long stub = 0;

        if (act) {
                kact.sa_handler = act->sa_handler;
                memcpy (&kact.sa_mask, &act->sa_mask, sizeof (sigset_t));

For me it is not at all obviousl that this memcpy is correct.
It looks like it copies 64bit object (act->sa_mask, which is sigset_t)
into 32bit field (long kact.sa_mask). longs are 32-bit on sparcs, right?

Why not simply kact.sa_mask = act->sa_mask.__val[0]; ?
It's demonstrably correct (both sides are longs).

                if (((kact.sa_flags = act->sa_flags) & SA_SIGINFO) != 0)
                        stub = (unsigned long) &__rt_sigreturn_stub;
                else
                        stub = (unsigned long) &__sigreturn_stub;
                stub -= 8;
                kact.sa_restorer = NULL;
        }

        /* XXX The size argument hopefully will have to be changed to the
         * real size of the user-level sigset_t.  */
        ret = INLINE_SYSCALL (rt_sigaction, 5, sig, act ? &kact : 0,
                        oact ? &koact : 0, stub, _NSIG / 8);

        if (oact && ret >= 0) {
                oact->sa_handler = koact.sa_handler;
                memcpy (&oact->sa_mask, &koact.sa_mask, sizeof (sigset_t));
Same here. Copying 64 bits out of 32-bit object!?
It should put garbage into high half of oact->sa_mask.

You can do oact->sa_mask.__val[0] = koact.sa_mask; instead.

                oact->sa_flags = koact.sa_flags;
                oact->sa_restorer = koact.sa_restorer;
        }
        return ret;
}

More benign problems are lying comment (size argument *must not*
be changed, syscall won't work if it is) and readability,
like assignment jammed inside a complex statement:
if (((kact.sa_flags = act->sa_flags) & SA_SIGINFO) != 0) ...


There are plenty of arches with better __libc_sigaction()s
to imitate.

--
vda