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] Fix for BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice


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.


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