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

[Carlos O'Donell]

On Wed, Nov 28, 2012 at 11:58 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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.

I've checked this into master a little early since this is a serious
regression, and I don't want to run out of time to do this on Friday.

Cheers,
Carlos.