[musl] [PATCH] add support for pthread_sigqueue

[musl] [PATCH] add support for pthread_sigqueue

[Daniel Fahlgren]

[-- Attachment #1: Type: text/plain, Size: 168 bytes --]

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

[-- Attachment #2: 0001-Add-pthread_sigqueue.patch --]
[-- Type: text/x-patch, Size: 1625 bytes --]

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

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

[Rich Felker]

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?

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

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

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 1058 bytes --]

On 05/02/2020 13:27, Rich Felker wrote:
> 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?


The signature differs only with the constness of the third argument in
the header.  (The constness in the function implementation is correct.)

It is implemented not only in glibc, but additionally in Solaris 11:

https://docs.oracle.com/cd/E88353_01/html/E37843/pthread-sigqueue-3c.html

The corresponding system call was added in Linux 2.6.31, if it matters.

I have no comments (positive or negative) regarding the addition of this
call nor its implementation other than that the constness should be
fixed in the header.

Best,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Fahlgren]

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

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

[Rich Felker]

On Wed, Feb 05, 2020 at 05:06:11PM -0600, A. Wilcox wrote:
> On 05/02/2020 13:27, Rich Felker wrote:
> > 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?
> 
> The signature differs only with the constness of the third argument in
> the header.  (The constness in the function implementation is correct.)

This is not a difference in the signature; the constness in the
declaration is meaningless (explicitly ignored by the language). In
the definition, it causes the local variable corresponding to the
argument to be const, but that's not terribly useful.

> It is implemented not only in glibc, but additionally in Solaris 11:
> 
> https://docs.oracle.com/cd/E88353_01/html/E37843/pthread-sigqueue-3c.html

Without _np -- interesting. I'm not opposed if this seems to be the
consensus.

> The corresponding system call was added in Linux 2.6.31, if it matters.
> 
> I have no comments (positive or negative) regarding the addition of this
> call nor its implementation other than that the constness should be
> fixed in the header.

That change isn't needed or wanted; it's just confusing since it looks
like it should have some meaning but doesn't. I would remove it from
the definition too; we don't normally make arguments gratuitously
const even if the function definition doesn't modify them during their
lifetime.

Rich

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

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 1836 bytes --]

On 05/02/2020 18:17, Rich Felker wrote:
> On Wed, Feb 05, 2020 at 05:06:11PM -0600, A. Wilcox wrote:
> This is not a difference in the signature; the constness in the
> declaration is meaningless (explicitly ignored by the language). In
> the definition, it causes the local variable corresponding to the
> argument to be const, but that's not terribly useful.


Sorry, I was confused and thought it was a pointer type.  Disregard.


>> It is implemented not only in glibc, but additionally in Solaris 11:
>>
>> https://docs.oracle.com/cd/E88353_01/html/E37843/pthread-sigqueue-3c.html
> 
> Without _np -- interesting. I'm not opposed if this seems to be the
> consensus.


Notably, no one in the BSD family has implemented it.  I have not found
any discussion of this interface on the Austin Group defect tracker.  It
is a very recent addition to Solaris – appearing to have been added last
year.

WebKit's JSC mentions the desire to utilise this interface in a comment
but stops short of actual use due to portability concerns.  Firefox
appears to use this interface for a test case for its sandbox, on glibc
hosts only.  Intriguingly, Xenomai does define the interface, but as
pthread_sigqueue_np.

The only seemingly-legitimate use of this interface comes from a
perplexingly wrong usage in Intel's graphics driver toolset at:

https://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/lib/igt_core.c?id=497e13d2b4#n2465

...where they call pthread_sigqueue only if the system is /not/ Linux.
I would assume this would break the build on FreeBSD, but perhaps they
don't support these tools on non-Linux platforms.  If that's the case,
then I'm not sure why this code block even exists.

Best,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]