[PATCHv2] Increase robustness of internal dlopen() by using RTLD_NOW [BZ #22766]

[Tulio Magno Quites Machado Filho]

Carlos O'Donell <carlos@redhat.com> writes:

> On 04/25/2018 03:26 PM, Tulio Magno Quites Machado Filho wrote:
>> 
>> Prevent random runtime crashes due to missing symbols caused by mixed
>> libnss_* versions.
>> 
>> 2018-04-25  Tulio Magno Quites Machado Filho  <tuliom@linux.ibm.com>
>> 
>> 	[BZ #22766]
>> 	* include/dlfcn.h [__libc_dl_open]: Replace RTLD_LAZY with RTLD_NOW.
>> 	* sysdeps/gnu/unwind-resume.c (__lib_gcc_s_init): Replace
>> 	__libc_dlopen_mode() using RTLD_NOW with __libc_dlopen.
>> 	* sysdeps/nptl/unwind-forcedunwind.c: Likewise.
>> 
>> Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
>
> OK with the changes below.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Ack.

>> diff --git a/include/dlfcn.h b/include/dlfcn.h
>> index 12ef913e19..558c9bfe17 100644
>> --- a/include/dlfcn.h
>> +++ b/include/dlfcn.h
>> @@ -31,8 +31,13 @@ extern char **__libc_argv attribute_hidden;
>>  
>>  /* Now define the internal interfaces.  */
>>  
>> +/* Use RTLD_NOW here because:
>> +   1. For consistency between __libgcc_s_init and pthread_cancel_init;
>> +   2. It allows libnss_* to provide a fallback code at dlopen() time in order
>> +      to prevent missing symbols caused by mixed glibc versions.  Using
>> +      RTLD_LAZY here could cause random failures at runtime.  */
>
> Suggest:
>
> /* Use RTLD_NOW here because:
>    1. In pthread_cancel_init we want to use RTLD_NOW to reduce the stack usage
>       of future cancellation operations, particularly when the target thread
>       is running with a small stack. Likewise for consistency we do the same
>       thing in __libgcc_s_init. RTLD_NOW will rarely make a difference for
>       __libgcc_s_init because unwinding is already in progress, so libgcc_s.so
>       has already been loaded if its unwinder is used (Bug 22636).
>    2. It allows us to provide robust fallback code at dlopen time for incorrectly 
>       configured systems that mix old libnss_* modules with newly installed
>       libraries e.g. old libnss_nis.so.2 with new libnsl.so.1.  Using RTLD_LAZY
>       here causes a failure at the time the symbol is called and at that point
>       it is much harder to safely return an error (Bug 22766).
>
>    The use of RTLD_NOW also impacts gconv module loading, backtracing (where the
>    unwinder form libgcc_s.so is used), and IDNA functions (which load libidn), all
>    of which load their respective DSOs on demand, and so should not impact program
>    startup.  That is to say that the DSOs are loaded as part of an API call and
>    therefore we will be calling that family of API functions shortly so RTLD_NOW or
>    RTLD_LAZY is not a big difference in performance, but RTLD_NOW has better error
>    handling semantics for the library.  */

Much better!  :-D

>>  #define __libc_dlopen(name) \
>> -  __libc_dlopen_mode (name, RTLD_LAZY | __RTLD_DLOPEN)
>> +  __libc_dlopen_mode (name, RTLD_NOW | __RTLD_DLOPEN)
>>  extern void *__libc_dlopen_mode  (const char *__name, int __mode);
>>  extern void *__libc_dlsym   (void *__map, const char *__name);
>>  extern void *__libc_dlvsym (void *map, const char *name, const char *version);
>> diff --git a/sysdeps/gnu/unwind-resume.c b/sysdeps/gnu/unwind-resume.c
>> index 7f9a1bf2c7..72a04c7969 100644
>> --- a/sysdeps/gnu/unwind-resume.c
>> +++ b/sysdeps/gnu/unwind-resume.c
>> @@ -39,7 +39,7 @@ __libgcc_s_init (void)
>>       RTLD_NOW will rarely make a difference here because unwinding is
>>       already in progress, so libgcc_s.so has already been loaded if
>>       its unwinder is used.  */
>
> ^^^ This should be adjusted to:
>
> /* See include/dlfcn.h. Use of __libc_dlopen requires RTLD_NOW.  */

Fixed.

>> -  handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN);
>> +  handle = __libc_dlopen (LIBGCC_S_SO);
>>  
>>    if (handle == NULL
>>        || (resume = __libc_dlsym (handle, "_Unwind_Resume")) == NULL
>> diff --git a/sysdeps/nptl/unwind-forcedunwind.c b/sysdeps/nptl/unwind-forcedunwind.c
>> index 67b8e74b53..dbd8d66cda 100644
>> --- a/sysdeps/nptl/unwind-forcedunwind.c
>> +++ b/sysdeps/nptl/unwind-forcedunwind.c
>> @@ -49,7 +49,8 @@ pthread_cancel_init (void)
>>        return;
>>      }
>>  
>> -  handle = __libc_dlopen_mode (LIBGCC_S_SO, RTLD_NOW | __RTLD_DLOPEN);
>> +  /* __libgcc_s_init depends on RTLD_NOW being used here.  */
>
> Suggest:
>
> /* See include/dlfcn.h. Use of __libc_dlopen requires RTLD_NOW.  */

Fixed.

I'm pushing it.

Thanks!

-- 
Tulio Magno