[PATCH] os: don't unconditionally unblock SIGIO in OsReleaseSignals()

[Peter Hutterer]

On Sun, Aug 05, 2012 at 10:06:34AM -0700, Jeremy Huddleston Sequoia wrote:
> > -        sigprocmask(SIG_UNBLOCK, &set, NULL);
> > +        sigprocmask(SIG_SETMASK, &PreviousSigIOMask, 0);
> 
> This should still be NULL instead of 0.

Will fix, though this was 0 to be identical to the command in OsReleaseSignals().
I guess I'll just sneak that fix too.

> You're now just dropping set on the floor, so you might as well not create it.

done, thanks.

> Overall, this change seems very fragile.
> 
> OsReleaseSIGIO and OsBlockSIGIO are well balanced.  I think a safer fix
> would be to change OsReleaseSignals().

unfortunately, the problem is not with OsReleaseSignals(), the problem is
OsBlockSIGIO(), which only looks balanced. when OsBlockSIGIO() is called
from within the signal handler, SIGIO is already in the sigprocmask, and
that must not be undone by the matching OsReleaseSIGIO(), even if the count
reaches zero.

For all other signals, this works because we remember the mask and re-apply
it. For SIGIO, it is simply unblocked. My patch would simply align the sigio
code with the other code, the functions are largely identical after.

> void
> OsReleaseSignals(void)
> {
> #ifdef SIG_BLOCK
>     if (--BlockedSignalCount == 0) {
>         sigprocmask(SIG_SETMASK, &PreviousSignalMask, 0);
> 
> #ifdef SIGIO
>         OsReleaseSIGIO();

with this code, you'd get a SIGIO here, leading to the same bug.

Cheers,
   Peter


> 
>         /* OsReleaseSIGIO only unblocks if necessary;
>          * we need to re-add SIGIO to the sigmask if we removed it with the sigprocmask above
>          */
>         if (sigio_blocked > 0) {
>             sigset_t set;
>             sigemptyset(&set);
>             sigaddset(&set, SIGIO);
>             sigprocmask(SIG_BLOCK, &set, NULL);
>         }
> #endif
>     }
> #endif
> }

> 
> 
> On Aug 4, 2012, at 00:41, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> 
> > Calling OsReleaseSignal() inside the signal handler releases SIGIO, causing
> > the signal handler to be called again from within the handler.
> > 
> > Practical use-case: when synaptics calls TimerSet in the signal handler,
> > this causes the signals to be released, eventually hanging the server.
> > 
> > Regression introduced in 08962951de.
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > os/utils.c |    9 +++++----
> > test/os.c  |   36 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 41 insertions(+), 4 deletions(-)
> > 
> > diff --git a/os/utils.c b/os/utils.c
> > index cb37162..afd50b3 100644
> > --- a/os/utils.c
> > +++ b/os/utils.c
> > @@ -1186,6 +1186,7 @@ OsBlockSignals(void)
> > 
> > #ifdef SIG_BLOCK
> > static sig_atomic_t sigio_blocked;
> > +static sigset_t PreviousSigIOMask;
> > #endif
> > 
> > /**
> > @@ -1198,13 +1199,13 @@ OsBlockSIGIO(void)
> > #ifdef SIGIO
> > #ifdef SIG_BLOCK
> >     if (sigio_blocked++ == 0) {
> > -        sigset_t set, old;
> > +        sigset_t set;
> >         int ret;
> > 
> >         sigemptyset(&set);
> >         sigaddset(&set, SIGIO);
> > -        sigprocmask(SIG_BLOCK, &set, &old);
> > -        ret = sigismember(&old, SIGIO);
> > +        sigprocmask(SIG_BLOCK, &set, &PreviousSigIOMask);
> > +        ret = sigismember(&PreviousSigIOMask, SIGIO);
> >         return ret;
> >     } else
> >         return 1;
> > @@ -1222,7 +1223,7 @@ OsReleaseSIGIO(void)
> > 
> >         sigemptyset(&set);
> >         sigaddset(&set, SIGIO);
> > -        sigprocmask(SIG_UNBLOCK, &set, NULL);
> > +        sigprocmask(SIG_SETMASK, &PreviousSigIOMask, 0);
> >     } else if (sigio_blocked < 0) {
> >         BUG_WARN(sigio_blocked < 0);
> >         sigio_blocked = 0;
> > diff --git a/test/os.c b/test/os.c
> > index 1460a34..8f1107d 100644
> > --- a/test/os.c
> > +++ b/test/os.c
> > @@ -28,6 +28,21 @@
> > #include <signal.h>
> > #include "os.h"
> > 
> > +static int last_signal = 0;
> > +static int expect_signal = 0;
> > +
> > +static void sighandler(int signal)
> > +{
> > +    assert(expect_signal);
> > +    expect_signal = 0;
> > +    if (!last_signal)
> > +        raise(signal);
> > +    OsBlockSignals();
> > +    OsReleaseSignals();
> > +    last_signal = 1;
> > +    expect_signal = 1;
> > +}
> > +
> > static int
> > sig_is_blocked(int sig)
> > {
> > @@ -118,7 +133,27 @@ static void block_sigio_test(void)
> >     assert(sig_is_blocked(SIGIO));
> >     OsReleaseSignals();
> >     assert(!sig_is_blocked(SIGIO));
> > +#endif
> > +}
> > 
> > +static void block_sigio_test_nested(void)
> > +{
> > +#ifdef SIG_BLOCK
> > +    /* Check for bug releasing SIGIO during SIGIO signal handling.
> > +       test case:
> > +           raise signal
> > +           → in signal handler:
> > +                raise signal
> > +                OsBlockSignals()
> > +                OsReleaseSignals()
> > +                tail guard
> > +       tail guard must be hit.
> > +     */
> > +    sighandler_t old_handler;
> > +    old_handler = signal(SIGIO, sighandler);
> > +    expect_signal = 1;
> > +    assert(raise(SIGIO) == 0);
> > +    assert(signal(SIGIO, old_handler) == sighandler);
> > #endif
> > }
> > 
> > @@ -126,5 +161,6 @@ int
> > main(int argc, char **argv)
> > {
> >     block_sigio_test();
> > +    block_sigio_test_nested();
> >     return 0;
> > }
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 
>