[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <50324A60.7040206@gmail.com>
Date: Mon, 20 Aug 2012 16:32:00 +0200
From: musl <b.brezillon.musl@...il.com>
To: musl@...ts.openwall.com
Subject: Re: ldso : dladdr support
I missed a bug in my previous patch :
in find_sym func precomptab was always set to sysv_precomp.
On 20/08/2012 14:55, musl wrote:
> Hi,
>
> This patch adds:
> * decode_vec2 implemented for high DT_x values.
> * find_closest_sym code integrated in do_dladdr function.
> * other fixes you asked in your previous mail.
>
> Regards,
>
> Boris.
>
>
> On 20/08/2012 04:06, Rich Felker wrote:
>> On Sun, Aug 19, 2012 at 06:42:30PM +0200, musl wrote:
>>> Hi,
>>>
>>> This patch fixes a bug in dladdr: sym var was not incremented across gnu hash chain iteration).
>>> I also reworked the dladdr implem to share more code between sysv and gnu hash.
>>> I still haven't found a better way to get the symbol table size. Do you?
>>>
>>> This patch uses the new decode_vec function, but as I told you in my
>>> previous mail, I'm not sure this the way to go.
>>> Could you tell me what you think?
>> Yeah, I'm not really happy with it either. Trying to think of
>> something better...
>>
>>> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
>>> index f55c6f1..bf1ec6b 100644
>>> --- a/src/ldso/dynlink.c
>>> +++ b/src/ldso/dynlink.c
>>> @@ -1,3 +1,4 @@
>>> +#define _GNU_SOURCE
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> @@ -28,12 +29,14 @@ typedef Elf32_Phdr Phdr;
>>> typedef Elf32_Sym Sym;
>>> #define R_TYPE(x) ((x)&255)
>>> #define R_SYM(x) ((x)>>8)
>>> +#define ELF_ST_TYPE ELF32_ST_TYPE
>>> #else
>>> typedef Elf64_Ehdr Ehdr;
>>> typedef Elf64_Phdr Phdr;
>>> typedef Elf64_Sym Sym;
>>> #define R_TYPE(x) ((x)&0xffffffff)
>>> #define R_SYM(x) ((x)>>32)
>>> +#define ELF_ST_TYPE ELF64_ST_TYPE
>>> #endif
>> These definitions are actually the same. I would just
>> #define ST_TYPE(x) ((x)&15)
>>
>>> -static uint32_t hash(const char *s0)
>>> +static uint32_t sysv_hash(const char *s0)
>>> {
>>> const unsigned char *s = (void *)s0;
>>> uint_fast32_t h = 0;
>>> @@ -105,7 +117,16 @@ static uint32_t hash(const char *s0)
>>> return h & 0xfffffff;
>>> }
>>>
>>> -static Sym *lookup(const char *s, uint32_t h, struct dso *dso)
>>> +static uint32_t gnu_hash (const char *s0)
>>> +{
>>> + const unsigned char *s = (void *)s0;
>>> + uint_fast32_t h = 5381;
>>> + for (; *s; s++)
>>> + h = h*33 + *s;
>>> + return h & 0xffffffff;
>>> +}
>> The final &0xffffffff is a no-op. Note that the one in sysv_hash is
>> not a no-op; sysv_hash's result is 28 bits, not 32.
>>
>> Re-reading this code also raised another issue: I'm not entirely
>> convinced that 0 is not a possible hash value, which may invalidate
>> what I said before about using h==0 to indicate "not yet computed". Of
>> course, it may not matter; if one in 4 billion symbol names get their
>> hashes repeatedly recomputed rather than being reused, it's not going
>> to make any difference to overall performance...
>>
>>> /* Only trust user/env if kernel says we're not suid/sgid */
>>> - if ((aux[0]&0x7800)!=0x7800 || aux[AT_UID]!=aux[AT_EUID]
>>> - || aux[AT_GID]!=aux[AT_EGID] || aux[AT_SECURE]) {
>>> + if ((found&0x1e0)!=0x1e0 || aux[5]!=aux[6]
>>> + || aux[7]!=aux[8] || aux[9]) {
>>> env_path = 0;
>>> env_preload = 0;
>> Looking at this, I agree that the new decode_vec idea is not a good
>> direction. It's obfuscating the code badly.
>>
>> For now, how about leaving the old decode_vec alone and just adding a
>> new one with a different name for getting to "high" entries. I wonder
>> if it would be possible, rather than using a list of wanted entries,
>> to use a base/count rather than always working zero-based like
>> decode_vec does. This would allow the resulting indices to still
>> actually mean something so we don't wind up with magic numbers all
>> over the code..
> The decode_vec uses the first entry in 'a' to store the tags found in the given vector.
> If cnt is bigger than sizeof(size_t) * 8, there is a shift overflow.
> It's fine in the current use as you only test tag values < 32 (or don't use decode_vec to test it : AT_SYSINFO_EHDR).
> Should I take care of those cases in decode_vec2 (add a separate 'found' table argument)?
>> Rich
View attachment "dladdr-gnu-hash-v4-bis.patch" of type "text/x-patch" (7893 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
