This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]