[PATCH] linux-user: make process_pending_signals thread-safe

[PATCH] linux-user: make process_pending_signals thread-safe

[Hamza Mahfooz]

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

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Peter Maydell]

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

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Hamza Mahfooz]

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)?

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Peter Maydell]

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

Re: [PATCH] linux-user: make process_pending_signals thread-safe

[Hamza Mahfooz]

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).

[PATCH] linux-user: make process_pending_signals thread-safe

[effective-light]

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