mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: Alexey Izbyshev <izbyshev@ispras.ru>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] mq_notify: fix close/recv race on failure path
Date: Fri, 10 Feb 2023 11:29:58 -0500	[thread overview]
Message-ID: <20230210162957.GB4163@brightrain.aerifal.cx> (raw)
In-Reply-To: <1a0289c15879bef6d538c0066f58545c@ispras.ru>

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

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

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

From 047594edd9c72c7b9b7e4f2b32d561a4b6555c2f Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
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 <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)
@@ -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


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

From 70dea58630dedb26b2fbf03422ad7a36f8e95c92 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
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


[-- Attachment #4: 0003-mq_notify-join-worker-thread-before-returning-in-err.patch --]
[-- Type: text/plain, Size: 1116 bytes --]

From 1e9313fec1c666be2d2a3013aa0f34ed0e166c45 Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
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


  reply	other threads:[~2023-02-10 16:30 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-09 10:46 Alexey Izbyshev
2022-12-14  2:26 ` Rich Felker
2022-12-14  6:49   ` Alexey Izbyshev
2023-02-10 16:29     ` Rich Felker [this message]
2023-02-11 14:45       ` Alexey Izbyshev
2023-02-11 14:52         ` Rich Felker
2023-02-11 15:13           ` Alexey Izbyshev
2023-02-11 15:06         ` Rich Felker
2023-02-11 17:13           ` Markus Wichmann
2023-02-11 17:46             ` Rich Felker
2023-02-11 17:50             ` Alexey Izbyshev
2023-02-11 17:59               ` Rich Felker
2023-02-11 18:08                 ` Alexey Izbyshev
2023-02-11 18:35                   ` Rich Felker
2023-02-11 19:28                     ` Alexey Izbyshev
2023-02-11 19:49                       ` Rich Felker
2023-02-11 20:14                         ` Alexey Izbyshev
2023-02-12  0:32                           ` Rich Felker
2023-02-12 18:23                             ` Alexey Izbyshev
2023-02-12 19:35                               ` Alexey Izbyshev
2023-02-12 20:04                                 ` Rich Felker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230210162957.GB4163@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=izbyshev@ispras.ru \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).