[Bug malloc/16742] New: race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[Bug malloc/16742] New: race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[prumpf at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

            Bug ID: 16742
           Summary: race condition: pthread_atfork() called before first
                    malloc() results in unexpected locking
                    behaviour/deadlocks
           Product: glibc
           Version: 2.19
            Status: NEW
          Severity: normal
          Priority: P2
         Component: malloc
          Assignee: unassigned at sourceware dot org
          Reporter: prumpf at gmail dot com

Created attachment 7488
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7488&action=edit
Test case

This issue appears with pthreads and NPTL on the latest git version of glibc
(and the latest git version of Perl).

I'm not sure this is a bug according to the specifications, but it is both
extremely counterintuitive and occurs in practice, with the latest version of
Perl: https://rt.perl.org/Public/Bug/Display.html?id=121490  I believe it is
worth fixing.

When pthread_atfork() is called prior to the first malloc() call in the
program, the malloc atfork handlers are called before the user's atfork
handlers, resulting in unpredictable locking order and deadlocks.

Here's a test case:

#include <pthread.h>
#include <unistd.h>
#include <stdlib.h>

pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

void lock(void)
{
  pthread_mutex_lock(&mutex);
}

void unlock(void)
{
  pthread_mutex_unlock(&mutex);
}

void *lock_then_malloc(void *dummy)
{
  pthread_mutex_lock(&mutex);
  sleep(2);
  volatile void *throwaway = malloc(1024);
  pthread_mutex_unlock(&mutex);
  return NULL;
}

int main(int argc, char **argv)
{
  pthread_attr_t attr;
  pthread_t tid;

  volatile void *throwaway;
  if (argc > 1)
    throwaway = malloc(1024);

  pthread_atfork(lock, unlock, unlock);

  pthread_attr_init(&attr);
  pthread_create(&tid, &attr, lock_then_malloc, NULL);

  sleep(1);
  fork();

  return 0;
}


The expected behaviour is that the program terminates (after two seconds)
whether or not an extra argument is supplied; the actual behaviour is that the
program terminates only if an extra argument is present: the (otherwise
useless) call to malloc() means ptmalloc_init() gets called before rather than
after main() calls pthread_atfork(), changing the order in which atfork
handlers are run. In the case without an extra argument, the main thread then
holds the mutex and is waiting on malloc/arena.c's list_lock, while the other
thread hold list_lock and is waiting on the mutex.

This is essentially what Perl does: it calls pthread_atfork() before first
calling malloc(), then locks a mutex both in the atfork handler and in another
thread which then calls malloc().

While I think this definitely needs to be fixed, I'm not sure what the best fix
would be. A simplistic workaround would be to call malloc() from
pthread_create(), while a more elegant solution would be to allow a priority
argument for pthread_atfork(), with user-defined handlers always ending up
outside of malloc/glibc-defined handlers. The ChangeLog suggests there once
existed a function called __register_atfork_malloc(), which might have done
this already.

(Note that we never get to unlock(), so it's irrelevant whether
pthread_mutex_unlock() works after the fork(). )

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/16742] race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[prumpf at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

prumpf at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |prumpf at gmail dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/16742] race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/16742] race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[prumpf at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

--- Comment #1 from prumpf at gmail dot com ---
Created attachment 7633
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7633&action=edit
proposed patch

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/16742] race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[prumpf at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

--- Comment #2 from prumpf at gmail dot com ---
I've attached a patch that would fix this issue (and a closely-related one
described at https://sourceware.org/ml/libc-alpha/2013-01/msg01051.html), but
which I'm not entirely satisfied is the right thing to do yet (it also contains
a debugging XXX). Still, it might serve as a starting point for fixing this.
The draft message I'd prepared for it follows:

------------

This is a bug report and proposed patch for the git version of glibc; it is
specific to NPTL.

There appear to be at least two deadlock problems caused by the order in which
atfork handlers, in particular ptmalloc_lock_all, are run.

https://sourceware.org/bugzilla/show_bug.cgi?id=16742 means ptmalloc_lock_all
must be run after other atfork handlers, even if registered after them.
https://sourceware.org/ml/libc-alpha/2013-01/msg01051.html is a bit trickier:
it demonstrates that lock ordering should be "lock IO list before taking malloc
locks", which means ptmalloc_lock_all has to be run with the IO list lock
already held. Other solutions would work as well, but I've focussed on that one
since it puts the least constraints on libio. This bug can be reproduced by
running a test program with the right breakpoints set in gdb.
I've prepared a patch that I believe fixes the problem, but I'm not overly
attached to doing it this way.

