Re: PATCH: dl_iterate_phdr()

[Rich Felker <dalias@aerifal.cx>]

On Thu, Oct 11, 2012 at 09:29:28AM -0500, Alex Caudill wrote:
> Hi!
> 
> With a little help on IRC I hacked together a basic version of
> dl_iterate_phdr(), which I dropped into src/ldso/dynlink.c in my tree.
> This introduces a new include file: <link.h>
> 
> Line #9 of dl_iterate_phdr.c throws a warning that I'm not sure how to silence.
> 
> I've attached both, and a test program. No copyright.
> 
> Thanks!

> int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
> {   
>     struct dso *current;
>     Ehdr *ehdr;
>     struct dl_phdr_info info;
>     int ret = 0;
>     for(current = head; current; current = current->next) {
>         ehdr = (Ehdr *)current->map;
>         info.dlpi_addr = (ehdr->e_type == ET_EXEC) ? 0 : current->map;

This is wrong; it should be current->base. See the man page:

    The dlpi_addr field indicates the base address of the shared
    object (i.e., the difference between the virtual memory address of
    the shared object and the offset of that object in the file from
    which it was loaded).

>         info.dlpi_name = current->shortname;

This too:

    The dlpi_name field is a null-terminated string giving the
    pathname from which the shared object was loaded.

i.e. it needs to be using name, not shortname. The only place
shortname is to use is to match libraries loaded by name without a
pathname provided. It's NULL for libraries explicitly loaded by
pathname.

>         info.dlpi_phdr = (Phdr *)(current->map + ehdr->e_phoff);

It might make more sense to save the phdr address during library
loading instead of reading the Ehdr again, especially since there's no
fundamental guarantee the Ehdr is available anymore. (It could be in
an unmapped page at the beginning of the library prior to the mapped
portion. This usage is ugly and may not even work with musl right now,
but there's no reason to add more gratuitous reasons it should fail.)

>         info.dlpi_phnum = ehdr->e_phnum;
> 
>         ret = (callback)(&info, sizeof (info), data);
>         if (ret != 0)
>             break;
>     }
>     return ret;
> }

> #ifndef LINK_H
> #define LINK_H

Missing underscore. Not absolutely essential since this is a
nonstandard header, but it should be there.

> 
> #include <elf.h>
> 
> #define __NEED_size_t
> #include <bits/alltypes.h>
> 
> #ifdef _LP64

Should use UINTPTR_MAX.

> #define ElfW(type)      Elf64_ ## type
> #else   
> #define ElfW(type)      Elf32_ ## type
> #endif
> 
> struct dl_phdr_info {
>     ElfW(Addr)              dlpi_addr;  
>     const char              *dlpi_name;
>     const ElfW(Phdr)        *dlpi_phdr;
>     ElfW(Half)              dlpi_phnum;
> };

We need to provide the "ElfW" macro since calling code might use it,
but I'd rather it not be used internally. Just use the correct types
(e.g. uintptr_t or size_t, etc.).

Otherwise, looks okay!

Rich