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