This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] BZ #14831: Segfault in _dl_profile_fixup with IRELATIVEand LD_AUDIT
On Wed, Nov 14, 2012 at 3:07 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> 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.
>
This is the patch I checked in. Andreas, Kaz, Thomas, could you
please define ELF_MACHINE_RUNTIME_FIXUP_PARAMS for
m68k and sh since they are the only targets which define
ELF_MACHINE_RUNTIME_FIXUP_ARGS. OK to backport
it to 2.16 with m68k/sh ELF_MACHINE_RUNTIME_FIXUP_PARAMS
changes?
Thanks.
--
H.J.
----diff --git a/ChangeLog b/ChangeLog
index f5a3645..8014b6c 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2012-11-14 H.J. Lu <hongjiu.lu@intel.com>
+
+ [BZ #14831]
+ * elf/Makefile (tests): Add tst-audit8.
+ ($(objpfx)tst-audit8): Also depend on $(common-objpfx)math/libm.so.
+ ($(objpfx)tst-audit8.out): New target.
+ (tst-audit8-ENV): New variable.
+ * elf/dl-runtime.c (_dl_profile_fixup): Call _dl_fixup to skip
+ audit if l_reloc_result is NULL.
+ (ELF_MACHINE_RUNTIME_FIXUP_PARAMS): Issue an error if it isn't
+ defined and ELF_MACHINE_RUNTIME_FIXUP_ARGS is defined.
+ * elf/tst-audit8.c: New file.
+
2012-11-14 Marcus Shawcroft <marcus.shawcroft@linaro.org>
* io/Makefile (CFLAGS-open.c, CFLAGS-open64.c): Define.
diff --git a/NEWS b/NEWS
index 3ffdb80..0e76063 100644
--- a/NEWS
+++ b/NEWS
@@ -19,7 +19,7 @@ Version 2.17
14562, 14568, 14576, 14579, 14583, 14587, 14595, 14602, 14610, 14621,
14638, 14645, 14648, 14652, 14660, 14661, 14669, 14683, 14694, 14716,
14743, 14767, 14783, 14784, 14785, 14793, 14796, 14797, 14801, 14805,
- 14807, 14809, 14811, 14815, 14821, 14824, 14838.
+ 14807, 14809, 14811, 14815, 14821, 14824, 14831, 14838.
* Port to ARM AArch64 contributed by Linaro.
diff --git a/elf/Makefile b/elf/Makefile
index c2f0e20..7e5c9c8 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -142,7 +142,7 @@ tests += loadtest restest1 preloadtest loadfail
multiload origtest resolvfail \
tst-dlmodcount tst-dlopenrpath tst-deep1 \
tst-dlmopen1 tst-dlmopen2 tst-dlmopen3 \
unload3 unload4 unload5 unload6 unload7 unload8 tst-global1 order2 \
- tst-audit1 tst-audit2 \
+ tst-audit1 tst-audit2 tst-audit8 \
tst-stackguard1 tst-addr1 tst-thrlock \
tst-unique1 tst-unique2 tst-unique3 tst-unique4 \
tst-initorder tst-initorder2 tst-relsort1
@@ -1020,6 +1020,10 @@ $(objpfx)tst-audit7: $(objpfx)tst-auditmod7a.so
$(objpfx)tst-audit7.out: $(objpfx)tst-auditmod7b.so
tst-audit7-ENV = LD_AUDIT=$(objpfx)tst-auditmod7b.so
+$(objpfx)tst-audit8: $(common-objpfx)math/libm.so
+$(objpfx)tst-audit8.out: $(objpfx)tst-auditmod1.so
+tst-audit8-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
+
$(objpfx)tst-global1: $(libdl)
$(objpfx)tst-global1.out: $(objpfx)testobj6.so $(objpfx)testobj2.so
diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
index 2e02a21..7a3bc9e 100644
--- a/elf/dl-runtime.c
+++ b/elf/dl-runtime.c
@@ -1,5 +1,5 @@
/* On-demand PLT fixup for shared objects.
- Copyright (C) 1995-2009, 2010, 2011 Free Software Foundation, Inc.
+ Copyright (C) 1995-2012 Free Software Foundation, Inc.
This file is part of the GNU C Library.
The GNU C Library is free software; you can redistribute it and/or
@@ -164,6 +164,26 @@ _dl_profile_fixup (
{
void (*mcount_fct) (ElfW(Addr), ElfW(Addr)) = INTUSE(_dl_mcount);
+ if (l->l_reloc_result == NULL)
+ {
+ /* BZ #14843: ELF_DYNAMIC_RELOCATE is called before l_reloc_result
+ is allocated. We will get here if ELF_DYNAMIC_RELOCATE calls a
+ resolver function to resolve an IRELATIVE relocation and that
+ resolver calls a function that is not yet resolved (lazy). For
+ example, the resolver in x86-64 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
+# ifndef ELF_MACHINE_RUNTIME_FIXUP_PARAMS
+# error Please define ELF_MACHINE_RUNTIME_FIXUP_PARAMS.
+# endif
+ ELF_MACHINE_RUNTIME_FIXUP_PARAMS,
+# endif
+ l, reloc_arg);
+ }
+
/* This is the address in the array where we store the result of previous
relocations. */
struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index];