[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <503233A8.8000604@gmail.com>
Date: Mon, 20 Aug 2012 14:55:04 +0200
From: musl <b.brezillon.musl@...il.com>
To: musl@...ts.openwall.com
Subject: Re: ldso : dladdr support
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.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.
