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

[Florian Weimer]

> 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));
> +

I believe this results in a data race on l_init_called.  In a
multi-threaded application, the same ELF constructor may execute with
itself in parallel on different threads, or dlopen may return before the
ELF constructor has completed running in another thread.  Some form of
synchronization is required to deal with these scenarios, perhaps using
pthread_once or a condition variable.

We also likely cannot remove a lock like this for old applications which
may depend on its existence.  We can deal with this with a different
mechanism.

Thanks,
Florian