Bug 24595 - [2.28 Regression]: Deadlock in atfork handler which calls dlclose
Summary: [2.28 Regression]: Deadlock in atfork handler which calls dlclose
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: nptl (show other bugs)
Version: 2.28
: P2 normal
Target Milestone: 2.36
Assignee: Arjun Shankar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-21 22:26 UTC by Jeremy Drake
Modified: 2022-05-31 16:22 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Fix proposal (1.75 KB, patch)
2019-05-22 11:41 UTC, Adhemerval Zanella
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Drake 2019-05-21 22:26:51 UTC
https://bugs.gentoo.org/685024

The specific use case is kind of complicated.  I am using OpenVPN with a pkcs11 "smartcard" (actually GnuK).  The software stack involved at the time consists of

sys-libs/glibc-2.28-r6
sys-apps/pcsc-lite-1.8.24
dev-libs/opensc-0.18.0
dev-libs/pkcs11-helper-1.25.1
net-vpn/openvpn-2.4.6

Note that both glibc and opensc have newer versions now, but I have not gotten the chance to test with those.

The situation is that pkcs11-helper installs an atfork handler, whose purpose is to deinitialize the smartcard in the child process, to avoid inadvertantly allowing it to inherit an open connection to the smartcard.  As part of opensc's "Finalize", it dlclose()s its backend module, which in this case is pcsc-lite.  It appears that pcsc-lite also has an atfork handler installed (I did not investigate that one, but presumably it is also for the purpose of closing any open smartcard in the child).  Glibc registers a mechansim, apparently the same way that C++ destructors are registered, to remove any atfork handlers that are registered in a module when it is being unloaded.  Now that there is a (non-recursive) lock around the list of atfork handlers, and the handlers are called while that lock is held, attempting to unregister an atfork handler from within an atfork handler callback results in a deadlock.

There is no need for you to bisect - I already tracked down the commit in question - 27761a1042daf01987e7d79636d0c41511c6df3c - and confirmed that reverting this solves my deadlock.  If you want a simple test case to reproduce this, I think the most simple incarnation would be to have an executable and a shared library.  The executable would register with pthread_atfork(), dlopen() the shared library, and call something in it which also registers an atfork handler with pthread_atfork().  The executable would then fork(), and in its atfork child handler dlclose() the shared library.  This should deadlock with glibc 2.28 (and 2.29, though I have not yet confirmed this), but work fine with 2.27 and older.

Backtrace:
#0  0x4ec8eb4c in __lll_lock_wait_private () from /lib/libc.so.6
#1  0x4ec8efc6 in __unregister_atfork () from /lib/libc.so.6
#2  0x4ebbd5a9 in __cxa_finalize () from /lib/libc.so.6
#3  0x4dc1a3b1 in __do_global_dtors_aux () from /usr/lib/libpcsclite.so.1
#4  0x4f05ddca in _dl_close_worker () from /lib/ld-linux.so.2
#5  0x4f05e9e2 in _dl_close () from /lib/ld-linux.so.2
#6  0x4ecbdda9 in _dl_catch_exception () from /lib/libc.so.6
#7  0x4ecbde50 in _dl_catch_error () from /lib/libc.so.6
#8  0x4ed3d5e4 in _dlerror_run () from /lib/libdl.so.2
#9  0x4ed3ce9f in dlclose () from /lib/libdl.so.2
#10 0x4ea70664 in sc_dlclose () from /usr/lib/libopensc.so.6
#11 0x4e98d3fb in pcsc_finish () from /usr/lib/libopensc.so.6
#12 0x4e95b259 in sc_release_context () from /usr/lib/libopensc.so.6
#13 0x4eb3ebf2 in C_Finalize () from /usr/lib/opensc-pkcs11.so
#14 0x4eb3ec72 in C_Initialize () from /usr/lib/opensc-pkcs11.so
#15 0x4efe6a1b in __pkcs11h_threading_atfork_child ()
   from /usr/lib/libpkcs11-helper.so.1
