Re: [PATCH] elf: Release dl_load_lock before running dlopen constructors (BZ 15686)

Am Sonntag, 12. Juli 2026, 05:25:32 Japanische Normalzeit schrieb [email protected]:
> From: Artem Proskurnev <[email protected]>
> 
> This addresses one instance of the long-standing class of deadlocks
> described in BZ #15686: ELF constructors and destructors invoked by
> the dynamic loader run with dl_load_lock held, so any code path in
> those constructors that itself needs dl_load_lock deadlocks.

In any case this is post release stuff.

> 
> dl_open_worker holds dl_load_lock across the entire _dl_open call,
> including the call to call_dl_init that runs the new objects'
> constructors.  If one of those constructors spawns a thread whose
> first access to a thread_local object triggers
> __cxa_thread_atexit_impl, the new thread blocks trying to acquire
> dl_load_lock -- which is held by the dlopen thread -- deadlocking
> the process.  The same deadlock arises when the spawned thread calls
> a function that triggers NSS module loading through _dl_open, or any
> other code path that needs dl_load_lock.
> 
> The blocking site is __cxa_thread_atexit_impl at
> stdlib/cxa_thread_atexit_impl.c.  BZ #28357 was a partial fix for
> the wider BZ #15686 problem: it moved dl_open_worker_begin and
> _dl_close_worker to the finer-grained dl_load_tls_lock (commit
> 024a7640ab) and used that new lock in pthread_create and
> __tls_get_addr.  __cxa_thread_atexit_impl, however, still takes
> dl_load_lock to protect its DSO lookup (_dl_find_dso_for_object)
> against a racing dlclose, and that path is not covered by the
> BZ #28357 fix.  Moving it to dl_load_tls_lock is not straightforward
> because _dl_find_dso_for_object walks _ns_loaded, which is protected
> by dl_load_lock rather than dl_load_tls_lock.
> 
> This patch takes the alternative approach of releasing dl_load_lock
> during constructor execution.  At the point where call_dl_init runs,
> the following invariants hold:
> 
>   * All link_map structures for the newly loaded DSO and its
>     dependencies are fully initialized and immutable.
>   * The DSO has l_direct_opencount == 1 (incremented in
>     dl_open_worker_begin), so a concurrent dlclose cannot unload it:
>     _dl_close_worker short-circuits when the count is non-zero.
>   * Implicit dependencies are protected by the l_map_used marking in
>     _dl_close_worker, which transitively marks the l_initfini chain
>     of any map with non-zero l_direct_opencount.
>   * dl_iterate_phdr uses dl_load_write_lock rather than dl_load_lock
>     and is unaffected by the unlock.  Other threads calling
>     dl_iterate_phdr during the constructor may observe the DSO before
>     its constructor has run; this is consistent with POSIX, which
>     does not guarantee atomic appearance of dlopen'd objects, and is
>     equivalent to dlsym from inside a constructor observing
>     partially-initialized main-executable symbols.
>   * Recursive dlopen from a constructor re-acquires dl_load_lock
>     normally in _dl_open and proceeds serially.
> 
> The lock is re-acquired immediately after constructors complete,
> before add_to_global_update and the lock/unlock pairing expected by
> _dl_open.
> 
> Exception safety: call_dl_init is invoked via
> _dl_catch_exception (NULL, ...) so that lazy binding failures are
> fatal (the process exits through _dl_fatal_printf); therefore the
> re-lock is not required on the error path.  C++ exceptions thrown
> from constructors are a separate, pre-existing concern: dl exception
> handling uses setjmp/longjmp rather than C++ unwinding, so a thrown
> exception may leave locks in any state regardless of this patch.
> Releasing the lock is strictly safer than holding it in that case.
> 
> Minimal reproducer: a DSO whose constructor calls
> gdk_pixbuf_new_from_file on a system where the glycin image loader
> is wired in via gdk-pixbuf reaches a sandboxed loader process spawn,
> which in turn calls std::thread::spawn; the spawned thread's first
> thread_local access (__cxa_thread_atexit_impl) blocks on dl_load_lock
> held by the dlopen caller.  The same hang reproduces with any
> constructor that spawns a thread touching thread_local state or
> triggering NSS module loads.
> 
> A regression test is added in sysdeps/pthread/tst-create2.c with its
> DSO in tst-create2mod.c.  The DSO constructor spawns a worker thread
> that calls __cxa_thread_atexit_impl and then joins it; under the
> pre-fix locking model the join deadlocks and the test framework
> times out.  The test follows the layout of tst-create1 (BZ #28357),
> which covers the pthread_create leg of the same bug class.
> 
> Tested on x86_64-linux-gnu. Verified both directions of the regression
> test: sysdeps/pthread/tst-create2 deadlocks (times out after 10 s)
> when run against the unpatched tree, and passes (exit 0) with this
> patch applied. No regressions in the full glibc test suite (make check).
> 
> Co-authored-by: Alexander Pevzner <[email protected]>
> Signed-off-by: Artem Proskurnev <[email protected]>
> Signed-off-by: Alexander Pevzner <[email protected]>
> ---
>  elf/dl-open.c                    | 26 ++++++++++++++
>  sysdeps/pthread/Makefile         |  7 ++++
>  sysdeps/pthread/tst-create2.c    | 48 +++++++++++++++++++++++++
>  sysdeps/pthread/tst-create2mod.c | 62 ++++++++++++++++++++++++++++++++
>  4 files changed, 143 insertions(+)
>  create mode 100644 sysdeps/pthread/tst-create2.c
>  create mode 100644 sysdeps/pthread/tst-create2mod.c
> 
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index cf4749694f..2e824d86bc 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -792,11 +792,37 @@ dl_open_worker (void *a)
>    int mode = args->mode;
>    struct link_map *new = args->map;
>  
> +  /* The link_map structures are fully initialized at this point.  We
> +     can release dl_load_lock so that constructors can safely spawn
> +     threads without deadlocking on dl_load_lock (e.g. if the new thread
> +     accesses a thread_local variable and calls __cxa_thread_atexit_impl,
> +     or performs operations that trigger NSS module loading).
> +
> +     The DSO has l_direct_opencount == 1 at this point, so it cannot be
> +     unloaded by a concurrent dlclose -- _dl_close_worker skips objects
> +     with nonzero opencount, and l_map_used marking transitively protects
> +     implicit dependencies.  Concurrent dlopen and dlclose in other
> +     threads are not blocked while the lock is released, but they are
> +     safe: our DSO's link_map structures are fully initialized and
> +     immutable at this point.
> +
> +     Notably, dl_iterate_phdr (which uses dl_load_write_lock, not
> +     dl_load_lock) can observe the DSO before its constructor has run.
> +     This is consistent with POSIX, which does not guarantee that a
> +     dlopen'd DSO appears atomically -- and is no different from dlsym
> +     inside a constructor observing partially-initialized symbols from
> +     the main executable.  */
> +  __rtld_lock_unlock_recursive (GL(dl_load_lock));
> +
>    /* Run the initializer functions of new objects.  Temporarily
>       disable the exception handler, so that lazy binding failures are
>       fatal.  */
>    _dl_catch_exception (NULL, call_dl_init, args);
>  
> +  /* Re-acquire dl_load_lock for the final global scope update and for
> +     the lock/unlock pairing expected by _dl_open.  */
> +  __rtld_lock_lock_recursive (GL(dl_load_lock));
> +
>    /* Now we can make the new map available in the global scope.  */
>    if (mode & RTLD_GLOBAL)
>      add_to_global_update (new);
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index d0f3cd59ac..9451a5c25c 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -349,6 +349,7 @@ tests += \
>    tst-atfork3 \
>    tst-atfork4 \
>    tst-create1 \
> +  tst-create2 \
>    tst-fini1 \
>    tst-pt-tls4 \
>    # tests
> @@ -365,6 +366,7 @@ modules-names += \
>    tst-atfork3mod \
>    tst-atfork4mod \
>    tst-create1mod \
> +  tst-create2mod \
>    tst-fini1mod \
>    tst-stack2-mod \
>    tst-tls4moda \
> @@ -377,6 +379,7 @@ tst-atfork2mod.so-no-z-defs = yes
>  tst-atfork3mod.so-no-z-defs = yes
>  tst-atfork4mod.so-no-z-defs = yes
>  tst-create1mod.so-no-z-defs = yes
> +tst-create2mod.so-no-z-defs = yes
>  
>  ifeq ($(build-shared),yes)
>  # Build all the modules even when not actually running test programs.
> @@ -540,6 +543,10 @@ LDFLAGS-tst-create1 = -Wl,-export-dynamic
>  $(objpfx)tst-create1: $(shared-thread-library)
>  $(objpfx)tst-create1.out: $(objpfx)tst-create1mod.so
>  
> +LDFLAGS-tst-create2 = -Wl,-export-dynamic
> +$(objpfx)tst-create2: $(shared-thread-library)
> +$(objpfx)tst-create2.out: $(objpfx)tst-create2mod.so
> +
>  $(objpfx)tst-stack2.out: $(objpfx)tst-stack2-mod.so
>  $(objpfx)tst-stack2-mod.so: $(shared-thread-library)
>  LDFLAGS-tst-stack2-mod.so = -Wl,-z,execstack
> diff --git a/sysdeps/pthread/tst-create2.c b/sysdeps/pthread/tst-create2.c
> new file mode 100644
> index 0000000000..d7df8aafc5
> --- /dev/null
> +++ b/sysdeps/pthread/tst-create2.c
> @@ -0,0 +1,48 @@
> +/* Verify that a thread spawned by a dlopen constructor can register a
> +   TLS destructor via __cxa_thread_atexit_impl without deadlocking on
> +   dl_load_lock held by the dlopen caller (BZ 15686).
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* Reproducer for one instance of the deadlock class described in
> +   BZ 15686.
> +
> +   thread 1: dlopen -> ctor -> pthread_create(worker) -> pthread_join(worker)
> +   thread 2 (worker): __cxa_thread_atexit_impl -> tries to lock dl_load_lock
> +
> +   Before the fix in elf/dl-open.c, dl_load_lock is held across
> +   call_dl_init, so thread 2 blocks on a lock that thread 1 will only
> +   release after pthread_join returns -- a deadlock that the
> +   test-driver timeout surfaces as a failure.  After the fix,
> +   dl_load_lock is released before constructors run and reacquired
> +   afterwards, so thread 2 makes progress and the dlopen call returns.  */
> +
> +#include <stdio.h>
> +#include <support/xdlfcn.h>
> +
> +static int
> +do_test (void)
> +{
> +  dprintf (1, "main: dlopen tst-create2mod.so\n");
> +  void *h = xdlopen ("tst-create2mod.so", RTLD_NOW);
> +  dprintf (1, "main: dlopen done\n");
> +  xdlclose (h);
> +  dprintf (1, "main: dlclose done\n");
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/pthread/tst-create2mod.c b/sysdeps/pthread/tst-create2mod.c
> new file mode 100644
> index 0000000000..d8254f729f
> --- /dev/null
> +++ b/sysdeps/pthread/tst-create2mod.c
> @@ -0,0 +1,62 @@
> +/* Verify that a thread spawned by a dlopen constructor can register a
> +   TLS destructor via __cxa_thread_atexit_impl without deadlocking on
> +   dl_load_lock held by the dlopen caller (BZ 15686).
> +   Copyright (C) 2026 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <pthread.h>
> +#include <stdlib.h>
> +#include <dso_handle.h>
> +
> +typedef struct { void *val; } A;
> +
> +static void
> +A_dtor (void *obj)
> +{
> +  ((A *) obj)->val = obj;
> +}
> +
> +/* Acquire dl_load_lock via __cxa_thread_atexit_impl.  Called from the
> +   worker thread spawned by the constructor below; if the constructor
> +   runs with dl_load_lock held, this blocks and pthread_join in the
> +   constructor never returns.  */
> +static void
> +reg_dtor (void)
> +{
> +  static __thread A b;
> +  __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
> +}
> +
> +static void *
> +worker (void *arg)
> +{
> +  reg_dtor ();
> +  return NULL;
> +}
> +
> +static void __attribute__ ((constructor))
> +do_init (void)
> +{
> +  pthread_t t;
> +  if (pthread_create (&t, NULL, worker, NULL) != 0)
> +    abort ();
> +  /* Blocks until worker has completed its __cxa_thread_atexit_impl
> +     call; under the pre-fix locking model that call deadlocks on
> +     dl_load_lock held by the dlopen caller running this ctor.  */
> +  if (pthread_join (t, NULL) != 0)
> +    abort ();
> +}
> 


-- 
PD Dr. Andreas K. Hüttel
[email protected]
Gentoo Linux developer 
(council, comrel, toolchain, base-system, perl, libreoffice)
https://wiki.gentoo.org/wiki/User:Dilfridge