Weird signal handling bug 221635

[Colin Watson]

On Thu, May 01, 2008 at 06:34:23PM +0100, Matt Zimmerman wrote:
> On Wed, Apr 30, 2008 at 11:15:23AM +0100, Colin Watson wrote:
> > While I've weakened the assertion upstream for other reasons (i.e. the
> > assert function isn't async-signal-safe, though that wouldn't cause this
> > bug), and may well propose this for 8.04.1, it worries me that a signal
> > handler is being called with a signal number that wasn't requested by
> > sigaction; the consequences might be more serious elsewhere. Can anyone
> > see anything wrong with my code, or think of a kernel bug that might
> > cause this?
> 
> My only guess would be some sloppy handling of a struct sigaction somewhere
> which led to it being re-used with old data.  I noticed the code saves and
> restores the SIGCHLD action a few times...

True, but that's done with:

#ifdef SA_RESTART
        /* We rely on getting EINTR from select. */
        sigaction (SIGCHLD, NULL, &sa);
        sa.sa_flags &= ~SA_RESTART;
        sigaction (SIGCHLD, &sa, NULL);
#endif

[...]

#ifdef SA_RESTART
        sigaction (SIGCHLD, NULL, &sa);
        sa.sa_flags |= SA_RESTART;
        sigaction (SIGCHLD, &sa, NULL);
#endif

... around a critical section. Obviously I'm not insisting my code is
correct, but am at a loss to see what might be wrong here.

Soren suggested that this might be due to not zeroing the struct
sigaction everywhere, which is a legitimate point but my code fills in
all the documented elements. The only way I can think of this to fail is
if the kernel looks at the value of sa_restorer (which would be
undefined) and does something with it; but on inspection the libc
sigaction stub seems to set sa_restorer itself so this seems unlikely.

I suppose another possibility is that sigaction fails in the
save/restore code above. That would certainly cause this problem,
although the manual page only documents two reasons for failure neither
of which seem like they could apply here. POSIX doesn't specify
sigaction as being interruptible so it presumably can't be EINTR.

Thanks,

-- 
Colin Watson                                       [cjwatson at ubuntu.com]