Re: [PATCH] Bug fix: Warning for unsupported DF_1_* flags when LD_DEBUG=files.

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Nov 28, 2012 at 8:56 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> H.J. and I both agreed that the dynamic linker should assert when
> it saw a DF_1_* flag that it couldn't support, and that is exactly
> what went into master with commit 7e1be741255c40a2d71ea7c888eff39a3aaa32e9.
> Unfortunately it turns out that we can't be quite that restrictive.
>
> We received feedback that the additional assert had broken Valgrind.
> It turns out some userspace applications have always been setting DF_1_*
> flags we don't support; binutils sets them, and glibc ignores them.
>
> As an example Valgrind's build uses:
> PRELOAD_LDFLAGS_COMMON_LINUX  = -nodefaultlibs -shared -Wl,-z,interpose,-z,initfirst
>
> Therefore we can't assert on unsupported flags or we risk breaking
> previously working programs. This needs to be fixed ASAP before we
> release 2.17 or there will be problems running Valgrind and other
> applications.
>
> The following patch converts the assertion into a warning that can
> be seen when running the linker with LD_DEBUG=files or LD_DEBUG=all.
> At the very least we'll be providing diagnostics for unsupported flags.
>
> With the patch applied you can do the following:
>
> * Build a DSO with DF_1_INTERPOSE set.
>
>   gcc -shared -fPIC -o libfoo.so foo.c -Wl,-z,interpose -Wl,--soname=libfoo.so
>
> * Run an application with LD_DEBUG=files or LD_DEBUG=all and LD_PRELOAD=./libfoo.so
>
>       8402:     file=/bin/ls [0];  generating link map
>       8402:       dynamic: 0x000000000061ae08  base: 0x0000000000000000   size: 0x000000000021c280
>       8402:         entry: 0x00000000004026e0  phdr: 0x0000000000400040  phnum:                  9
>       8402:
>       8402:
>       8402:     file=./libfoo.so [0];  needed by /bin/ls [0]
>       8402:     file=./libfoo.so [0];  generating link map
>       8402:
>       8402:     WARNING: Unsupported flag value(s) of 0x400 in DT_FLAGS_1.
>       8402:       dynamic: 0x00007fe1ca2cde20  base: 0x00007fe1ca0cd000   size: 0x0000000000201020
>       8402:         entry: 0x00007fe1ca0cd4e0  phdr: 0x00007fe1ca0cd040  phnum:                  7
>       ...
>
> * Receive a `WARNING: ...' as the DSO is loaded
>
>   The printed value is the OR of all the unsupported flags detected in DT_FLAGS_1
>   for the DSO.
>
>   In this case just:
>   elf.h:#define DF_1_INTERPOSE    0x00000400      /* Object is used to interpose.  */
>
> It is safe to use _dl_debug_printf in all of the callers of this code.
> The most critical caller is the early relocation in rtld, but that sets
> RTLD_BOOTSTRAP and disables the DT_FLAGS_1 parsing at compile time.
>
> No regressions on x86-64.
>
> I'll check this in on Friday if nobody objects to the warning message or comments.
>
> 2012-11-28  Carlos O'Donell  <carlos@systemhalted.org>
>
>         * elf/get-dynamic-info.h (elf_get_dynamic_info): Warn
>         for unsupported DF_1_* bits when DL_DEBUG_FILES is set.
>
> diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h
> index 9e018de..026f246 100644
> --- a/elf/get-dynamic-info.h
> +++ b/elf/get-dynamic-info.h
> @@ -151,8 +151,16 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>      {
>        l->l_flags_1 = info[VERSYMIDX (DT_FLAGS_1)]->d_un.d_val;
>
> -      /* Only DT_1_SUPPORTED_MASK bits are allowed.  */
> -      assert ((l->l_flags_1 & ~DT_1_SUPPORTED_MASK) == 0);
> +      /* Only DT_1_SUPPORTED_MASK bits are supported, and we would like
> +        to assert this, but we can't. Users have been setting
> +        unsupported DF_1_* flags for a long time and glibc has ignored
> +        them. Therefore to avoid breaking existing applications the
> +        best we can do is add a warning during debugging with the
> +        intent of notifying the user of the problem.  */
> +      if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_FILES, 0)
> +         && l->l_flags_1 & ~DT_1_SUPPORTED_MASK)
> +       _dl_debug_printf ("\nWARNING: Unsupported flag value(s) of 0x%x in DT_FLAGS_1.\n",
> +                         l->l_flags_1 & ~DT_1_SUPPORTED_MASK);
>
>        if (l->l_flags_1 & DF_1_NOW)
>         info[DT_BIND_NOW] = info[VERSYMIDX (DT_FLAGS_1)];
> ---
>

Thanks. It looks good to me.


-- 
H.J.