Re: ldso : dladdr support

[Rich Felker <dalias@aerifal.cx>]

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..

Rich