Re: [musl] [PATCH] mq_notify: fix close/recv race on failure path

[Rich Felker <dalias@libc.org>]

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

On Sat, Feb 11, 2023 at 05:45:14PM +0300, Alexey Izbyshev wrote:
> On 2023-02-10 19:29, Rich Felker wrote:
> >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?
> >
> The first and third patches add calls to sem_wait, pthread_join, and
> pthread_detach, which are cancellation points in musl, so
> cancellation needs to be disabled across those calls. I mentioned
> that in my initial mail.
> 
> Also, I wasn't sure if it's fine to just remove
> pthread_attr_setdetachstate call, and I found the following in
> POSIX[1]:
> 
> "The function shall be executed in an environment as if it were the
> start_routine for a newly created thread with thread attributes
> specified by sigev_notify_attributes. If sigev_notify_attributes is
> NULL, the behavior shall be as if the thread were created with the
> detachstate attribute set to PTHREAD_CREATE_DETACHED. Supplying an
> attributes structure with a detachstate attribute of
> PTHREAD_CREATE_JOINABLE results in undefined behavior."
> 
> This language seems to forbid calling sigev_notify_function in the
> context of a joinable thread. And even if musl wants to ignore this,
> PTHREAD_CREATE_JOINABLE must still be set manually if
> sigev_notify_attributes is not NULL.
> 
> Otherwise, the patches look good to me.
> 
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_02

Updated patch series attached.

[-- Attachment #2: 0001-fix-pthred_detach-inadvertently-acting-as-cancellati.patch --]
[-- Type: text/plain, Size: 1331 bytes --]

From 382009435fd6bf61df8f7c94dd44ea0ddd42f749 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Sat, 11 Feb 2023 09:54:12 -0500
Subject: [PATCH 1/4] fix pthred_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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/thread/pthread_detach.c b/src/thread/pthread_detach.c
index 77772af2..ac9c1035 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)
+	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);
+		__pthread_setcancelstate(cs, 0);
+	}
 	return 0;
 }
 
-- 
2.21.0


[-- Attachment #3: 0002-mq_notify-use-semaphore-instead-of-barrier-to-sync-a.patch --]
[-- Type: text/plain, Size: 2162 bytes --]

From 348bf7160dded85792fa24f4e310d2595dae7f8c Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Fri, 10 Feb 2023 11:17:02 -0500
Subject: [PATCH 2/4] 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 <sys/socket.h>
 #include <signal.h>
 #include <unistd.h>
+#include <semaphore.h>
 #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


[-- Attachment #4: 0003-mq_notify-fix-use-after-close-double-close-bug-in-er.patch --]
[-- Type: text/plain, Size: 1596 bytes --]

From 560764d8d2ef5e531bb0fb99bcd092169abafbe0 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Fri, 10 Feb 2023 11:22:45 -0500
Subject: [PATCH 3/4] 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 a42888d2..ab718732 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;
@@ -69,7 +75,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


[-- Attachment #5: 0004-mq_notify-join-worker-thread-before-returning-in-err.patch --]
[-- Type: text/plain, Size: 1499 bytes --]

From e071fb9757ae71c3ec2c7b467e511171a906ae03 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Fri, 10 Feb 2023 11:27:18 -0500
Subject: [PATCH 4/4] 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 ab718732..7c6e3b5a 100644
--- a/src/mq/mq_notify.c
+++ b/src/mq/mq_notify.c
@@ -55,7 +55,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)) {
@@ -66,7 +66,6 @@ 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);
 
 	sev2.sigev_notify = SIGEV_THREAD;
@@ -75,8 +74,12 @@ 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);
+		pthread_setcancelstate(cs, 0);
 		return -1;
 	}
 
+	pthread_setcancelstate(cs, 0);
+	pthread_detach(td);
 	return 0;
 }
-- 
2.21.0