This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2] dlopen: Do not block signals
On 12/12/19 12:01 PM, Adhemerval Zanella wrote:
>
>
> On 05/12/2019 12:20, Florian Weimer wrote:
>> Blocking signals causes issues with certain anti-malware solutions
>> which rely on an unblocked SIGSYS signal for system calls they
>> intercept.
>>
>> This reverts commit a2e8aa0d9ea648068d8be52dd7b15f1b6a008e23
>> ("Block signals during the initial part of dlopen") and adds
>> comments related to async signal safety to active_nodelete and
>> its caller.
>>
>> Note that this does not make lazy binding async-signal-safe with regards
>> to dlopen. It merely avoids introducing new async-signal-safety hazards
>> as part of the NODELETE changes.
>
> LGTM, thanks.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
LGTM too.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>> ---
>> elf/dl-open.c | 37 +++++++++++--------------------------
>> 1 file changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/elf/dl-open.c b/elf/dl-open.c
>> index c23341be58..5a1c5b5326 100644
>> --- a/elf/dl-open.c
>> +++ b/elf/dl-open.c
>> @@ -34,7 +34,6 @@
>> #include <atomic.h>
>> #include <libc-internal.h>
>> #include <array_length.h>
>> -#include <internal-signals.h>
>>
>> #include <dl-dst.h>
>> #include <dl-prop.h>
>> @@ -53,10 +52,6 @@ struct dl_open_args
>> /* Namespace ID. */
>> Lmid_t nsid;
>>
>> - /* Original signal mask. Used for unblocking signal handlers before
>> - running ELF constructors. */
>> - sigset_t original_signal_mask;
>> -
>> /* Original value of _ns_global_scope_pending_adds. Set by
>> dl_open_worker. Only valid if nsid is a real namespace
>> (non-negative). */
>
> Ok.
>
>> @@ -446,6 +441,9 @@ activate_nodelete (struct link_map *new)
>> _dl_debug_printf ("activating NODELETE for %s [%lu]\n",
>> l->l_name, l->l_ns);
>>
>> + /* The flag can already be true at this point, e.g. a signal
>> + handler may have triggered lazy binding and set NODELETE
>> + status immediately. */
>> l->l_nodelete_active = true;
>>
>> /* This is just a debugging aid, to indicate that
>
> Ok.
>
>> @@ -520,16 +518,12 @@ dl_open_worker (void *a)
>> if (new == NULL)
>> {
>> assert (mode & RTLD_NOLOAD);
>> - __libc_signal_restore_set (&args->original_signal_mask);
>> return;
>> }
>>
>> if (__glibc_unlikely (mode & __RTLD_SPROF))
>> - {
>> - /* This happens only if we load a DSO for 'sprof'. */
>> - __libc_signal_restore_set (&args->original_signal_mask);
>> - return;
>> - }
>> + /* This happens only if we load a DSO for 'sprof'. */
>> + return;
>>
>> /* This object is directly loaded. */
>> ++new->l_direct_opencount;
>
> Ok.
>
>> @@ -565,7 +559,6 @@ dl_open_worker (void *a)
>>
>> assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT);
>>
>> - __libc_signal_restore_set (&args->original_signal_mask);
>> return;
>> }
>>
>
> Ok.
>
>> @@ -712,6 +705,12 @@ dl_open_worker (void *a)
>> All memory allocations for new objects must have happened
>> before. */
>>
>> + /* Finalize the NODELETE status first. This comes before
>> + update_scopes, so that lazy binding will not see pending NODELETE
>> + state for newly loaded objects. There is a compiler barrier in
>> + update_scopes which ensures that the changes from
>> + activate_nodelete are visible before new objects show up in the
>> + local scope. */
>> activate_nodelete (new);
>>
>> /* Second stage after resize_scopes: Actually perform the scope
>
> Ok.
>
>> @@ -745,10 +744,6 @@ dl_open_worker (void *a)
>> if (mode & RTLD_GLOBAL)
>> add_to_global_resize (new);
>>
>> - /* Unblock signals. Data structures are now consistent, and
>> - application code may run. */
>> - __libc_signal_restore_set (&args->original_signal_mask);
>> -
>> /* Run the initializer functions of new objects. Temporarily
>> disable the exception handler, so that lazy binding failures are
>> fatal. */
>
> Ok.
>
>> @@ -838,10 +833,6 @@ no more namespaces available for dlmopen()"));
>> args.argv = argv;
>> args.env = env;
>>
>> - /* Recursive lazy binding during manipulation of the dynamic loader
>> - structures may result in incorrect behavior. */
>> - __libc_signal_block_all (&args.original_signal_mask);
>> -
>> struct dl_exception exception;
>> int errcode = _dl_catch_exception (&exception, dl_open_worker, &args);
>>
>
> Ok.
>
>> @@ -882,16 +873,10 @@ no more namespaces available for dlmopen()"));
>>
>> _dl_close_worker (args.map, true);
>>
>> - /* Restore the signal mask. In the success case, this
>> - happens inside dl_open_worker. */
>> - __libc_signal_restore_set (&args.original_signal_mask);
>> -
>> /* All l_nodelete_pending objects should have been deleted
>> at this point, which is why it is not necessary to reset
>> the flag here. */
>> }
>> - else
>> - __libc_signal_restore_set (&args.original_signal_mask);
>>
>> assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);
>>
>>
>
> Ok.
>
--
Cheers,
Carlos.