mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [musl] Restrictions on child context after multithreaded fork
Date: Fri, 14 Aug 2020 22:40:49 -0400	[thread overview]
Message-ID: <20200815024049.GS3265@brightrain.aerifal.cx> (raw)
In-Reply-To: <45ce1753-52ec-5890-58c1-5350cb6c8c39@adelielinux.org>

On Fri, Aug 14, 2020 at 07:47:28PM -0500, A. Wilcox wrote:
> On 14/08/2020 16:41, Rich Felker wrote:
> > musl 1.2.1 has exposed bugs in several applications and libraries
> > caused by async-signal-unsafe code between (multithreaded) fork and
> > subsequent exec. So far, dbus library code, pulseaudio library code,
> > and libvirt have been found to be affected. 
> > 
> > Fixing the affected library code looks very straightforward; it's just
> > a matter of doing proper iterations of existing data/state rather than
> > allocating lists and using opendir on procfs and such. I've discussed
> > fixes with Alpine Linux folks and I believe fixes have been tested,
> > but I don't see any patches in aports yet.
> 
> Over at Adélie we've been monitoring the situation intently.  We're not
> bumping musl to 1.2.1 until we have time to do a full tree test (similar
> to time64) due to this.

Just be aware: any bugs exposed by this as deadlocks were already
present, just manifesting as access to inconsistent state. It's
possible for them to yield freelist corruption, etc. So even if not
upgrading musl yet, fixing them should be treated as high-priority.

> > I've seen suspicions that the switch to mallocng exposed this, but I'm
> > pretty sure it was:
> > 
> > https://git.musl-libc.org/cgit/musl/commit/?id=e01b5939b38aea5ecbe41670643199825874b26c
> 
> Doesn't make sense to me.  We backported that to Adélie's musl 1.2.0
> package the day it was committed to musl, and Firefox has been rock
> solid for me for months.  No deadlocks, no weird behaviour.  Similar
> with other apps that use D-Bus and Pulse.  At least on ppc64, x86_64,
> and aarch64 (my daily driver arches), it's been through almost three
> months of usage without issue.
> 
> Perhaps it's the mixture of that commit + malloc-ng?

I was about to say I'm strongly recommending e01b5939b3 for
backporting, but you've already got that. With oldmalloc each size
class has a separate lock, so instead of always getting a deadlock if
parent was using malloc at the time of fork, you'd expect to only get
it if parent was allocating in the same class the child requests, or
if parent was performing free (when a global free-lock is held
momentarily). I'm still rather surprised you don't hit them at all,
rather than just hitting them less frequently.

> > Note that this is a type of bug that's possibly hard to get upstreams
> > to take seriously. libvirt in particular, despite having multiple
> > comments throughout the source warning developers that they can't do
> > anything AS-unsafe between fork and exec, is somehow deeming malloc an
> > exception to that rule because they want to use it (despite it clearly
> > not being necessary).
> 
> libvirt already isn't usable on musl, so this isn't really something we
> are going to spend any time on.  Others are free to do it.

Alpine is and has been using it, apparently. Are you aware of others
blockers for compat with musl?

> > And the dbus issue has been known for a long time; see open bug:
> > 
> > https://gitlab.freedesktop.org/dbus/dbus/-/issues/173
> > (originally: https://bugs.freedesktop.org/show_bug.cgi?id=100843)
> > 
> > This is largely because glibc attempts to make the erroneous usage by
> > these libraries work (more on that below).
> > 
> > The next issue of POSIX (Issue 8) will drop the requirement that fork
> > be AS-safe, as a result of Austin Group tracker issue #62. This makes
> > the glibc behavior permissible/conforming, but there does not seem to
> > be any effort on the POSIX side to drop the requirement on
> > applications not to do AS-unsafe things in the child before exec, so
> > regardless of this change, what these libraries are doing is still
> > wrong.
> > 
> > In order to make the child environment unrestricted after fork, either
> > fork must hold *all* locks at the time the actual fork syscall takes
> > place, or it must be able to reset any state protected by a lock that
> > was held in the parent (or some mix of the two). It's fundamentally
> > impossible to do this completely (in a way that lets the child run
> > unrestricted), since some locks in the parent may be held arbitrarily
> > long such that fork waiting on them would deadlock. In particular, any
> > stdio FILE lock may be held indefinitely because there's a blocking
> > operation in progress on the underlying fd, or because the application
> > has called flockfile. Thus, at best, the implementation can give the
> > child an environment where fflush(0) and exit() still deadlock.
> > 
> > In case we do want to follow a direction of trying to provide some
> > degree of relaxation of restrictions on the child (taking the liberty
> > of POSIX-future drop of fork's AS-safety requirement), I did a quick
> > survey of libc-internal locks, and found:
> > 
> > - at_quick_exit
> > - atexit
> > - dlerror
> > - gettext
> > - malloc
> > - pthread_atfork (already necessarily held at fork)
> > - random
> > - sem_open
> > - stdio open file list (vs individual FILEs)
> > - syslog
> > - timezone
> > 
> > This list looks tractable. Aside from malloc, whose locks would need
> > to be taken last since the others may call malloc, these don't seem to
> > have any lock order dependencies between them, and each one's lock
> > functions could be provided as strong overrides to weak no-op
> > definitions in fork.c.
> 
> 
> Some of these seem more important than others.  Including the list in
> your follow-up, I would say a priority would be
> 
> - malloc
> - gettext
> - random
> 
> in roughly that order.  That's just based on my experience with these
> not-great codebases.

If we go this route, I'll cover them all; it's not any harder than
doing just some of them. My enumeration of them was more to determine
whether the problem is tractable and whether any have special
properties than make them especially difficult. Assuming that's not
the case, there's a standard pattern. In the source file for "foo":

void __foo_atfork(int x)
{
	if (x<0) LOCK(mylock);
	else UNLOCK(mylock);
}

and in fork.c:

static void dummy_atfork(int x)
{
}

weak_alias(dummy_atfork, __foo_atfork);

with __foo_atfork(-1) before the fork and __foo_atfork(!ret) after, in
proper ordering (after __fork_handler, before __block_all_sigs for the
locking, and reverse order for the unlocking).

malloc needs to be last, and there need to be either 2 versions to
cover the bump allocator or some special machinery so the bump
allocator's function gets replaced by the real one if real allocator
is linked.

Rich

  reply	other threads:[~2020-08-15  2:41 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-14 21:41 Rich Felker
2020-08-14 22:02 ` Florian Weimer
2020-08-14 22:14   ` Rich Felker
2020-08-15  0:47 ` A. Wilcox
2020-08-15  2:40   ` Rich Felker [this message]
2020-08-15  2:07 ` Ariadne Conill
2020-08-15  3:02   ` Rich Felker
2020-08-15  6:51 ` Timo Teras
2020-08-15 11:51   ` Natanael Copa
2020-08-15 16:25     ` Rich Felker
2020-08-16  1:27       ` Rich Felker
2020-08-16 12:48         ` Natanael Copa
2020-08-16  3:57 ` Rich Felker
2020-08-16  9:10   ` Florian Weimer
2020-08-16 16:56     ` Rich Felker
2020-08-16 17:11       ` Florian Weimer
2020-08-16 18:33         ` Rich Felker
2020-08-16  7:05 ` Pirmin Walthert
2020-08-16 16:55   ` Rich Felker
2020-09-30 18:38 ` 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=20200815024049.GS3265@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --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).