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 25954 invoked from network); 10 Feb 2023 16:30:14 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 10 Feb 2023 16:30:14 -0000 Received: (qmail 8076 invoked by uid 550); 10 Feb 2023 16:30: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 8037 invoked from network); 10 Feb 2023 16:30:10 -0000 Date: Fri, 10 Feb 2023 11:29:58 -0500 From: Rich Felker To: Alexey Izbyshev Cc: musl@lists.openwall.com Message-ID: <20230210162957.GB4163@brightrain.aerifal.cx> References: <20221109104613.48062-1-izbyshev@ispras.ru> <20221214022618.GB15716@brightrain.aerifal.cx> <1a0289c15879bef6d538c0066f58545c@ispras.ru> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="98e8jtXdkpgskNou" Content-Disposition: inline In-Reply-To: <1a0289c15879bef6d538c0066f58545c@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH] mq_notify: fix close/recv race on failure path --98e8jtXdkpgskNou Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Dec 14, 2022 at 09:49:26AM +0300, Alexey Izbyshev wrote: > On 2022-12-14 05:26, Rich Felker wrote: > >On Wed, Nov 09, 2022 at 01:46:13PM +0300, Alexey Izbyshev wrote: > >>In case of failure mq_notify closes the socket immediately after > >>sending a cancellation request to the worker thread that is going to > >>call or have already called recv on that socket. Even if we don't > >>consider the kernel behavior when the only descriptor to an > >>object that > >>is being used in a system call is closed, if the socket descriptor is > >>closed before the kernel looks at it, another thread could open a > >>descriptor with the same value in the meantime, resulting in recv > >>acting on a wrong object. > >> > >>Fix the race by moving pthread_cancel call before the barrier wait to > >>guarantee that the cancellation flag is set before the worker thread > >>enters recv. > >>--- > >>Other ways to fix this: > >> > >>* Remove the racing close call from mq_notify and surround recv > >> with pthread_cleanup_push/pop. > >> > >>* Make the worker thread joinable initially, join it before closing > >> the socket on the failure path, and detach it on the happy path. > >> This would also require disabling cancellation around join/detach > >> to ensure that mq_notify itself is not cancelled in an inappropriate > >> state. > > > >I'd put this aside for a while because of the pthread barrier > >involvement I kinda didn't want to deal with. The fix you have sounds > >like it works, but I think I'd rather pursue one of the other > >approaches, probably the joinable thread one. > > > >At present, the implementation of barriers seems to be buggy (I need > >to dig back up the post about that), and they're also a really > >expensive synchronization tool that goes both directions where we > >really only need one direction (notifying the caller we're done > >consuming the args). I'd rather switch to a semaphore, which is the > >lightest and most idiomatic (at least per present-day musl idioms) way > >to do this. > > > This sounds good to me. The same approach can also be used in > timer_create (assuming it's acceptable to add dependency on > pthread_cancel to that code). > > >Using a joinable thread also lets us ensure we don't leave around > >threads that are waiting to be scheduled just to exit on failure > >return. Depending on scheduling attributes, this probably could be > >bad. > > > I also prefer this approach, though mostly for aesthetic reasons (I > haven't thought about the scheduling behavior). I didn't use it only > because I felt it's a "logically larger" change than simply moving > the pthread_barrier_wait call. And I wasn't aware that barriers are > buggy in musl. Finally following up on this. How do the attached commits look? Rich --98e8jtXdkpgskNou Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0001-mq_notify-use-semaphore-instead-of-barrier-to-sync-a.patch" >From 047594edd9c72c7b9b7e4f2b32d561a4b6555c2f Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 10 Feb 2023 11:17:02 -0500 Subject: [PATCH 1/3] 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 | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index 221591c7..7d3dff5f 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) @@ -48,7 +49,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 +57,8 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) return -1; } - pthread_barrier_wait(&args.barrier); - pthread_barrier_destroy(&args.barrier); + sem_wait(&args.sem); + sem_destroy(&args.sem); sev2.sigev_notify = SIGEV_THREAD; sev2.sigev_signo = s; -- 2.21.0 --98e8jtXdkpgskNou Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0002-mq_notify-fix-use-after-close-double-close-bug-in-er.patch" >From 70dea58630dedb26b2fbf03422ad7a36f8e95c92 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 10 Feb 2023 11:22:45 -0500 Subject: [PATCH 2/3] mq_notify: fix use-after-close/double-close bug in error path in the error path where the mq_notify syscall fails, the initiating thread may close the socket before the worker thread calls recv on it. even if this does not happen, both threads perform the close operation, resulting in double-close. move the close to a cancellation cleanup handler so this cannot happen. this fix is based on one of the alternate proposals in Alexey Izbyshev's original report of the bug. --- src/mq/mq_notify.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index 7d3dff5f..a42e6afc 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -13,6 +13,11 @@ struct args { const struct sigevent *sev; }; +static void cleanup(void *p) +{ + __syscall(SYS_close, *(int *)p); +} + static void *start(void *p) { struct args *args = p; @@ -23,8 +28,9 @@ static void *start(void *p) union sigval val = args->sev->sigev_value; sem_post(&args->sem); + pthread_cleanup_push(cleanup, &s); n = recv(s, buf, sizeof(buf), MSG_NOSIGNAL|MSG_WAITALL); - close(s); + pthread_cleanup_pop(1); if (n==sizeof buf && buf[sizeof buf - 1] == 1) func(val); return 0; @@ -66,7 +72,6 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) if (syscall(SYS_mq_notify, mqd, &sev2) < 0) { pthread_cancel(td); - __syscall(SYS_close, s); return -1; } -- 2.21.0 --98e8jtXdkpgskNou Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0003-mq_notify-join-worker-thread-before-returning-in-err.patch" >From 1e9313fec1c666be2d2a3013aa0f34ed0e166c45 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 10 Feb 2023 11:27:18 -0500 Subject: [PATCH 3/3] 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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mq/mq_notify.c b/src/mq/mq_notify.c index a42e6afc..31538880 100644 --- a/src/mq/mq_notify.c +++ b/src/mq/mq_notify.c @@ -54,7 +54,6 @@ 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); sem_init(&args.sem, 0, 0); if (pthread_create(&td, &attr, start, &args)) { @@ -72,8 +71,10 @@ int mq_notify(mqd_t mqd, const struct sigevent *sev) if (syscall(SYS_mq_notify, mqd, &sev2) < 0) { pthread_cancel(td); + pthread_join(td, 0); return -1; } + pthread_detach(td); return 0; } -- 2.21.0 --98e8jtXdkpgskNou--