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

[Jeremy Huddleston Sequoia]

> -        sigprocmask(SIG_UNBLOCK, &set, NULL);
> +        sigprocmask(SIG_SETMASK, &PreviousSigIOMask, 0);

This should still be NULL instead of 0.
You're now just dropping set on the floor, so you might as well not create it.

Overall, this change seems very fragile.

OsReleaseSIGIO and OsBlockSIGIO are well balanced.  I think a safer fix would be to change OsReleaseSignals().

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

#ifdef SIGIO
        OsReleaseSIGIO();

        /* 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