#16 0x4ec8f1ed in __run_fork_handlers () from /lib/libc.so.6
#17 0x4ec43aa8 in fork () from /lib/libc.so.6
#18 0x4f00c540 in fork_compat () from /lib/libpthread.so.0
#19 0x125511d5 in openvpn_execve ()
#20 0x12551355 in openvpn_execve_check ()
Comment 1 Adhemerval Zanella 2019-05-22 11:41:00 UTC
Created attachment 11790 [details]
Fix proposal
Comment 2 Adhemerval Zanella 2019-05-22 11:46:09 UTC
(In reply to Jeremy Drake from comment #0)
> https://bugs.gentoo.org/685024
> 
> The specific use case is kind of complicated.  I am using OpenVPN with a
> pkcs11 "smartcard" (actually GnuK).  The software stack involved at the time
> consists of
> 
> sys-libs/glibc-2.28-r6
> sys-apps/pcsc-lite-1.8.24
> dev-libs/opensc-0.18.0
> dev-libs/pkcs11-helper-1.25.1
> net-vpn/openvpn-2.4.6
> 
> Note that both glibc and opensc have newer versions now, but I have not
> gotten the chance to test with those.
> 
> The situation is that pkcs11-helper installs an atfork handler, whose
> purpose is to deinitialize the smartcard in the child process, to avoid
> inadvertantly allowing it to inherit an open connection to the smartcard. 
> As part of opensc's "Finalize", it dlclose()s its backend module, which in
> this case is pcsc-lite.  It appears that pcsc-lite also has an atfork
> handler installed (I did not investigate that one, but presumably it is also
> for the purpose of closing any open smartcard in the child).  Glibc
> registers a mechansim, apparently the same way that C++ destructors are
> registered, to remove any atfork handlers that are registered in a module
> when it is being unloaded.  Now that there is a (non-recursive) lock around
> the list of atfork handlers, and the handlers are called while that lock is
> held, attempting to unregister an atfork handler from within an atfork
> handler callback results in a deadlock.

The glibc-2.27 and used to support it by copying the atfork handlers on multithread fork in lock-free way using dynamic stack allocation. A lock was taken only when a symbol was actually removed, insertions (pthread_atfork) and walking (running the handlers) only used atomics.

It has its drawbacks in some rough points, such some code complexity and dynamic stack allocation., so the refactor done on 27761a1042daf01987e7d79636d0c41511c6df3c was aimed mostly to simplify the code and remove the alloca usage.

> 
> There is no need for you to bisect - I already tracked down the commit in
> question - 27761a1042daf01987e7d79636d0c41511c6df3c - and confirmed that
> reverting this solves my deadlock.  If you want a simple test case to
> reproduce this, I think the most simple incarnation would be to have an
> executable and a shared library.  The executable would register with
> pthread_atfork(), dlopen() the shared library, and call something in it
> which also registers an atfork handler with pthread_atfork().  The
> executable would then fork(), and in its atfork child handler dlclose() the
> shared library.  This should deadlock with glibc 2.28 (and 2.29, though I
> have not yet confirmed this), but work fine with 2.27 and older.

The atfork handlers are not really well defined by POSIX standard, so it an implementation detail what functions and environments it aims to implements.  It looks like the described scenario is indeed used by some programs so I think it would be feasible to keep glibc support.

We need to make the atfork handling to use a recursive mutex and make the data structure to handle reentracy.  Unfortunately, I think the current one (dynarray) is not the best suitable one due on reentracy the iterator is usually an index (which might render invalid in an atfork handler removal). A simple double-linked list should handle it.

Could you try the attached patch to see if it fixes the issue you are seeing? I would like just to confirm before sending upstream to review.

Also keep in mind that the described use case is *far* from portable and most likely will fail on different environments.  The reduced test case based on your description:

--
$ cat tst-atfork3.c
#include <stdio.h>
#include <dlfcn.h>
#include <pthread.h>
#include <unistd.h>
#include <stdlib.h>
#include <assert.h>

static void *handler;

static void
prepare_dlclose (void)
{
  assert (dlclose (handler) == 0);
}

static void
parent_null (void)
{
}

static void
child_null (void)
{
}

static void *
thread_func (void *arg)
{
  return 0;
}

int
main (void)
{
  {
    /* Make the process acts as multithread.  */
    pthread_attr_t attr;
    assert (pthread_attr_init (&attr) == 0);
    assert (pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED) == 0);
    pthread_t thr;
    assert (pthread_create (&thr, &attr, thread_func, NULL) == 0);
  }

  {
    assert (pthread_atfork (prepare_dlclose, parent_null, child_null) == 0);

    handler = dlopen ("tst-atfork3mod.so", RTLD_NOW);
    assert (handler != NULL);

    int (*atfork3mod_func) (void);
    atfork3mod_func = dlsym (handler, "atfork3mod_func");
    assert (atfork3mod_func != NULL);

    atfork3mod_func ();

    pid_t pid = fork ();
    if (pid == 0)
      exit (0);
  }

  return 0;
}
$ cat tst-atfork3mod.c
#include <unistd.h>
#include <stdlib.h>
#include <pthread.h>
#include <assert.h>

