This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 11/15] Hurd signals: fix sigwait() for global signals


On Sun, Jul 03, 2011 at 02:32:00AM +0200, Samuel Thibault wrote:
> Jeremie Koenig, le Wed 29 Jun 2011 18:30:23 +0200, a écrit :
> > * sysdeps/mach/hurd/sigwait.c (__sigwait): Change the blocking mask
> > temporarily so that we catch global as well as thread-specific signals.
> 
> Mmm, this is unsafe: if yet another signal arrives between the
> setjmp return and locking ss, it will be processed instead of being
> blocked.

The attached patch fixes this: the blocking mask is restored by the
preemptor rather than by sigwait() itself.

While testing it I stumbled upon another (long-standing) bug whereby
sigwait does not clear a signal which was pending prior to calling it.
The other attached patch fixes that.

-- 
Jeremie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org
commit 65dbf10c7cb179f0f373dca0a4983f358e8d695d
Author: Jeremie Koenig <jk@jk.fr.eu.org>
Date:   Tue Jul 19 20:56:16 2011 +0000

    patch 11: fix race condition with the blocking mask

diff --git a/hurd/hurdsig.c b/hurd/hurdsig.c
index 91917aa..67037e8 100644
--- a/hurd/hurdsig.c
+++ b/hurd/hurdsig.c
@@ -683,40 +683,45 @@ post_signal (struct hurd_sigstate *ss,
     struct hurd_sigstate *rss;
 
     __mutex_lock (&_hurd_siglock);
     for (rss = _hurd_sigstates; rss != NULL; rss = rss->next)
       {
 	if (! sigstate_is_global_rcv (rss))
 	  continue;
 
 	/* The global sigstate is already locked.  */
 	__spin_lock (&rss->lock);
 	if (! __sigismember (&rss->blocked, signo))
 	  {
 	    ss = rss;
 	    break;
 	  }
 	__spin_unlock (&rss->lock);
       }
     __mutex_unlock (&_hurd_siglock);
   }
 
