Use pthread_sigmask instead of sigprocmask inside process_pending_signals
to ensure that race conditions aren't possible.
Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
---
linux-user/signal.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7eecec46c4..81ff753c01 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1005,9 +1005,8 @@ void process_pending_signals(CPUArchState *cpu_env)
sigset_t *blocked_set;
while (qatomic_read(&ts->signal_pending)) {
- /* FIXME: This is not threadsafe. */
sigfillset(&set);
- sigprocmask(SIG_SETMASK, &set, 0);
+ pthread_sigmask(SIG_SETMASK, &set, 0);
restart_scan:
sig = ts->sync_signal.pending;
--
2.31.1
On Mon, 24 May 2021 at 03:48, Hamza Mahfooz <someguy@effective-light.com> wrote: > > Use pthread_sigmask instead of sigprocmask inside process_pending_signals > to ensure that race conditions aren't possible. > > Signed-off-by: Hamza Mahfooz <someguy@effective-light.com> > --- > linux-user/signal.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index 7eecec46c4..81ff753c01 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -1005,9 +1005,8 @@ void process_pending_signals(CPUArchState *cpu_env) > sigset_t *blocked_set; > > while (qatomic_read(&ts->signal_pending)) { > - /* FIXME: This is not threadsafe. */ > sigfillset(&set); > - sigprocmask(SIG_SETMASK, &set, 0); > + pthread_sigmask(SIG_SETMASK, &set, 0); We use sigprocmask() in plenty more places than this one in linux-user, so it seems unlikely that the FIXME comment is simply noting that we've used sigprocmask() rather than pthread_sigmask(). Indeed, the comment dates back to before this function called sigprocmask() at all (the sigprocmask() call was added in commit 3d3efba020da which just preserves the FIXME comment. So I think we cannot remove this FIXME comment like this: we need to more carefully analyze the code/dig through the history to identify what race condition/threadsafety issue the comment is attempting to point out, because it's not "we didn't use pthread_sigmask()". (As it happens, on Linux/glibc sigprocmask() is implemented as simply calling pthread_sigmask(): https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/sigprocmask.c;h=9dfd8076d12aff9014fa40f7e93111760a1a8bad;hb=HEAD If we do want to change from sigprocmask() to pthread_sigmask(), we should be consistent about doing that, not just change this call only.) thanks -- PMM
On Thu, May 27 2021 at 11:16:56 AM +0100, Peter Maydell <peter.maydell@linaro.org> wrote: > If we do want to change from sigprocmask() to pthread_sigmask(), we > should be consistent about doing that, not just change this call > only.) On that note, do you think it would worthwhile to have a Coccinelle script replace all instances of sigprocmask with pthread_sigmask (in linux-user, of course)?
On Thu, 27 May 2021 at 11:37, Hamza Mahfooz <someguy@effective-light.com> wrote: > > > > On Thu, May 27 2021 at 11:16:56 AM +0100, Peter Maydell > <peter.maydell@linaro.org> wrote: > > If we do want to change from sigprocmask() to pthread_sigmask(), we > > should be consistent about doing that, not just change this call > > only.) > On that note, do you think it would worthwhile to have a Coccinelle > script replace all instances of sigprocmask with pthread_sigmask (in > linux-user, of course)? What issue are we trying to fix by making this change ? There are only 7 calls so a coccinelle script seems like overkill. thanks -- PMM
On Thu, May 27 2021 at 11:45:26 AM +0100, Peter Maydell <peter.maydell@linaro.org> wrote: > What issue are we trying to fix by making this change ? I suppose that it wouldn't fix any issues in the current state of affairs, maybe it is something to reconsider if glibc ever changes such that, sigprocmask is no longer MT-safe (which is within reason since the MT behavior of sigprocmask is undefined by POSIX).
© 2016 - 2024 Red Hat, Inc.