| Message ID | 20120804074111.GA19629@yabbi.redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
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; }
> - 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@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@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@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel
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@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@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@lists.x.org: X.Org development > > Archives: http://lists.x.org/archives/xorg-devel > > Info: http://lists.x.org/mailman/listinfo/xorg-devel > >
On Aug 5, 2012, at 23:18, Peter Hutterer <peter.hutterer@who-t.net> wrote: >> 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. Gah... yeah... blech... Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com> Although I am 90% certain this is going to come back and bite us some day...
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@who-t.net> --- os/utils.c | 9 +++++---- test/os.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-)