This patch fixes both deadlocks and introduces the concept of an atfork
handler's priority—a high-priority handler is run inside of a low-priority one;
it creates a new atfork handler for the IO lock, and gives it priority
intermediate between low-priority user handlers and the high-priority malloc
atfork handler; it uses a high priority for malloc's atfork handler; it adjusts
register-atfork.c and unregister-atfork.c so they keep the __fork_handlers list
sorted by priority.

The complication introduced by this patch is unfortunate, but I believe it is
the best approach. However, an alternative would be: don't introduce a priority
level; put _IO_list_lock into an atfork handler that's installed first; make
thread_atfork install malloc's atfork handler at the end of the list, no matter
what. (I believe that approach would leave us with very fragile code and
without the ability to put more of our MT-specific fork code into atfork
handlers.)
I believe there are no new restrictions on libio code, but there is a (new?)
restriction on malloc: code must not hold malloc locks while waiting for I/O to
finish, unless it gets the I/O lock first. In particular, this applies to
diagnostic output and malloc.c's assert(), which might deadlock rather than
printing a message.
In addition to the alternative mentioned above, I've also considered another
approach: pretend to take both the I/O and the malloc lock or neither: first
lock A, then try to lock B; in the failure case, unlock A, lock B, then try to
lock A, and so on. However, that's complicated, doesn't extend well to the case
of several high-priority atfork handlers, and might result in a livelock.
I do not suggest making the priority level accessible from user space: it's
easy enough for glibc users to implement their own priority system or call
pthread_atfork() in the right order.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
>From glibc-bugs-return-22807-listarch-glibc-bugs=sources.redhat.com@sourceware.org Fri Jun 13 06:06:57 2014
Return-Path: <glibc-bugs-return-22807-listarch-glibc-bugs=sources.redhat.com@sourceware.org>
Delivered-To: listarch-glibc-bugs@sources.redhat.com
Received: (qmail 16751 invoked by alias); 13 Jun 2014 06:06:56 -0000
Mailing-List: contact glibc-bugs-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <glibc-bugs.sourceware.org>
List-Subscribe: <mailto:glibc-bugs-subscribe@sourceware.org>
List-Post: <mailto:glibc-bugs@sourceware.org>
List-Help: <mailto:glibc-bugs-help@sourceware.org>, <http://sourceware.org/lists.html#faqs>
Sender: glibc-bugs-owner@sourceware.org
Delivered-To: mailing list glibc-bugs@sourceware.org
Received: (qmail 16713 invoked by uid 48); 13 Jun 2014 06:06:51 -0000
From: "fweimer at redhat dot com" <sourceware-bugzilla@sourceware.org>
To: glibc-bugs@sourceware.org
Subject: [Bug libc/17048] posix_spawn_file_actions_addopen fails to copy the path argument (CVE-2014-4043)
Date: Fri, 13 Jun 2014 06:06:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: glibc
X-Bugzilla-Component: libc
X-Bugzilla-Version: unspecified
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: fweimer at redhat dot com
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: P2
X-Bugzilla-Assigned-To: fweimer at redhat dot com
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags: security+
X-Bugzilla-Changed-Fields: short_desc
Message-ID: <bug-17048-131-VIpiCkop8v@http.sourceware.org/bugzilla/>
In-Reply-To: <bug-17048-131@http.sourceware.org/bugzilla/>
References: <bug-17048-131@http.sourceware.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://sourceware.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-06/txt/msg00183.txt.bz2
Content-length: 597

https://sourceware.org/bugzilla/show_bug.cgi?id\x17048

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|posix_spawn_file_actions_ad |posix_spawn_file_actions_ad
                   |dopen fails to copy the     |dopen fails to copy the
                   |path argument               |path argument
                   |                            |(CVE-2014-4043)

--
You are receiving this mail because:
You are on the CC list for the bug.

[Bug malloc/16742] race condition: pthread_atfork() called before first malloc() results in unexpected locking behaviour/deadlocks

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16742

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security?

-- 
You are receiving this mail because:
You are on the CC list for the bug.