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

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

On Tue, Nov 13, 2012 at 9:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> I can work with sh and m68k maintainers on a solution. It shouldn't be
>> too hard.
>
> Here is the updated patch.

I have some questions and some nits.

> --
> H.J.
> ---2012-11-13  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.
>
> 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..51356a6 100644
> --- a/elf/dl-runtime.c
> +++ b/elf/dl-runtime.c

Update copyright year and merge.

> @@ -164,6 +164,25 @@ _dl_profile_fixup (
>  {
>    void (*mcount_fct) (ElfW(Addr), ElfW(Addr)) = INTUSE(_dl_mcount);
>
> +  if (l->l_reloc_result == NULL)
> +    {
> +      /* Resolve an IRELATIVE relocation in another DSO may reference a

s/Resolve/Resolving/g

> +        function defined in libc.so, which leads to l_reloc_result == NULL.
> +        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.  */

This still doesn't make sense to me, but I see you've posted a new email with
a more detailed description.

> +      *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];
> diff --git a/elf/tst-audit8.c b/elf/tst-audit8.c
> new file mode 100644
> index 0000000..63656b4
> --- /dev/null
> +++ b/elf/tst-audit8.c
> @@ -0,0 +1 @@
> +#include "../io/pwd.c"
> --
> 1.7.11.7

Cheers,
Carlos.