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

Artem Proskurnev <[email protected]>
Newsgroups gmane.comp.lib.glibc.alpha
Message-ID <[email protected]>
>> diff --git a/elf/dl-init.c b/elf/dl-init.c
>> index bd85bacdc1..250f7cc175 100644
>> --- a/elf/dl-init.c
>> +++ b/elf/dl-init.c
>> @@ -21,27 +21,24 @@
>>   #include <ldsodefs.h>
>>   #include <elf-initfini.h>
>>   
>> +#if !IS_IN (rtld)
>> +# include <stdint.h>
>> +/* pthread_once_t is int; we use the l_init_once field in struct
>> +   link_map (also int, zero from calloc matches PTHREAD_ONCE_INIT).  */
>> +extern int __pthread_once (int *once_control, void (*init_routine) (void));
>> +
>> +/* Argument for call_init_once_cb.  Set by call_init before calling
>> +   __pthread_once, which invokes the callback synchronously (either
>> +   immediately, or after blocking).  */
>> +static __thread struct link_map *call_init_once_arg;
>> +
>> +static void call_init_once_cb (void);
>> +#endif
> I don't think this change is effective because elf/dl-init.c is not
> built at all for dynamically linked libc.so, only for libc.a and ld.so.
>
> I think a fallible pthread_once variant with a closure pointer would be
> generally useful.  It could be used here to avoid the thread-local
> variable.
>
> We cannot use ELF TLS in ld.so, but we can access fields in struct
> pthread.  See rtld_catch.
>
>> diff --git a/include/link.h b/include/link.h
>> index 8f851d2212..e539535a44 100644
>> --- a/include/link.h
>> +++ b/include/link.h
>> @@ -346,6 +346,12 @@ struct link_map
>>       size_t l_relro_size;
>>   
>>       unsigned long long int l_serial;
>> +
>> +    /* Per-DSO once-initialization control for constructor execution.
>> +       Used as pthread_once_t (PTHREAD_ONCE_INIT == 0, matching
>> +       calloc).  Only accessed from the libc.so build of dl-init.c
>> +       (IS_IN (rtld) path is single-threaded).  */
>> +    int l_init_once;
>>     };
> I don't think this will build on Hurd.  It doesn't have futexes, so
> pthread_once_t contains a spinlock and isn't just an int.
>
> We'll need additional tests that show the synchronization is working as
> expected.
>
> My concern regarding the removal of a lock that applications could
> previously rely on remains.
>
> Thanks,
> Florian
>
I got it. Indeed I did it wrong when I used IS_IN (rtld). This is a 
significantly more complex patch. But I will try to implement it.

Thank you very much for your response!

Artem
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.