This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [patch] Fix for BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 12 Mar 2014 20:01:01 -0400
- Subject: Re: [patch] Fix for BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice
- Authentication-results: sourceware.org; auth=none
- References: <52D20FE1 dot 60102 at google dot com> <52D80FFC dot 60909 at redhat dot com> <CALoOobOXUbNraGsnoXE7XQnCF+J6uDFootHL2O8=OeysA1vCZA at mail dot gmail dot com> <53166951 dot 4090502 at redhat dot com> <CALoOobNzR6f1ZaOJ3Vn+R1VowkJ688y_pLb_oi58dZZdZZ_9CA at mail dot gmail dot com>
On 03/12/2014 07:47 PM, Paul Pluzhnikov wrote:
> On Tue, Mar 4, 2014 at 4:01 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>
>> On 01/17/2014 06:36 PM, Paul Pluzhnikov wrote:
>
>>> This patch simply sets correct type for loading of the executable from
>>> the start.
>>>
>>> Tested on Linux / x86_64 with no regressions.
>>
>> OK to checkin if you fix the minor nit and explain my
>> question in the middle of the patch.
>
> Thanks for the review!
>
> Committed after minor fixes.
>
>>> @@ -1075,7 +1076,7 @@ of this helper program; chances are you did not intend to run this program.\n\
>>> else
>>> {
>>> HP_TIMING_NOW (start);
>>> - _dl_map_object (NULL, rtld_progname, lt_library, 0,
>>> + _dl_map_object (NULL, rtld_progname, lt_executable, 0,
>>
>> OK, but I'd like you to tell me you verified that the lt_library here
>> was previously ignored and set to lt_executable by the code in
>> _dl_map_object_from_fd.
>
> This took longer than I expected, _dl_map_object_from_fd is long and
> complicated :-(
It always does when the reviewer asks icky questions like this ;-)
> AFAICT, _dl_map_object_from_fd used passed-in l_type once here:
>
> l = _dl_new_object (realname, name, l_type, loader, mode, nsid);
>
> that sets l->l_type = l_type and performs no conditional logic; so safe
> (since it was resetting l->l_type to l_executable before returning).
>
> It then used l->l_type once here (before resetting it to lt_executable):
>
> case PT_TLS:
> ...
> /* If not loading the initial set of shared libraries,
> check whether we should permit loading a TLS segment. */
> if (__builtin_expect (l->l_type == lt_library, 1)
> /* If GL(dl_tls_dtv_slotinfo_list) == NULL, then rtld.c did
> not set up TLS data structures, so don't use them now. */
> || __builtin_expect (GL(dl_tls_dtv_slotinfo_list) != NULL, 1))
> {
> /* Assign the next available module ID. */
> l->l_tls_modid = _dl_next_tls_modid ();
> break;
> }
>
> Here, I have changed the condition, and so the main executable no longer
> gets its l_tls_modid set to 1.
>
> ... which would be a problem, except that it is unconditionally set to 1
> soon thereafter by this code in rtld.c dl_main():
Agreed. I was only ever aware of the unconditional setting.
> case PT_TLS:
> if (ph->p_memsz > 0)
> {
> ...
> /* This image gets the ID one. */
> GL(dl_tls_max_dtv_idx) = main_map->l_tls_modid = 1;
> }
> break;
>
>
> Almost certainly the code in rtld.c did not expect the _dl_map_object_from_fd
> to have already set its l_tls_modid.
>
> So in the end, main_map->l_tls_modid still ends up as 1, and everything
> is happy. At least that's my story and I am sticking to it until proven
> wrong :-)
That's fine, what you described makes sense and roughly answers my question,
and leaves us with a more detailed trail in the mailing list for later
review if there is a problem.
Cheers,
Carlos.