This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 11/15] Hurd signals: fix sigwait() for global signals
- From: Jeremie Koenig <jk at jk dot fr dot eu dot org>
- To: libc-alpha at sourceware dot org, bug-hurd at gnu dot org,Roland McGrath <roland at hack dot frob dot com>
- Date: Wed, 20 Jul 2011 03:51:23 +0200
- Subject: Re: [PATCH 11/15] Hurd signals: fix sigwait() for global signals
- References: <1309365027-4774-1-git-send-email-jk@jk.fr.eu.org><1309365027-4774-12-git-send-email-jk@jk.fr.eu.org><20110703003200.GM11935@const.famille.thibault.fr>
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)