+  /* We want the preemptors to be able to update the blocking mask
+     without affecting the delivery of this signal, so we save the
+     current value to test against later.  */
+  sigset_t blocked = ss->blocked;
+
   /* Check for a preempted signal.  Preempted signals can arrive during
      critical sections.  */
   {
     inline sighandler_t try_preemptor (struct hurd_signal_preemptor *pe)
       {				/* PE cannot be null.  */
 	do
 	  {
 	    if (HURD_PREEMPT_SIGNAL_P (pe, signo, detail->code))
 	      {
 		if (pe->preemptor)
 		  {
 		    sighandler_t handler = (*pe->preemptor) (pe, ss,
 							     &signo, detail);
 		    if (handler != SIG_ERR)
 		      return handler;
 		  }
 		else
 		  return pe->handler;
 	      }
 	    pe = pe->next;
@@ -838,41 +843,41 @@ post_signal (struct hurd_sigstate *ss,
 
 	  if (_hurd_stopped && act != stop && (untraced || signo == SIGCONT))
 	    resume ();
 	}
     }
 
   if (_hurd_orphaned && act == stop &&
       (__sigmask (signo) & (__sigmask (SIGTTIN) | __sigmask (SIGTTOU) |
 			    __sigmask (SIGTSTP))))
     {
       /* If we would ordinarily stop for a job control signal, but we are
 	 orphaned so noone would ever notice and continue us again, we just
 	 quietly die, alone and in the dark.  */
       detail->code = signo;
       signo = SIGKILL;
       act = term;
     }
 
   /* Handle receipt of a blocked signal, or any signal while stopped.  */
   if (act != ignore &&		/* Signals ignored now are forgotten now.  */
-      __sigismember (&ss->blocked, signo) ||
+      __sigismember (&blocked, signo) ||
       (signo != SIGKILL && _hurd_stopped))
     {
       mark_pending ();
       act = ignore;
     }
 
   /* Perform the chosen action for the signal.  */
   switch (act)
     {
     case stop:
       if (_hurd_stopped)
 	{
 	  /* We are already stopped, but receiving an untraced stop
 	     signal.  Instead of resuming and suspending again, just
 	     notify the proc server of the new stop signal.  */
 	  error_t err = __USEPORT (PROC, __proc_mark_stop
 				   (port, signo, detail->code));
 	  assert_perror (err);
 	}
       else
diff --git a/sysdeps/mach/hurd/sigwait.c b/sysdeps/mach/hurd/sigwait.c
index 3f9ebc1..af50f74 100644
--- a/sysdeps/mach/hurd/sigwait.c
+++ b/sysdeps/mach/hurd/sigwait.c
@@ -11,130 +11,128 @@
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
    Lesser General Public License for more details.
 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, write to the Free
    Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
    02111-1307 USA.  */
 
 #include <errno.h>
 #include <hurd.h>
 #include <hurd/signal.h>
 #include <hurd/msg.h>
 #include <hurd/sigpreempt.h>
 #include <assert.h>
 
 /* Select any of pending signals from SET or wait for any to arrive.  */
 int
 __sigwait (const sigset_t *set, int *sig)
 {
   struct hurd_sigstate *ss;
-  sigset_t mask, ready;
+  sigset_t mask, ready, blocked;
   int signo = 0;
   struct hurd_signal_preemptor preemptor;
   jmp_buf buf;
   mach_port_t wait;
   mach_msg_header_t msg;
 
   sighandler_t
     preempt_fun (struct hurd_signal_preemptor *pe,
 		 struct hurd_sigstate *ss,
 		 int *sigp,
 		 struct hurd_signal_detail *detail)
     {
       if (signo)
 	/* We've already been run; don't interfere. */
 	return SIG_ERR;
 
       signo = *sigp;
 
       /* Make sure this is all kosher */
       assert (__sigismember (&mask, signo));
 
+      /* Restore the blocking mask. */
+      ss->blocked = blocked;
+
       return pe->handler;
     }
 
   void
     handler (int sig)
     {
       assert (sig == signo);
       longjmp (buf, 1);
     }
 
   wait = __mach_reply_port ();
 
   if (set != NULL)
     /* Crash before locking */
     mask = *set;
   else
     __sigemptyset (&mask);
 
   ss = _hurd_self_sigstate ();
   _hurd_sigstate_lock (ss);
 
   /* See if one of these signals is currently pending.  */
   sigset_t pending = _hurd_sigstate_pending (ss);
   __sigandset (&ready, &pending, &mask);
   if (! __sigisemptyset (&ready))
     {
       for (signo = 1; signo < NSIG; signo++)
 	if (__sigismember (&ready, signo))
 	  {
 	    __sigdelset (&ready, signo);
 	    goto all_done;
 	  }
       /* Huh?  Where'd it go? */
       abort ();
     }
 
   /* Wait for one of them to show up.  */
 
-  sigset_t blocked = 0;
-
   if (!setjmp (buf))
     {
       /* Make the preemptor */
       preemptor.signals = mask;
       preemptor.first = 0;
       preemptor.last = -1;
       preemptor.preemptor = preempt_fun;
       preemptor.handler = handler;
 
       /* Install this preemptor */
       preemptor.next = ss->preemptors;
       ss->preemptors = &preemptor;
 
       /* Unblock the expected signals */
       blocked = ss->blocked;
       ss->blocked &= ~mask;
 
       _hurd_sigstate_unlock (ss);
 
       /* Wait. */
       __mach_msg (&msg, MACH_RCV_MSG, 0, sizeof (msg), wait,
 		  MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
       abort ();
     }
   else
     {
       assert (signo);
 
       _hurd_sigstate_lock (ss);
 
       /* Delete our preemptor. */
       assert (ss->preemptors == &preemptor);
       ss->preemptors = preemptor.next;
-
-      /* Restore the blocking mask. */
-      ss->blocked = blocked;
     }
 
 
 all_done:
   _hurd_sigstate_unlock (ss);
 
   __mach_port_destroy (__mach_task_self (), wait);
   *sig = signo;
   return 0;
 }
 
 weak_alias (__sigwait, sigwait)
commit 055b96e76b6f7b8937828b89361f609a55a3df0f
Author: Jeremie Koenig <jk@jk.fr.eu.org>
Date:   Wed Jul 20 00:55:34 2011 +0000

    fix sigwait for pending signals

diff --git a/sysdeps/mach/hurd/sigwait.c b/sysdeps/mach/hurd/sigwait.c
index af50f74..61316bc 100644
--- a/sysdeps/mach/hurd/sigwait.c
+++ b/sysdeps/mach/hurd/sigwait.c
@@ -11,41 +11,41 @@
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
    Lesser General Public License for more details.
 
    You should have received a copy of the GNU Lesser General Public
    License along with the GNU C Library; if not, write to the Free
    Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
    02111-1307 USA.  */
 
 #include <errno.h>
 #include <hurd.h>
 #include <hurd/signal.h>
 #include <hurd/msg.h>
 #include <hurd/sigpreempt.h>
 #include <assert.h>
 
 /* Select any of pending signals from SET or wait for any to arrive.  */
 int
 __sigwait (const sigset_t *set, int *sig)
 {
   struct hurd_sigstate *ss;
-  sigset_t mask, ready, blocked;
+  sigset_t mask, blocked;
   int signo = 0;
   struct hurd_signal_preemptor preemptor;
   jmp_buf buf;
   mach_port_t wait;
   mach_msg_header_t msg;
 
   sighandler_t
     preempt_fun (struct hurd_signal_preemptor *pe,
 		 struct hurd_sigstate *ss,
 		 int *sigp,
 		 struct hurd_signal_detail *detail)
     {
       if (signo)
 	/* We've already been run; don't interfere. */
 	return SIG_ERR;
 
       signo = *sigp;
 
       /* Make sure this is all kosher */
       assert (__sigismember (&mask, signo));
@@ -57,82 +57,74 @@ __sigwait (const sigset_t *set, int *sig)
     }
 
   void
     handler (int sig)
     {
       assert (sig == signo);
       longjmp (buf, 1);
     }
 
   wait = __mach_reply_port ();
 
   if (set != NULL)
     /* Crash before locking */
     mask = *set;
   else
     __sigemptyset (&mask);
 
   ss = _hurd_self_sigstate ();
   _hurd_sigstate_lock (ss);
 
-  /* See if one of these signals is currently pending.  */
-  sigset_t pending = _hurd_sigstate_pending (ss);
-  __sigandset (&ready, &pending, &mask);
-  if (! __sigisemptyset (&ready))
-    {
-      for (signo = 1; signo < NSIG; signo++)
-	if (__sigismember (&ready, signo))
-	  {
-	    __sigdelset (&ready, signo);
-	    goto all_done;
-	  }
-      /* Huh?  Where'd it go? */
-      abort ();
-    }
-
-  /* Wait for one of them to show up.  */
+  /* Wait for one of the signals in MASK to show up.  */
 
   if (!setjmp (buf))
     {
       /* Make the preemptor */
       preemptor.signals = mask;
       preemptor.first = 0;
       preemptor.last = -1;
       preemptor.preemptor = preempt_fun;
       preemptor.handler = handler;
 
       /* Install this preemptor */
       preemptor.next = ss->preemptors;
       ss->preemptors = &preemptor;
 
       /* Unblock the expected signals */
       blocked = ss->blocked;
       ss->blocked &= ~mask;
 
+      /* See if one of them is currently pending.  */
+      sigset_t ready = _hurd_sigstate_pending (ss);
+      __sigandset (&ready, &ready, &mask);
+
       _hurd_sigstate_unlock (ss);
 
+      if (! __sigisemptyset (&ready))
+	/* Send a message to the signal thread so it
+	   will wake up and check for pending signals.  */
+	__msg_sig_post (_hurd_msgport, 0, 0, __mach_task_self ());
+
       /* Wait. */
       __mach_msg (&msg, MACH_RCV_MSG, 0, sizeof (msg), wait,
 		  MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
       abort ();
     }
   else
     {
       assert (signo);
 
       _hurd_sigstate_lock (ss);
 
       /* Delete our preemptor. */
       assert (ss->preemptors == &preemptor);
       ss->preemptors = preemptor.next;
     }
 
-
-all_done:
   _hurd_sigstate_unlock (ss);
 
   __mach_port_destroy (__mach_task_self (), wait);
   *sig = signo;
   return 0;
 }
 
 weak_alias (__sigwait, sigwait)

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]