Re: [musl] [PATCH] add support for pthread_sigqueue

[Daniel Fahlgren <daniel@fahlgren.se>]

On Wed, 2020-02-05 at 14:27 -0500, Rich Felker wrote:
> On Wed, Feb 05, 2020 at 03:12:55PM +0100, Daniel Fahlgren wrote:
> > Hi,
> > 
> > This patch adds the function pthread_sigqueue. Since this is my
> > first
> > patch to musl I probably have missed something. Any feedback?
> > 
> > Best regards,
> > Daniel Fahlgren
> > From 2e3e423b4d3d62fec3525c2e09fc9daf35fbe885 Mon Sep 17 00:00:00
> > 2001
> > From: Daniel Fahlgren <daniel@fahlgren.se>
> > Date: Wed, 5 Feb 2020 13:24:43 +0100
> > Subject: [PATCH] Add pthread_sigqueue
> > 
> > This makes it possible to queue a signal and data to a thread
> > ---
> >  include/signal.h              |  1 +
> >  src/thread/pthread_sigqueue.c | 24 ++++++++++++++++++++++++
> >  2 files changed, 25 insertions(+)
> >  create mode 100644 src/thread/pthread_sigqueue.c
> > 
> > diff --git a/include/signal.h b/include/signal.h
> > index fbdf667b..8bb7d1b4 100644
> > --- a/include/signal.h
> > +++ b/include/signal.h
> > @@ -214,6 +214,7 @@ int sigqueue(pid_t, int, union sigval);
> >  
> >  int pthread_sigmask(int, const sigset_t *__restrict, sigset_t
> > *__restrict);
> >  int pthread_kill(pthread_t, int);
> > +int pthread_sigqueue(pthread_t, int, union sigval);
> >  
> >  void psiginfo(const siginfo_t *, const char *);
> >  void psignal(int, const char *);
> > diff --git a/src/thread/pthread_sigqueue.c
> > b/src/thread/pthread_sigqueue.c
> > new file mode 100644
> > index 00000000..8de8d49a
> > --- /dev/null
> > +++ b/src/thread/pthread_sigqueue.c
> > @@ -0,0 +1,23 @@
> > +#include <signal.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include "pthread_impl.h"
> > +#include "lock.h"
> > +
> > +int pthread_sigqueue(pthread_t t, int sig, const union sigval
> > value)
> > +{
> > +	siginfo_t si;
> > +	sigset_t set;
> > +	int r;
> > +	memset(&si, 0, sizeof si);
> > +	si.si_signo = sig;
> > +	si.si_code = SI_QUEUE;
> > +	si.si_value = value;
> > +	si.si_uid = getuid();
> > +	si.si_pid = getpid();
> > +	LOCK(t->killlock);
> > +	r = t->tid ? -__syscall(SYS_rt_tgsigqueueinfo, si.si_pid, t-
> > >tid, sig, &si)
> > +		: (sig+0U >= _NSIG ? EINVAL : 0);
> > +	UNLOCK(t->killlock);
> > +	return r;
> > +}
> > -- 
> > 2.17.1
> > 
> 
> Is the naming pthread_sigqueue (vs pthread_sigqueue_np) intentional?
> Normally pthread functions that are not part of the standard are
> introduced as _np (non-portable) and promoted later if accepted as
> standards since the pthread_* namespace is reserved for future
> directions of the standard. Does glibc have this function with the
> same signature already?

Yes, I noticed this function was missing when trying to switch some of
the code base at work from uClibc-ng to musl. According to the
changelog this function has been part of glibc since 2009.

> I also notice that there's a fundamental race between getting the
> uid,
> pid, and tid and sending the signal that would normally need to be
> mitigated by blocking signals for the duration of the operation.
> However, I don't think we need to care about a fork interrupting it;
> if that happens, the t argument is invalid and thus the call has
> undefined behavior. This leaves concurrent change of uid as a
> consideration. The normal sigqueue is not doing anything special
> about
> that, but perhaps it should; I haven't thought enough about it yet to
> have an opinion, so I'm just raising the issue for consideration at
> this point.
> 
> With that said, this mostly looks good.
> 
> Rich

I'm not sure it even is possible to do anything special. The process
would have to block from getuid() until the signal is processed. If a
different thread is changing the uid it doesn't really matter if that
is done between getting the uid and doing the syscall, or just after
the syscall has returned. As long as the change is done before the
receiving thread has processed the signal the effect should be the
same, or?

Daniel