[Patch] Fix race with signals in signal_block()

[Patch] Fix race with signals in signal_block()

[Guillaume Chazarain]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Hi !

Here is patch to fix a bug where zsh can become unkillable (except of 
course with SIGKILL), I use Linux-2.6/i386 so the POSIX_SIGNALS macro is 
defined.

signal_block() and signal_unblock() use two global variables 
dummy_sigset1 and dummy_sigset2. I observed a race between
child_block() and
oldmask = signal_block(newmask); /* Block all signals temporarily */
called in the zhandler() signal handler.

The attached patch fixes the problem by getting rid of the global 
variables, and instead using the signal_block() and signal_unblock() 
functions.

Hopefully I did not break the NO_SIGNAL_BLOCKING, SYSV_SIGNALS and 
BSD_SIGNALS cases.

Cheers.

-- 
Guillaume

[-- Attachment #2: zsh-signal-race.diff --]
[-- Type: text/x-patch, Size: 2589 bytes --]

--- zsh-4.3.4/Src/signals.c
+++ zsh-4.3.4/Src/signals.c
@@ -200,15 +200,6 @@ signal_mask(int sig)
  * set. Return the old signal set.       */
 
 /**/
-#ifdef POSIX_SIGNALS
-
-/**/
-mod_export sigset_t dummy_sigset1, dummy_sigset2;
-
-/**/
-#else
-
-/**/
 #ifndef BSD_SIGNALS
 
 sigset_t
@@ -216,7 +207,11 @@ signal_block(sigset_t set)
 {
     sigset_t oset;
  
-#ifdef SYSV_SIGNALS
+#ifdef POSIX_SIGNALS
+    sigprocmask(SIG_BLOCK, &set, &oset);
+
+#else
+# ifdef SYSV_SIGNALS
     int i;
  
     oset = blocked_set;
@@ -226,7 +221,7 @@ signal_block(sigset_t set)
             sighold(i);
         }
     }
-#else  /* NO_SIGNAL_BLOCKING */
+# else  /* NO_SIGNAL_BLOCKING */
 /* We will just ignore signals if the system doesn't have *
  * the ability to block them.                             */
     int i;
@@ -238,7 +233,8 @@ signal_block(sigset_t set)
             signal_ignore(i);
         }
    }
-#endif /* SYSV_SIGNALS  */
+# endif /* SYSV_SIGNALS */
+#endif /* POSIX_SIGNALS */
  
     return oset;
 }
@@ -246,19 +242,17 @@ signal_block(sigset_t set)
 /**/
 #endif /* BSD_SIGNALS */
 
-/**/
-#endif /* POSIX_SIGNALS */
-
 /* Unblock the signals in the given signal *
  * set. Return the old signal set.         */
 
-#ifndef POSIX_SIGNALS
-
 sigset_t
 signal_unblock(sigset_t set)
 {
     sigset_t oset;
- 
+
+#ifdef POSIX_SIGNALS
+    sigprocmask(SIG_UNBLOCK, &set, &oset);
+#else
 # ifdef BSD_SIGNALS
     sigfillset(&oset);
     oset = sigsetmask(oset);
@@ -288,12 +282,11 @@ signal_unblock(sigset_t set)
    }
 #  endif /* SYSV_SIGNALS  */
 # endif  /* BSD_SIGNALS   */
+#endif   /* POSIX_SIGNALS */
  
     return oset;
 }
 
-#endif   /* POSIX_SIGNALS */
-
 /* set the process signal mask to *
  * be the given signal mask       */
 
--- zsh-4.3.4/Src/signals.h
+++ zsh-4.3.4/Src/signals.h
@@ -100,26 +100,10 @@
 
 #define restore_queue_signals(q) (queueing_enabled = (q))
 
-/* Make some signal functions faster. */
-
-#ifdef POSIX_SIGNALS
-#define signal_block(S) \
-    ((dummy_sigset1 = (S)), \
-     sigprocmask(SIG_BLOCK, &dummy_sigset1, &dummy_sigset2), \
-     dummy_sigset2)
-#else
-# ifdef BSD_SIGNALS
+#ifdef BSD_SIGNALS
 #define signal_block(S) sigblock(S)
-# else
+#else
 extern sigset_t signal_block _((sigset_t));
-# endif  /* BSD_SIGNALS   */
-#endif   /* POSIX_SIGNALS */
+#endif  /* BSD_SIGNALS   */
 
-#ifdef POSIX_SIGNALS
-#define signal_unblock(S) \
-    ((dummy_sigset1 = (S)), \
-     sigprocmask(SIG_UNBLOCK, &dummy_sigset1, &dummy_sigset2), \
-     dummy_sigset2)
-#else
 extern sigset_t signal_unblock _((sigset_t));
-#endif   /* POSIX_SIGNALS */

Re: [Patch] Fix race with signals in signal_block()

[Peter Stephenson]

Guillaume Chazarain <guichaz@yahoo.fr> wrote:
> Here is patch to fix a bug where zsh can become unkillable (except of 
> course with SIGKILL), I use Linux-2.6/i386 so the POSIX_SIGNALS macro
> is defined.
> 
> signal_block() and signal_unblock() use two global variables 
> dummy_sigset1 and dummy_sigset2. I observed a race between
> child_block() and
> oldmask = signal_block(newmask); /* Block all signals temporarily */
> called in the zhandler() signal handler.
> 
> The attached patch fixes the problem by getting rid of the global 
> variables, and instead using the signal_block() and signal_unblock() 
> functions.

Looking at your patch, it's hard to see what it wasn't done that way
before... I couldn't see anything obviously wrong and I've committed it.

Thanks.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview