Re: [PATCH] BZ #14831: Segfault in _dl_profile_fixup with IRELATIVEand LD_AUDIT

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Wed, Nov 14, 2012 at 5:13 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Nov 14, 2012 at 1:46 PM, Carlos O'Donell
> <carlos@systemhalted.org> wrote:
>> On Wed, Nov 14, 2012 at 1:59 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> So the problem is that the objects are loaded in reverse order (for copy reloc
>>>> purposes) and therefore we load libm.so *before* libc.so, and thus
>>>> l_reloc_result hasn't been allocated for libc.so yet, and thus trying to
>>>> resolve a function in libc.so will fail if we have profiling enabled because
>>>> that entry in the link_map is not yet allocated (because we haven't called
>>>> _dl_relocate_object for libc.so yet).
>>>>
>>>> We are going to eventually allocate l_reloc_result anyway. Could we instead
>>>> refactor the l_reloc_result allocation out of _dl_relocate_object and into
>>>> the loop in rtld.c which loads all the objects?
>>>>
>>>
>>> It isn't libc.so's l_reloc_result hasn't been allocated.  It is libm.so's
>>> l_reloc_result hasn't been allocated since we have
>>
>> Yes, sorry, you're right.
>>
>>> ELF_DYNAMIC_RELOCATE (l, lazy, consider_profiling, skip_ifunc);
>>> ...
>>> l->l_reloc_result = calloc (sizeof (l->l_reloc_result[0]),
>>>
>>> Allocate l_reloc_result before ELF_DYNAMIC_RELOCATE causes
>>> tst-audit2 failure:
>>>
>>> {abcdef72, d8675309} != {d8675309, abcdef72}
>>>
>>> since wrong calloc is called.  How about this updated comments?
>>
>> Could you please look more closely at why tst-audit2 fails?
>>
>> I see that tst-audit2 simply wants to know if the static TLS variables
>> have been initialized *before* calloc ever gets called.
>>
>> If we move the first call to calloc *before* ELF_DYNAMIC_RELOCATE, how
>> does that effect this? I would have expected the static TLS vars to be
>> initialized well before this point.
>>
>> Cheers,
>> Carlos.
>
> The calloc check is intentional:
>
> http://www.sourceware.org/ml/libc-alpha/2005-09/msg00075.html
> http://sourceware.org/ml/libc-alpha/2006-03/msg00138.html

Darn, yes, I see the problem.

>From ELF_DYNAMIC_RELOCATE we call elf_machine_rela, which calls
CHECK_STATIC_TLS, which eventually allocates the static TLS *before*
the first call to any malloc-esque hooks.

The only way to fix that would be to refactor the TLS allocation
to a point before the calloc and that's just a non-starter.

OK, I accept the patch on three conditions:

(1) File a BZ to fix the fact that we can't audit IFUNC resolver functions
    because of this ordering issue. Personally I find it useful to be able
    to audit all function calls in my code.

(2) Add the BZ as a project in the Generic TODO in the wiki.

(3) Amend comment to include BZ# and a couple more grammar tweaks.

@@ -166,12 +166,11 @@ _dl_profile_fixup (

   if (l->l_reloc_result == NULL)
     {
-      /* Resolve an IRELATIVE relocation in another DSO may reference a
-        function defined in libc.so before l_reloc_result is allocated.
-        For example, __get_cpu_features in libc.so is called to resolve
-        R_X86_64_IRELATIVE relocations in x86-64 libm.so.  Skip audit and
-        resolve the function in this case.  It is OK since we aren't
-        supposed to audit IRELATIVE relocations.  */

+      /* ELF_DYNAMIC_RELOCATE is called before l_reloc_result is allocated.

s/ELF/BZ#XXXX: ELF/g

+        We will get here if ELF_DYNAMIC_RELOCATE calls a resolver function
+        to resolve IRELATIVE relocation.  For example, resolver in x86-64

s/resolve/resolve an/g

s/./, and that resolver calls a function that is not yet resolved (lazy)./g

s/resolver/the resolver/g

+        libm.so calls __get_cpu_features defined in libc.so.  Skip audit
+        and resolve the external function in this case.  */
       *framesizep = -1;
       return _dl_fixup (
 # ifdef ELF_MACHINE_RUNTIME_FIXUP_ARGS

OK with those changes.

Cheers,
Carlos.