Re: [PATCH] ld.so: Examine GLRO to detect inactive loader [BZ #20204]

[Carlos O'Donell <carlos at redhat dot com>]

On 12/18/2017 11:06 AM, Florian Weimer wrote:
> On 12/18/2017 07:25 PM, Carlos O'Donell wrote:
> 
>> I looked into trying to write a test case for this, but it's not trivial.
> 
> There is an existing test case, it's dlfcn/tststatic2, I think.  It calls dlopen etc. from an inner DSO, so it needs the dlfcn hooks.
> 
> In both directions, it is self-asserting: non-static dlopen will never call the hook functions, so a wrong rtld_active result in a fully dynamic application will cause a null pointer dereference.  Similarly, failure to correctly detect an incative ld.so in a static dlopen situation will lead to a non-working dynamic linker.

That's a good point.

I was more focused on asserting the changes you made were being used at all,
and not some other code path. However, functionally, this is no different
from testing the intended external behaviour. So it does look like tststatic2
(with the inner DSO calling dlopen also) should cover this.

>>> +/* Return true if the ld.so copy in this namespace is actually active
>>> +   and working.  If false, the dl_open/dlfcn hooks have to be used to
>>> +   call into the outer dynamic linker (which happens after static
>>> +   dlopen).  */
>>> +#ifdef SHARED
>>> +static inline bool
>>> +rtld_active (void)
>>> +{
>>> +  /* The default-initialized variable does not have a non-zero
>>> +     dl_init_all_dirs member, so this allows us to recognize an
>>> +     initialized and active ld.so copy.  */
>>> +  return GLRO(dl_init_all_dirs) != NULL;
>>
>> We need to add a comment to the definition of  GLRO(dl_init_all_dirs) that
>> it is being used as the initialization marker, then at assignment in rtld.c
>> we need a similar comment. This ties the definition, assignment, and usage
>> together in a meaningful way.
> 
> Sure, makes sense.
> 
> I'm testing the attached patch now.  It also changes the hook check
> used for libio vtable compatibility across multiple namespaces.  I'll
> commit this if testing passes.  (We have some minimal test coverage
> for the libio vtables check, too.)
Your v2 patch looks perfect. Thanks for the additional comments.

-- 
Cheers,
Carlos.