From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 15065 invoked from network); 12 Feb 2023 00:32:21 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 12 Feb 2023 00:32:21 -0000 Received: (qmail 25863 invoked by uid 550); 12 Feb 2023 00:32:15 -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 25828 invoked from network); 12 Feb 2023 00:32:14 -0000 Date: Sat, 11 Feb 2023 19:32:01 -0500 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20230212003158.GO4163@brightrain.aerifal.cx> References: <63c0897d647936c946268f5a967a5e4d@ispras.ru> <20230211150603.GI4163@brightrain.aerifal.cx> <20230211171338.GD1903@voyager> <2da3840a9345c0a810e9d93ab4f6bca7@ispras.ru> <20230211175948.GK4163@brightrain.aerifal.cx> <20230211183505.GL4163@brightrain.aerifal.cx> <20230211194950.GN4163@brightrain.aerifal.cx> <4408deeb62fe668bf720d3c6c8bedda2@ispras.ru> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="jKBxcB1XkHIR0Eqt" Content-Disposition: inline In-Reply-To: <4408deeb62fe668bf720d3c6c8bedda2@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] mq_notify: fix close/recv race on failure path --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Feb 11, 2023 at 11:14:33PM +0300, Alexey Izbyshev wrote: > On 2023-02-11 22:49, Rich Felker wrote: > >On Sat, Feb 11, 2023 at 10:28:20PM +0300, Alexey Izbyshev wrote: > >>On 2023-02-11 21:35, Rich Felker wrote: > >>>On Sat, Feb 11, 2023 at 09:08:53PM +0300, Alexey Izbyshev wrote: > >>>>On 2023-02-11 20:59, Rich Felker wrote: > >>>>>On Sat, Feb 11, 2023 at 08:50:15PM +0300, Alexey Izbyshev wrote: > >>>>>>On 2023-02-11 20:13, Markus Wichmann wrote: > >>>>>>>On Sat, Feb 11, 2023 at 10:06:03AM -0500, Rich Felker wrote: > >>>>>>>>--- a/src/thread/pthread_detach.c > >>>>>>>>+++ b/src/thread/pthread_detach.c > >>>>>>>>@@ -5,8 +5,12 @@ static int __pthread_detach(pthread_t t) > >>>>>>>> { > >>>>>>>> /* If the cas fails, detach state is either already-detached > >>>>>>>> * or exiting/exited, and pthread_join will trap or cleanup. */ > >>>>>>>>- if (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED) != > >>>>>>>>DT_JOINABLE) > >>>>>>>>+ if (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED) != > >>>>>>>>DT_JOINABLE) { > >>>>>>>>+ int cs; > >>>>>>>>+ __pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); > >>>>>>>> return __pthread_join(t, 0); > >>>>>>> ^^^^^^ I think you forgot to rework this. > >>>>>>>>+ __pthread_setcancelstate(cs, 0); > >>>>>>>>+ } > >>>>>>>> return 0; > >>>>>>>> } > >>>>>>>> > >>>>>>> > >>>>>>>I see no other obvious missteps, though. > >>>>>>> > >>>>>>Same here, apart from this and misspelled "pthred_detach" in the > >>>>>>commit message, the patches look good to me. > >>>>>> > >>>>>>Regarding the POSIX requirement to run sigev_notify_function in the > >>>>>>context of a detached thread, while it's possible to observe the > >>>>>>wrong detachstate for a short while via pthread_getattr_np after > >>>>>>these patches, I'm not sure there is a standard way to do that. Even > >>>>>>if it exists, this minor issue may be not worth caring about. > >>>>> > >>>>>Would this just be if the notification callback executes before > >>>>>mq_notify returns in the parent? > >>>> > >>>>Yes, it seems so. > >>>> > >>>>>I suppose we could have the newly > >>>>>created thread do the work of making the syscall, handling the error > >>>>>case, detaching itself on success and and reporting back to the > >>>>>mq_notify function whether it succeeded or failed via the > >>>>>semaphore/args structure. Thoughts on that? > >>>>> > >>>>Could we just move pthread_detach call to the worker thread to the > >>>>point after pthread_cleanup_pop? > >>> > >>>I thought that sounded dubious, in that it might lead to an attempt to > >>>join a detached thread, but maybe it's safe to assume recv will never > >>>return if the mq_notify syscall failed...? > >>> > >>Actually, because app signals are not blocked when the worker thread > >>is created, recv can indeed return early with EINTR. But this looks > >>like just a bug. > > > >Yes. While it's not a conformance bug to run with signals unblocked > >("The signal mask of this thread is implementation-defined.") it's a > >functional bug to ever introduce threads that don't block all > >application signals, since these interfere with sigwait & other > >application control of where signals are delivered. This is an > >oversight. I'll make it mask all signals. > > > >>Otherwise, mq_notify already assumes that recv can't return before > >>SYS_mq_notify (if it did, the syscall would try to register a closed > >>fd). I haven't tried to prove it (e.g. maybe recv may need to > >>allocate something before blocking and hence can fail with ENOMEM?), > >>but if it's true, I don't see how a failed SYS_mq_notify could cause > >>recv to return, so joining a detached thread should be impossible if > >>we make pthread_detach follow recv. > > > >I'm thinking for now maybe we should just drop the joining on error, > >and leave it starting out detached. While recv should not fail, it's > >obviously possible to make it fail in a seccomp sandbox, and you don't > >want that to turn into UB inside the implementation. If it does fail, > >the thread should still exit, but we have no way to synchronize with > >the mq_notify parent to decide whether it's being joined or not in > >this case without extra sync machinery... > > > By dropping pthread_join we'd avoid introducing a new UB case if > recv fails unexpectedly, but the existing case that I mentioned > (SYS_mq_notify trying to register a closed fd) would remain. It > seems to me that moving SYS_mq_notify into the worker thread as you > suggested earlier is the cleanest option if we're worrying about > recv. OK, I've done and reworked this series in a way that I think addresses all the problems. Rich --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-fix-pthread_detach-inadvertently-acting-as-cancellat.patch" >From c3cd04fa5fecd2c349aefde090c602554ee4fa24 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Sat, 11 Feb 2023 09:54:12 -0500 Subject: [PATCH 1/5] fix pthread_detach inadvertently acting as cancellation point in race case disabling cancellation around the pthread_join call seems to be the safest and logically simplest fix. i believe it would also be possible to just perform the unmap directly here after __tl_sync, removing the dependency on pthread_join, but such an approach duplicately encodes a lot more implementation assumptions. --- src/thread/pthread_detach.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/thread/pthread_detach.c b/src/thread/pthread_detach.c index 77772af2..d73a500e 100644 --- a/src/thread/pthread_detach.c +++ b/src/thread/pthread_detach.c @@ -5,8 +5,12 @@ static int __pthread_detach(pthread_t t) { /* If the cas fails, detach state is either already-detached * or exiting/exited, and pthread_join will trap or cleanup. */ - if (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED) != DT_JOINABLE) - return __pthread_join(t, 0); + if (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED) != DT_JOINABLE) { + int cs; + __pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); + __pthread_join(t, 0); + __pthread_setcancelstate(cs, 0); + } return 0; } -- 2.21.0 --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0002-mq_notify-use-semaphore-instead-of-barrier-to-sync-a.patch" >From fde6891e59c315e4a0ec7e69182e1d6314e3795e Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 10 Feb 2023 11:17:02 -0500 Subject: [PATCH 2/5] mq_notify: use semaphore instead of barrier to sync args consumption semaphores are a much lighter primitive, and more idiomatic with current usage in the code base. --- src/mq/mq_notify.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index 221591c7..a42888d2 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -4,10 +4,11 @@ #include #include #include +#include #include "syscall.h" struct args { - pthread_barrier_t barrier; + sem_t sem; int sock; const struct sigevent *sev; }; @@ -21,7 +22,7 @@ static void *start(void *p) void (*func)(union sigval) = args->sev->sigev_notify_function; union sigval val = args->sev->sigev_value; - pthread_barrier_wait(&args->barrier); + sem_post(&args->sem); n = recv(s, buf, sizeof(buf), MSG_NOSIGNAL|MSG_WAITALL); close(s); if (n==sizeof buf && buf[sizeof buf - 1] == 1) @@ -37,6 +38,7 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) int s; struct sigevent sev2; static const char zeros[32]; + int cs; if (!sev || sev->sigev_notify != SIGEV_THREAD) return syscall(SYS_mq_notify, mqd, sev); @@ -48,7 +50,7 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) if (sev->sigev_notify_attributes) attr = *sev->sigev_notify_attributes; else pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); - pthread_barrier_init(&args.barrier, 0, 2); + sem_init(&args.sem, 0, 0); if (pthread_create(&td, &attr, start, &args)) { __syscall(SYS_close, s); @@ -56,8 +58,10 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) return -1; } - pthread_barrier_wait(&args.barrier); - pthread_barrier_destroy(&args.barrier); + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); + sem_wait(&args.sem); + pthread_setcancelstate(cs, 0); + sem_destroy(&args.sem); sev2.sigev_notify = SIGEV_THREAD; sev2.sigev_signo = s; -- 2.21.0 --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0003-mq_notify-rework-to-fix-use-after-close-double-close.patch" >From 1c75ded5ad35aaf3f4e1da316c50869c73903420 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 10 Feb 2023 11:22:45 -0500 Subject: [PATCH 3/5] mq_notify: rework to fix use-after-close/double-close bugs in the error path where the mq_notify syscall fails, the initiating thread may have closed the socket before the worker thread calls recv on it. even in the absence of such a race, if the recv call failed, e.g. due to seccomp policy blocking it, the worker thread could proceed to close, producing a double-close condition. this can all be simplified by moving the mq_notify syscall into the new thread, so that the error case does not require pthread_cancel. now, the initiating thread only needs to read back the error status after waiting for the worker thread to consume its arguments. --- src/mq/mq_notify.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index a42888d2..8eac71ed 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -10,6 +10,8 @@ struct args { sem_t sem; int sock; + mqd_t mqd; + int err; const struct sigevent *sev; }; @@ -21,8 +23,21 @@ static void *start(void *p) int s = args->sock; void (*func)(union sigval) = args->sev->sigev_notify_function; union sigval val = args->sev->sigev_value; + struct sigevent sev2; + static const char zeros[32]; + int err = 0; + sev2.sigev_notify = SIGEV_THREAD; + sev2.sigev_signo = s; + sev2.sigev_value.sival_ptr = (void *)&zeros; + + err = 0; + if (syscall(SYS_mq_notify, args->mqd, &sev2) < 0) + err = errno; + args->err = err; sem_post(&args->sem); + if (err) return 0; + n = recv(s, buf, sizeof(buf), MSG_NOSIGNAL|MSG_WAITALL); close(s); if (n==sizeof buf && buf[sizeof buf - 1] == 1) @@ -36,8 +51,6 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) pthread_attr_t attr; pthread_t td; int s; - struct sigevent sev2; - static const char zeros[32]; int cs; if (!sev || sev->sigev_notify != SIGEV_THREAD) @@ -46,6 +59,7 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) s = socket(AF_NETLINK, SOCK_RAW|SOCK_CLOEXEC, 0); if (s < 0) return -1; args.sock = s; + args.mqd = mqd; if (sev->sigev_notify_attributes) attr = *sev->sigev_notify_attributes; else pthread_attr_init(&attr); @@ -63,13 +77,9 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) pthread_setcancelstate(cs, 0); sem_destroy(&args.sem); - sev2.sigev_notify = SIGEV_THREAD; - sev2.sigev_signo = s; - sev2.sigev_value.sival_ptr = (void *)&zeros; - - if (syscall(SYS_mq_notify, mqd, &sev2) < 0) { - pthread_cancel(td); + if (args.err) { __syscall(SYS_close, s); + errno = args.err; return -1; } -- 2.21.0 --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0004-mq_notify-join-worker-thread-before-returning-in-err.patch" >From b6130c3da61e3d1bc61be88b0d3c8d1687089dfa Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Sat, 11 Feb 2023 19:09:53 -0500 Subject: [PATCH 4/5] mq_notify: join worker thread before returning in error path this avoids leaving behind transient resource consumption whose cleanup is subject to scheduling behavior. --- src/mq/mq_notify.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index 8eac71ed..0ffee7d1 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -38,6 +38,7 @@ static void *start(void *p) sem_post(&args->sem); if (err) return 0; + pthread_detach(pthread_self()); n = recv(s, buf, sizeof(buf), MSG_NOSIGNAL|MSG_WAITALL); close(s); if (n==sizeof buf && buf[sizeof buf - 1] == 1) @@ -63,7 +64,7 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) if (sev->sigev_notify_attributes) attr = *sev->sigev_notify_attributes; else pthread_attr_init(&attr); - pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); + pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); sem_init(&args.sem, 0, 0); if (pthread_create(&td, &attr, start, &args)) { @@ -74,14 +75,16 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); sem_wait(&args.sem); - pthread_setcancelstate(cs, 0); sem_destroy(&args.sem); if (args.err) { __syscall(SYS_close, s); + pthread_join(td, 0); + pthread_setcancelstate(cs, 0); errno = args.err; return -1; } + pthread_setcancelstate(cs, 0); return 0; } -- 2.21.0 --jKBxcB1XkHIR0Eqt Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0005-mq_notify-block-all-application-signals-in-the-worke.patch" >From 0968c0c29252eb5fbca147fca901a53fb1b8ae04 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Sat, 11 Feb 2023 19:13:10 -0500 Subject: [PATCH 5/5] mq_notify: block all (application) signals in the worker thread until the mq notification event arrives, it is mandatory that signals be blocked. otherwise, a signal can be received, and its handler executed, in a thread which does not yet exist on the abstract machine. after the point of the event arriving, having signals blocked is not a conformance requirement but a QoI requirement. while the application can unblock any signals it wants unblocked in the event handler thread, if they did not start out blocked, it could not block them without a race window where they are momentarily unblocked, and this would preclude controlled delivery or other forms of acceptance (sigwait, etc.) anywhere in the application. --- src/mq/mq_notify.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index 0ffee7d1..01ec3517 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -53,6 +53,7 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) pthread_t td; int s; int cs; + sigset_t allmask, origmask; if (!sev || sev->sigev_notify != SIGEV_THREAD) return syscall(SYS_mq_notify, mqd, sev); @@ -67,11 +68,15 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); sem_init(&args.sem, 0, 0); + sigfillset(&allmask); + pthread_sigmask(SIG_BLOCK, &allmask, &origmask); if (pthread_create(&td, &attr, start, &args)) { __syscall(SYS_close, s); + pthread_sigmask(SIG_SETMASK, &origmask, 0); errno = EAGAIN; return -1; } + pthread_sigmask(SIG_SETMASK, &origmask, 0); pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); sem_wait(&args.sem); -- 2.21.0 --jKBxcB1XkHIR0Eqt--