From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.0 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.2 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by inbox.vuxu.org (OpenSMTPD) with SMTP id a1d37687 for ; Wed, 5 Feb 2020 19:28:13 +0000 (UTC) Received: (qmail 25978 invoked by uid 550); 5 Feb 2020 19:28:11 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 25957 invoked from network); 5 Feb 2020 19:28:08 -0000 Date: Wed, 5 Feb 2020 14:27:56 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20200205192756.GC1663@brightrain.aerifal.cx> References: <294c4c13f86c6f9ea3593309458bcb75a1d5d9e8.camel@fahlgren.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <294c4c13f86c6f9ea3593309458bcb75a1d5d9e8.camel@fahlgren.se> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: Rich Felker Subject: Re: [musl] [PATCH] add support for pthread_sigqueue 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 > 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 > +#include > +#include > +#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