static void
mod_prepare (void)
{
}

static void
mod_parent (void)
{
}

static void
mod_child (void)
{
}

int atfork3mod_func (void)
{
  assert (pthread_atfork (mod_prepare, mod_parent, mod_child) == 0);

  return 0;
}
--

It fails on FreeBSD11 and AIX72 (former with a hang, latter with a invalid jump at fork handler).  It works on Solaris11 and Android (by reading bionic atfork implementation).
Comment 3 Jeremy Drake 2019-05-22 17:34:14 UTC
(In reply to Adhemerval Zanella from comment #2)
> Could you try the attached patch to see if it fixes the issue you are
> seeing? I would like just to confirm before sending upstream to review.

I will try to get to that this weekend.  I just had a quick read-through, and I was curious about the __libc_lock_unlock_recursive.  I had read that recursive locks generally need to be unlocked on the same thread they were locked on, but in the case of the child handler, it is not only a different thread, but a different process entirely.  Is this a supported case for this lock type?  It seems it may be backed by a recursive pthread mutex, whose manpage says "If a thread attempts to unlock a mutex that it has not locked or a mutex which is unlocked, an error shall be returned."

> 
> Also keep in mind that the described use case is *far* from portable and
> most likely will fail on different environments.  The reduced test case
> based on your description:

The exact case is that the dlclose happens in the child handler, not the prepare handler, but other than that this is what I was describing.

> It fails on FreeBSD11 and AIX72 (former with a hang, latter with a invalid
> jump at fork handler).  It works on Solaris11 and Android (by reading bionic
> atfork implementation).

It might be nice if someone who understood such things told the OpenSC/pkcs11-helper people that.
Comment 4 Adhemerval Zanella 2019-05-22 18:26:43 UTC
(In reply to Jeremy Drake from comment #3)
> (In reply to Adhemerval Zanella from comment #2)
> > Could you try the attached patch to see if it fixes the issue you are
> > seeing? I would like just to confirm before sending upstream to review.
> 
> I will try to get to that this weekend.  I just had a quick read-through,
> and I was curious about the __libc_lock_unlock_recursive.  I had read that
> recursive locks generally need to be unlocked on the same thread they were
> locked on, but in the case of the child handler, it is not only a different
> thread, but a different process entirely.  Is this a supported case for this
> lock type?  It seems it may be backed by a recursive pthread mutex, whose
> manpage says "If a thread attempts to unlock a mutex that it has not locked
> or a mutex which is unlocked, an error shall be returned."

The internal glibc locks (__libc_lock_*) are private to process, so the child will unlock its copy after fork (remind that although not all thread are replicated in Linux fork, the memory is).

> 
> > 
> > Also keep in mind that the described use case is *far* from portable and
> > most likely will fail on different environments.  The reduced test case
> > based on your description:
> 
> The exact case is that the dlclose happens in the child handler, not the
> prepare handler, but other than that this is what I was describing.

In fact is you dlclose on prepare the parent will hang, while if you do in the child it will hang in child.

> 
> > It fails on FreeBSD11 and AIX72 (former with a hang, latter with a invalid
> > jump at fork handler).  It works on Solaris11 and Android (by reading bionic
> > atfork implementation).
> 
> It might be nice if someone who understood such things told the
> OpenSC/pkcs11-helper people that.
Comment 5 Jeremy Drake 2019-05-24 23:01:14 UTC
I did some playing in a test chroot with the test code and patch you provided.  I modified the test case to call dlclose in the child handler, since that is what I saw happening, and added a waitpid call in the parent case (else on the pid == 0 if after the fork).  Your proposed patch seems to do the trick there.

I upgraded to sys-libs/glibc-2.29-r2 (which contains 669ff911e2571f74a2668493e326ac9a505776bd) and dev-libs/opensc-0.19.0-r2 on the real system I was having this issue on, and was surprised to see the hang/deadlock had gone away.  One of the first things I found when I idenitifed this problem was that 669ff911 commit, and I attempted applying that as a patch and it did not solve the problem.  Either I screwed that up, or the opensc change did something too.  I am considering downgrading opensc back to 0.18.0 to attempt to determine which of these are the case.
Comment 6 Jeremy Drake 2019-05-25 01:02:32 UTC
I downgraded opensc to 0.18.0 and the hang/deadlock is back.  I also confirmed that there was a second thread in the parent.  So I can test the patch in the real scenario after all.
Comment 7 Jeremy Drake 2019-05-27 05:20:47 UTC
(In reply to Adhemerval Zanella from comment #2)
> We need to make the atfork handling to use a recursive mutex and make the
> data structure to handle reentracy.  Unfortunately, I think the current one
> (dynarray) is not the best suitable one due on reentracy the iterator is
> usually an index (which might render invalid in an atfork handler removal).
> A simple double-linked list should handle it.
> 
> Could you try the attached patch to see if it fixes the issue you are
> seeing? I would like just to confirm before sending upstream to review.

I finally got a chance to test this in the original situation where I saw the hang, and it seems to do the trick.  Even if opensc-0.19.0 has removed a background thread, which causes the hang to no longer trigger due to the 669ff911e2571f74a2668493e326ac9a505776bd commit, your approach seems safer in the case when the list changes while being iterated.
Comment 8 Adhemerval Zanella 2019-05-27 11:45:00 UTC
(In reply to Jeremy Drake from comment #7)
> (In reply to Adhemerval Zanella from comment #2)
> > We need to make the atfork handling to use a recursive mutex and make the
> > data structure to handle reentracy.  Unfortunately, I think the current one
> > (dynarray) is not the best suitable one due on reentracy the iterator is
> > usually an index (which might render invalid in an atfork handler removal).
> > A simple double-linked list should handle it.
> > 
> > Could you try the attached patch to see if it fixes the issue you are
> > seeing? I would like just to confirm before sending upstream to review.
> 
> I finally got a chance to test this in the original situation where I saw
> the hang, and it seems to do the trick.  Even if opensc-0.19.0 has removed a
> background thread, which causes the hang to no longer trigger due to the
> 669ff911e2571f74a2668493e326ac9a505776bd commit, your approach seems safer
> in the case when the list changes while being iterated.

Thanks for confirming this fix at least improve the situation. Florian Weimer has raised some question about the validity of the fix in some cases, so we are still discussion upstream.
Comment 9 Arjun Shankar 2022-05-31 16:20:35 UTC
This should now be fixed in master with:

commit 52a103e237329b9f88a28513fe7506ffc3bd8ced
Author: Arjun Shankar <arjun@redhat.com>
Date:   Tue May 24 17:57:36 2022 +0200

    Fix deadlock when pthread_atfork handler calls pthread_atfork or dlclose
    
    In multi-threaded programs, registering via pthread_atfork,
    de-registering implicitly via dlclose, or running pthread_atfork
    handlers during fork was protected by an internal lock.  This meant
    that a pthread_atfork handler attempting to register another handler or
    dlclose a dynamically loaded library would lead to a deadlock.
    
    This commit fixes the deadlock in the following way:
    
    During the execution of handlers at fork time, the atfork lock is
    released prior to the execution of each handler and taken again upon its
    return.  Any handler registrations or de-registrations that occurred
    during the execution of the handler are accounted for before proceeding
    with further handler execution.
    
    If a handler that hasn't been executed yet gets de-registered by another
    handler during fork, it will not be executed.   If a handler gets
    registered by another handler during fork, it will not be executed
    during that particular fork.
    
    The possibility that handlers may now be registered or deregistered
    during handler execution means that identifying the next handler to be
    run after a given handler may register/de-register others requires some
    bookkeeping.  The fork_handler struct has an additional field, 'id',
    which is assigned sequentially during registration.  Thus, handlers are
    executed in ascending order of 'id' during 'prepare', and descending
    order of 'id' during parent/child handler execution after the fork.
    
    Two tests are included:
    
    * tst-atfork3: Adhemerval Zanella <adhemerval.zanella@linaro.org>
      This test exercises calling dlclose from prepare, parent, and child
      handlers.
    
    * tst-atfork4: This test exercises calling pthread_atfork and dlclose
      from the prepare handler.
    
    [BZ #24595, BZ #27054]
    
    Co-authored-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
    Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
Comment 10 Arjun Shankar 2022-05-31 16:22:53 UTC
I have also backported the fix to release/2.35/master and release/2.34/master.