On Mon, 24 May 2021 at 03:48, Hamza Mahfooz <[email protected]> wrote: > > Use pthread_sigmask instead of sigprocmask inside process_pending_signals > to ensure that race conditions aren't possible. > > Signed-off-by: Hamza Mahfooz <[email protected]> > --- > 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
