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: Sat, 11 Feb 2023 09:52:47 -0500	[thread overview]
Message-ID: <20230211145246.GH4163@brightrain.aerifal.cx> (raw)
In-Reply-To: <63c0897d647936c946268f5a967a5e4d@ispras.ru>

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
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Nice catch -- this is actually a bug. pthread_detach is not permitted
to be a cancellation point.

Rich

  reply	other threads:[~2023-02-11 14:53 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
2023-02-11 14:45       ` Alexey Izbyshev
2023-02-11 14:52         ` Rich Felker [this message]
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=20230211145246.GH4163@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).