PATCH: dl_iterate_phdr()

PATCH: dl_iterate_phdr()

[Alex Caudill]

[-- Attachment #1: Type: text/plain, Size: 329 bytes --]

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!

[-- Attachment #2: dl_iterate_phdr.c --]
[-- Type: text/x-csrc, Size: 632 bytes --]

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;
        info.dlpi_name = current->shortname;
        info.dlpi_phdr = (Phdr *)(current->map + ehdr->e_phoff);
        info.dlpi_phnum = ehdr->e_phnum;

        ret = (callback)(&info, sizeof (info), data);
        if (ret != 0)
            break;
    }
    return ret;
}

[-- Attachment #3: link.h --]
[-- Type: text/x-chdr, Size: 480 bytes --]

#ifndef LINK_H
#define LINK_H

#include <elf.h>

#define __NEED_size_t
#include <bits/alltypes.h>

#ifdef _LP64
#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;
};
 
int dl_iterate_phdr(int (*)(struct dl_phdr_info *, size_t, void *), void *);

#endif

[-- Attachment #4: dltest.c --]
[-- Type: text/x-csrc, Size: 528 bytes --]

#define _GNU_SOURCE
#include <link.h>
#include <stdlib.h>
#include <stdio.h>

static int
callback(struct dl_phdr_info *info, size_t size, void *data)
{
    int j;

   printf("name=%s (%d segments)\n", info->dlpi_name,
        info->dlpi_phnum);

   for (j = 0; j < info->dlpi_phnum; j++)
         printf("\t\t header %2d: address=%10p\n", j,
             (void *) (info->dlpi_addr + info->dlpi_phdr[j].p_vaddr));
    return 0;
}

int
main(int argc, char *argv[])
{
    dl_iterate_phdr(callback, NULL);

   exit(EXIT_SUCCESS);
}

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

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

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Thu, Oct 11, 2012 at 07:42:55PM -0400, Rich Felker wrote:
> Otherwise, looks okay!

Actually one more issue -- it needs to be made thread-safe. This would
just be a matter of obtaining a read-lock on the dynamic linker lock,
except that this lock should not be held during callbacks to
application code, which need not return in bounded time. Instead, I
think it would work to read the global "tail" while a read-lock is
held, then use a loop of the form:

	for (current=head; ; current=current->next) {
		...
		if (current == saved_tail) break;
	}

This will simply skip any new libraries that were not (fully) loaded
yet at the time of the call to dl_iterate_phdr.

Rich

Re: PATCH: dl_iterate_phdr()

[Alex Caudill]

[-- Attachment #1: Type: text/plain, Size: 3097 bytes --]

Thanks for the review! To clarify, I meant that the patch is public
domain. This version implements most of your changes and attempts
support for dlopen()'d libraries and vdso.

Some questions:

1.) I guarded link.h with _GNU_SOURCE or _BSD_SOURCE; I'm unclear on
whether this is necessary since it isn't a standard header, so please
let me know your preference.

2.) I'm unsure of how to replace ElfW(Phdr) in dl_phdr_info - or did
you mean that musl should internally use some opaque type for
dl_phdr_info? Also, should ElfW() go in elf.h?

3.) The attached dltest.c reveals a problem with dlopen()'d libraries
which seems to be related to the locking strategy (see output below).
If I don't take the lock and check for current == saved_tail, it
"fixes" the example. At the least, I think this reveals a flaw with
dlopen("foo", RTLD_NOW) - shouldn't it hold the lock until the dso
list has been updated?

This function is used by libunwind and (I think) libgcc_eh for C++
exception support, and it's possible that additional fields in
dl_phdr_info will be necessary in order for those to work unmodified
with musl. I'll look into this today and come up with more tests.
Solaris and FreeBSD, at least, have these appended to struct
dl_phdr_info:

unsigned long long int dlpi_adds;       /* total # of loads */
unsigned long long int dlpi_subs;       /* total # of unloads */

The dl_iterate_phdr() callback is passed a size arg, and the callback
is responsible for checking the size to ensure that the additional
fields are present. Don't shoot the messenger! :)

TEST OUTPUT:
musl:
name=./dltest (6 segments)
                 header  0: address= 0x8048034
                 header  1: address= 0x80480f4
                 header  2: address= 0x8048000
                 header  3: address= 0x8049538
                 header  4: address= 0x804954c
                 header  5: address=         0
name=/lib/ld-musl-i386.so.1 (4 segments)
                 header  0: address=0x28049000
                 header  1: address=0x280c7714
                 header  2: address=0x280c77d8
                 header  3: address=0x28049000

FreeBSD libc:
name=/usr/home/alex/dltest (8 segments)
                 header  0: address= 0x8048034
                 header  1: address= 0x8048134
                 header  2: address= 0x8048000
                 header  3: address= 0x80497d0
                 header  4: address= 0x80497e4
                 header  5: address= 0x804814c
                 header  6: address= 0x8048784
                 header  7: address=       0x0
name=libc.so.7 (6 segments)
                 header  0: address=0x2806b000
                 header  1: address=0x28195000
                 header  2: address=0x281976b4
                 header  3: address=0x28195000
                 header  4: address=0x28194b38
                 header  5: address=0x2806b000
name=/compat/linux/lib/libavl.so (4 segments)
                 header  0: address=0x281c3000
                 header  1: address=0x281c5738
                 header  2: address=0x281c5738
                 header  3: address=0x281c3000

[-- Attachment #2: dynlink.patch --]
[-- Type: application/octet-stream, Size: 3447 bytes --]

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index c3cb611..503ed0e 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <elf.h>
+#include <link.h>
 #include <setjmp.h>
 #include <pthread.h>
 #include <ctype.h>
@@ -57,6 +58,8 @@ struct dso {
 	size_t *dynv;
 	struct dso *next, *prev;
 
+        Phdr *phdr;
+        uint16_t phnum;
 	int refcnt;
 	Sym *syms;
 	uint32_t *hashtab;
@@ -324,6 +327,8 @@ static void *map_library(int fd, struct dso *dso)
 		eh->e_phoff = sizeof *eh;
 	}
 	ph = (void *)((char *)buf + eh->e_phoff);
+        dso->phdr = ph;
+        dso->phnum = (uint16_t)eh->e_phnum;
 	for (i=eh->e_phnum; i; i--, ph=(void *)((char *)ph+eh->e_phentsize)) {
 		if (ph->p_type == PT_DYNAMIC)
 			dyn = ph->p_vaddr;
@@ -815,19 +820,20 @@ void *__dynlink(int argc, char **argv)
 	lib->name = lib->shortname = "libc.so";
 	lib->global = 1;
 	ehdr = (void *)lib->base;
-	find_map_range((void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize, lib);
-	lib->dynv = (void *)(lib->base + find_dyn(
-		(void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize));
+        lib->phnum = (uint16_t)ehdr->e_phnum;
+        lib->phdr = (void *)(aux[AT_BASE]+ehdr->e_phoff);
+	find_map_range(lib->phdr, ehdr->e_phnum, ehdr->e_phentsize, lib);
+	lib->dynv = (void *)(lib->base + find_dyn(lib->phdr,
+                    ehdr->e_phnum, ehdr->e_phentsize));
 	decode_dyn(lib);
 
 	if (aux[AT_PHDR]) {
 		size_t interp_off = 0;
 		size_t tls_image = 0;
 		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
-		phdr = (void *)aux[AT_PHDR];
-		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
+		app->phdr = phdr = (void *)aux[AT_PHDR];
+                app->phnum = aux[AT_PHNUM];
+		for (i=app->phnum; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
 			if (phdr->p_type == PT_PHDR)
 				app->base = (void *)(aux[AT_PHDR] - phdr->p_vaddr);
 			else if (phdr->p_type == PT_INTERP)
@@ -890,7 +896,8 @@ void *__dynlink(int argc, char **argv)
 	/* Attach to vdso, if provided by the kernel */
 	if (search_vec(auxv, &vdso_base, AT_SYSINFO_EHDR)) {
 		ehdr = (void *)vdso_base;
-		phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phdr = phdr = (void *)(vdso_base + ehdr->e_phoff);
+                vdso->phnum = ehdr->e_phnum;
 		for (i=ehdr->e_phnum; i; i--, phdr=(void *)((char *)phdr + ehdr->e_phentsize)) {
 			if (phdr->p_type == PT_DYNAMIC)
 				vdso->dynv = (void *)(vdso_base + phdr->p_offset);
@@ -1166,6 +1173,30 @@ void *__dlsym(void *restrict p, const char *restrict s, void *restrict ra)
 	pthread_rwlock_unlock(&lock);
 	return res;
 }
+
+int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
+{
+    struct dso *current, *saved_tail;
+    struct dl_phdr_info info;
+    int ret = 0;
+
+    pthread_rwlock_rdlock(&lock);
+    saved_tail = tail;
+    pthread_rwlock_unlock(&lock);
+
+    for(current = head; ; current = current->next) {
+        info.dlpi_addr  = (uintptr_t)current->base;
+        info.dlpi_name  = current->name;
+        info.dlpi_phdr  = current->phdr;
+        info.dlpi_phnum = current->phnum;
+
+        ret = (callback)(&info, sizeof (info), data);
+        if (ret != 0) break;
+
+        if (current == saved_tail) break;
+    }
+    return ret;
+}
 #else
 void *dlopen(const char *file, int mode)
 {

[-- Attachment #3: link.h --]
[-- Type: text/x-chdr, Size: 474 bytes --]

#ifndef _LINK_H
#define _LLINK_H

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

#if defined(_GNU_SOURCE) || defined(_BSD_SOURCE)

#ifdef _LP64
#define ElfW(type) Elf64_ ## type
#else
#define ElfW(type) Elf32_ ## type
#endif

struct dl_phdr_info {
    uintptr_t dlpi_addr;
    const char *dlpi_name;
    const ElfW(Phdr) *dlpi_phdr;
    uint16_t dlpi_phnum;
};

int dl_iterate_phdr(int (*)(struct dl_phdr_info *, size_t, void *), void *);

#endif
#endif

[-- Attachment #4: dltest.c --]
[-- Type: text/x-csrc, Size: 623 bytes --]

#define _GNU_SOURCE
#include <dlfcn.h>
#include <link.h>
#include <stdlib.h>
#include <stdio.h>

static int
callback(struct dl_phdr_info *info, size_t size, void *data)
{
    int j;

   printf("name=%s (%d segments)\n", info->dlpi_name,
        info->dlpi_phnum);

   for (j = 0; j < info->dlpi_phnum; j++)
         printf("\t\t header %2d: address=%10p\n", j,
             (void *) (info->dlpi_addr + info->dlpi_phdr[j].p_vaddr));
    return 0;
}

int
main(int argc, char *argv[])
{
    void *test;
    test = dlopen("/compat/linux/lib/libavl.so", RTLD_NOW);
    dl_iterate_phdr(callback, NULL);

   exit(EXIT_SUCCESS);
}

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Fri, Oct 12, 2012 at 08:28:59AM -0500, Alex Caudill wrote:
> Thanks for the review! To clarify, I meant that the patch is public
> domain. This version implements most of your changes and attempts
> support for dlopen()'d libraries and vdso.
> 
> Some questions:
> 
> 1.) I guarded link.h with _GNU_SOURCE or _BSD_SOURCE; I'm unclear on
> whether this is necessary since it isn't a standard header, so please
> let me know your preference.

Not needed. I'd rather avoid excessive feature test macro checks like
this. The only time it might be desirable is when the nonstandard
header has a strong traditional precedent and the GNU version pollutes
the namespace with a lot of junk that might break apps. Then, the GNU
part should be protected.

Also, FYI, any header that checks feature test macros needs to be
including <features.h> to setup the default features if none are
explicitly defined.

> 2.) I'm unsure of how to replace ElfW(Phdr) in dl_phdr_info - or did
> you mean that musl should internally use some opaque type for
> dl_phdr_info? Also, should ElfW() go in elf.h?

Indeed, I think it's best to just keep using ElfW like you originally
did, then.

The issue with the types in elf.h is that they are designed for
writing a program that processes arbitrary (possibly foreign arch) ELF
files, not for writing native code, and thereby they make it so that
you have to use ugly macros to get the right native types. Most of
musl's dynamic linker just uses size_t for the address/word-size
types, and uint32_t and uint16_t for the unconditionally 32/16-bit
types, rather than the complex ELF type mess. However, since link.h is
a public interface, it's probably best to just follow the ugly
precedent there. After all, if somebody takes &foo->dlpi_addr, they
expect it to have the right type, and this could break if the ELF type
is unsigned long while the uintptr_t type is unsigned int (even if
they have the same size/representation).

Sorry for wasting your time with this issue.

> 3.) The attached dltest.c reveals a problem with dlopen()'d libraries
> which seems to be related to the locking strategy (see output below).
> If I don't take the lock and check for current == saved_tail, it
> "fixes" the example. At the least, I think this reveals a flaw with
> dlopen("foo", RTLD_NOW) - shouldn't it hold the lock until the dso
> list has been updated?
> 
> This function is used by libunwind and (I think) libgcc_eh for C++
> exception support, and it's possible that additional fields in
> dl_phdr_info will be necessary in order for those to work unmodified
> with musl. I'll look into this today and come up with more tests.
> Solaris and FreeBSD, at least, have these appended to struct
> dl_phdr_info:
> 
> unsigned long long int dlpi_adds;       /* total # of loads */
> unsigned long long int dlpi_subs;       /* total # of unloads */
> 
> The dl_iterate_phdr() callback is passed a size arg, and the callback
> is responsible for checking the size to ensure that the additional
> fields are present. Don't shoot the messenger! :)

That's no problem; they look easy to add. Also, the last two fields
for TLS stuff would be easy to add; they're already in struct dso (as
tls_image and tls_id).

> @@ -57,6 +58,8 @@ struct dso {
>  	size_t *dynv;
>  	struct dso *next, *prev;
>  
> +        Phdr *phdr;
> +        uint16_t phnum;

It looks like your patch is mixing tabs and spaces. Please use tabs
for indention, and make indention consistent with the rest of musl.

>  	if (aux[AT_PHDR]) {
>  		size_t interp_off = 0;
>  		size_t tls_image = 0;
>  		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
> -		phdr = (void *)aux[AT_PHDR];
> -		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
> +		app->phdr = phdr = (void *)aux[AT_PHDR];
> +                app->phnum = aux[AT_PHNUM];
> +		for (i=app->phnum; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {

I would avoid changing the for look here. Hopefully the compiler
generates the same code either way, but in principle it's
easier/faster to read from aux[AT_PHNUM] (a fixed offset from the
stack pointer) than through indirection via app->phnum. And avoiding
the extra change makes it clear that the patch is not changing much,
just saving new values.

> +int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
> +{
> +    struct dso *current, *saved_tail;
> +    struct dl_phdr_info info;
> +    int ret = 0;
> +
> +    pthread_rwlock_rdlock(&lock);
> +    saved_tail = tail;
> +    pthread_rwlock_unlock(&lock);
> +
> +    for(current = head; ; current = current->next) {
> +        info.dlpi_addr  = (uintptr_t)current->base;
> +        info.dlpi_name  = current->name;
> +        info.dlpi_phdr  = current->phdr;
> +        info.dlpi_phnum = current->phnum;
> +
> +        ret = (callback)(&info, sizeof (info), data);
> +        if (ret != 0) break;
> +
> +        if (current == saved_tail) break;
> +    }
> +    return ret;
> +}
>  #else
>  void *dlopen(const char *file, int mode)

After the #else, there are no-op/dummy versions of all the dl
functions for static linked programs. I think it should be possible to
do this for dl_iterate_phdr too -- just use __libc.auxv to get the
AT_PHDR and AT_PHNUM entries, and there's only a single DSO, the main
program.

Actually with vsyscall, there's also code running from the vdso, but
exceptions cannot occur in that code so only the debugger needs to be
aware of its existence. If you want, you could report it with
__libc.auxv using AT_SYSINFO_EHDR to get the phdr pointer.

> #ifdef _LP64

This should still be something like #if UINTPTR_MAX > 0xffffffff

> struct dl_phdr_info {
>     uintptr_t dlpi_addr;
>     const char *dlpi_name;
>     const ElfW(Phdr) *dlpi_phdr;
>     uint16_t dlpi_phnum;
> };

And could you use consistent indention (tabs) here?

Rich

Re: PATCH: dl_iterate_phdr()

[Alex Caudill]

[-- Attachment #1: Type: text/plain, Size: 6568 bytes --]

Thanks again, Rich - I think this turned out to be a pretty clean
implementation. I got frustrated with the static version (it's almost
twice as big!), but I'm planning to get it done tomorrow. For now,
though, here's the updated dynamic version and a revised header.

One minor thing: I believe the (Addr)current->base cast is necessary
to ensure correctness (per your previous mail). Please correct me if
I'm wrong.

On 10/12/12, Rich Felker <dalias@aerifal.cx> wrote:
> On Fri, Oct 12, 2012 at 08:28:59AM -0500, Alex Caudill wrote:
>> Thanks for the review! To clarify, I meant that the patch is public
>> domain. This version implements most of your changes and attempts
>> support for dlopen()'d libraries and vdso.
>>
>> Some questions:
>>
>> 1.) I guarded link.h with _GNU_SOURCE or _BSD_SOURCE; I'm unclear on
>> whether this is necessary since it isn't a standard header, so please
>> let me know your preference.
>
> Not needed. I'd rather avoid excessive feature test macro checks like
> this. The only time it might be desirable is when the nonstandard
> header has a strong traditional precedent and the GNU version pollutes
> the namespace with a lot of junk that might break apps. Then, the GNU
> part should be protected.
>
> Also, FYI, any header that checks feature test macros needs to be
> including <features.h> to setup the default features if none are
> explicitly defined.
>
>> 2.) I'm unsure of how to replace ElfW(Phdr) in dl_phdr_info - or did
>> you mean that musl should internally use some opaque type for
>> dl_phdr_info? Also, should ElfW() go in elf.h?
>
> Indeed, I think it's best to just keep using ElfW like you originally
> did, then.
>
> The issue with the types in elf.h is that they are designed for
> writing a program that processes arbitrary (possibly foreign arch) ELF
> files, not for writing native code, and thereby they make it so that
> you have to use ugly macros to get the right native types. Most of
> musl's dynamic linker just uses size_t for the address/word-size
> types, and uint32_t and uint16_t for the unconditionally 32/16-bit
> types, rather than the complex ELF type mess. However, since link.h is
> a public interface, it's probably best to just follow the ugly
> precedent there. After all, if somebody takes &foo->dlpi_addr, they
> expect it to have the right type, and this could break if the ELF type
> is unsigned long while the uintptr_t type is unsigned int (even if
> they have the same size/representation).
>
> Sorry for wasting your time with this issue.
>
>> 3.) The attached dltest.c reveals a problem with dlopen()'d libraries
>> which seems to be related to the locking strategy (see output below).
>> If I don't take the lock and check for current == saved_tail, it
>> "fixes" the example. At the least, I think this reveals a flaw with
>> dlopen("foo", RTLD_NOW) - shouldn't it hold the lock until the dso
>> list has been updated?
>>
>> This function is used by libunwind and (I think) libgcc_eh for C++
>> exception support, and it's possible that additional fields in
>> dl_phdr_info will be necessary in order for those to work unmodified
>> with musl. I'll look into this today and come up with more tests.
>> Solaris and FreeBSD, at least, have these appended to struct
>> dl_phdr_info:
>>
>> unsigned long long int dlpi_adds;       /* total # of loads */
>> unsigned long long int dlpi_subs;       /* total # of unloads */
>>
>> The dl_iterate_phdr() callback is passed a size arg, and the callback
>> is responsible for checking the size to ensure that the additional
>> fields are present. Don't shoot the messenger! :)
>
> That's no problem; they look easy to add. Also, the last two fields
> for TLS stuff would be easy to add; they're already in struct dso (as
> tls_image and tls_id).
>
>> @@ -57,6 +58,8 @@ struct dso {
>>  	size_t *dynv;
>>  	struct dso *next, *prev;
>>
>> +        Phdr *phdr;
>> +        uint16_t phnum;
>
> It looks like your patch is mixing tabs and spaces. Please use tabs
> for indention, and make indention consistent with the rest of musl.
>
>>  	if (aux[AT_PHDR]) {
>>  		size_t interp_off = 0;
>>  		size_t tls_image = 0;
>>  		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
>> -		phdr = (void *)aux[AT_PHDR];
>> -		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr +
>> aux[AT_PHENT])) {
>> +		app->phdr = phdr = (void *)aux[AT_PHDR];
>> +                app->phnum = aux[AT_PHNUM];
>> +		for (i=app->phnum; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT]))
>> {
>
> I would avoid changing the for look here. Hopefully the compiler
> generates the same code either way, but in principle it's
> easier/faster to read from aux[AT_PHNUM] (a fixed offset from the
> stack pointer) than through indirection via app->phnum. And avoiding
> the extra change makes it clear that the patch is not changing much,
> just saving new values.
>
>> +int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t
>> size, void *data), void *data)
>> +{
>> +    struct dso *current, *saved_tail;
>> +    struct dl_phdr_info info;
>> +    int ret = 0;
>> +
>> +    pthread_rwlock_rdlock(&lock);
>> +    saved_tail = tail;
>> +    pthread_rwlock_unlock(&lock);
>> +
>> +    for(current = head; ; current = current->next) {
>> +        info.dlpi_addr  = (uintptr_t)current->base;
>> +        info.dlpi_name  = current->name;
>> +        info.dlpi_phdr  = current->phdr;
>> +        info.dlpi_phnum = current->phnum;
>> +
>> +        ret = (callback)(&info, sizeof (info), data);
>> +        if (ret != 0) break;
>> +
>> +        if (current == saved_tail) break;
>> +    }
>> +    return ret;
>> +}
>>  #else
>>  void *dlopen(const char *file, int mode)
>
> After the #else, there are no-op/dummy versions of all the dl
> functions for static linked programs. I think it should be possible to
> do this for dl_iterate_phdr too -- just use __libc.auxv to get the
> AT_PHDR and AT_PHNUM entries, and there's only a single DSO, the main
> program.
>
> Actually with vsyscall, there's also code running from the vdso, but
> exceptions cannot occur in that code so only the debugger needs to be
> aware of its existence. If you want, you could report it with
> __libc.auxv using AT_SYSINFO_EHDR to get the phdr pointer.
>
>> #ifdef _LP64
>
> This should still be something like #if UINTPTR_MAX > 0xffffffff
>
>> struct dl_phdr_info {
>>     uintptr_t dlpi_addr;
>>     const char *dlpi_name;
>>     const ElfW(Phdr) *dlpi_phdr;
>>     uint16_t dlpi_phnum;
>> };
>
> And could you use consistent indention (tabs) here?
>
> Rich
>

[-- Attachment #2: dynlink.patch --]
[-- Type: application/octet-stream, Size: 4199 bytes --]

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index c3cb611..2802c21 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <elf.h>
+#include <link.h>
 #include <setjmp.h>
 #include <pthread.h>
 #include <ctype.h>
@@ -30,12 +31,14 @@ static char errbuf[128];
 typedef Elf32_Ehdr Ehdr;
 typedef Elf32_Phdr Phdr;
 typedef Elf32_Sym Sym;
+typedef Elf32_Addr Addr;
 #define R_TYPE(x) ((x)&255)
 #define R_SYM(x) ((x)>>8)
 #else
 typedef Elf64_Ehdr Ehdr;
 typedef Elf64_Phdr Phdr;
 typedef Elf64_Sym Sym;
+typedef Elf64_Addr Addr;
 #define R_TYPE(x) ((x)&0xffffffff)
 #define R_SYM(x) ((x)>>32)
 #endif
@@ -57,6 +60,8 @@ struct dso {
 	size_t *dynv;
 	struct dso *next, *prev;
 
+	Phdr *phdr;
+	uint16_t phnum;
 	int refcnt;
 	Sym *syms;
 	uint32_t *hashtab;
@@ -92,6 +97,7 @@ void *__install_initial_tls(void *);
 
 static struct dso *head, *tail, *libc, *fini_head;
 static char *env_path, *sys_path, *r_path;
+static unsigned long long gencnt = 0;
 static int ssp_used;
 static int runtime;
 static int ldd_mode;
@@ -324,6 +330,8 @@ static void *map_library(int fd, struct dso *dso)
 		eh->e_phoff = sizeof *eh;
 	}
 	ph = (void *)((char *)buf + eh->e_phoff);
+	dso->phdr = ph;
+	dso->phnum = (uint16_t)eh->e_phnum;
 	for (i=eh->e_phnum; i; i--, ph=(void *)((char *)ph+eh->e_phentsize)) {
 		if (ph->p_type == PT_DYNAMIC)
 			dyn = ph->p_vaddr;
@@ -815,18 +823,19 @@ void *__dynlink(int argc, char **argv)
 	lib->name = lib->shortname = "libc.so";
 	lib->global = 1;
 	ehdr = (void *)lib->base;
-	find_map_range((void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize, lib);
-	lib->dynv = (void *)(lib->base + find_dyn(
-		(void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize));
+	lib->phnum = (uint16_t)ehdr->e_phnum;
+	lib->phdr = (void *)(aux[AT_BASE]+ehdr->e_phoff);
+	find_map_range(lib->phdr, ehdr->e_phnum, ehdr->e_phentsize, lib);
+	lib->dynv = (void *)(lib->base + find_dyn(lib->phdr,
+                    ehdr->e_phnum, ehdr->e_phentsize));
 	decode_dyn(lib);
 
 	if (aux[AT_PHDR]) {
 		size_t interp_off = 0;
 		size_t tls_image = 0;
 		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
-		phdr = (void *)aux[AT_PHDR];
+		app->phdr = phdr = (void *)aux[AT_PHDR];
+		app->phnum = aux[AT_PHNUM];
 		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
 			if (phdr->p_type == PT_PHDR)
 				app->base = (void *)(aux[AT_PHDR] - phdr->p_vaddr);
@@ -890,7 +899,8 @@ void *__dynlink(int argc, char **argv)
 	/* Attach to vdso, if provided by the kernel */
 	if (search_vec(auxv, &vdso_base, AT_SYSINFO_EHDR)) {
 		ehdr = (void *)vdso_base;
-		phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phdr = phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phnum = ehdr->e_phnum;
 		for (i=ehdr->e_phnum; i; i--, phdr=(void *)((char *)phdr + ehdr->e_phentsize)) {
 			if (phdr->p_type == PT_DYNAMIC)
 				vdso->dynv = (void *)(vdso_base + phdr->p_offset);
@@ -1051,6 +1061,7 @@ void *dlopen(const char *file, int mode)
 	orig_tail = tail;
 end:
 	__release_ptc();
+	if (errflag == 0) gencnt++;
 	pthread_rwlock_unlock(&lock);
 	if (p) do_init_fini(orig_tail);
 	pthread_setcancelstate(cs, 0);
@@ -1166,6 +1177,33 @@ void *__dlsym(void *restrict p, const char *restrict s, void *restrict ra)
 	pthread_rwlock_unlock(&lock);
 	return res;
 }
+
+int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
+{
+	struct dso *current;
+	struct dl_phdr_info info;
+	int ret = 0;
+	for(current = head; current;) {
+		info.dlpi_addr      = (Addr)current->base;
+		info.dlpi_name      = current->name;
+		info.dlpi_phdr      = current->phdr;
+		info.dlpi_phnum     = current->phnum;
+		info.dlpi_adds      = gencnt;
+		info.dlpi_subs      = 0;
+		info.dlpi_tls_modid = current->tls_id;
+		info.dlpi_tls_data  = current->tls_image;
+
+		ret = (callback)(&info, sizeof (info), data);
+
+		if (ret != 0) break;
+		if (current == tail) break;
+
+		pthread_rwlock_rdlock(&lock);
+		current = current->next;
+		pthread_rwlock_unlock(&lock);
+	}
+	return ret;
+}
 #else
 void *dlopen(const char *file, int mode)
 {

[-- Attachment #3: link.h --]
[-- Type: text/x-chdr, Size: 539 bytes --]

#ifndef _LINK_H
#define _LINK_H

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

#if UINTPTR_MAX > 0xffffffff
#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;
	unsigned long long int dlpi_adds;
	unsigned long long int dlpi_subs;
	size_t dlpi_tls_modid;
	void *dlpi_tls_data;
};

int dl_iterate_phdr(int (*)(struct dl_phdr_info *, size_t, void *), void *);

#endif

Re: PATCH: dl_iterate_phdr()

[Alex Caudill]

[-- Attachment #1: Type: text/plain, Size: 6841 bytes --]

No, seriously, this time it's right! ;)

On 10/12/12, Alex Caudill <alex.caudill@gmail.com> wrote:
> Thanks again, Rich - I think this turned out to be a pretty clean
> implementation. I got frustrated with the static version (it's almost
> twice as big!), but I'm planning to get it done tomorrow. For now,
> though, here's the updated dynamic version and a revised header.
>
> One minor thing: I believe the (Addr)current->base cast is necessary
> to ensure correctness (per your previous mail). Please correct me if
> I'm wrong.
>
> On 10/12/12, Rich Felker <dalias@aerifal.cx> wrote:
>> On Fri, Oct 12, 2012 at 08:28:59AM -0500, Alex Caudill wrote:
>>> Thanks for the review! To clarify, I meant that the patch is public
>>> domain. This version implements most of your changes and attempts
>>> support for dlopen()'d libraries and vdso.
>>>
>>> Some questions:
>>>
>>> 1.) I guarded link.h with _GNU_SOURCE or _BSD_SOURCE; I'm unclear on
>>> whether this is necessary since it isn't a standard header, so please
>>> let me know your preference.
>>
>> Not needed. I'd rather avoid excessive feature test macro checks like
>> this. The only time it might be desirable is when the nonstandard
>> header has a strong traditional precedent and the GNU version pollutes
>> the namespace with a lot of junk that might break apps. Then, the GNU
>> part should be protected.
>>
>> Also, FYI, any header that checks feature test macros needs to be
>> including <features.h> to setup the default features if none are
>> explicitly defined.
>>
>>> 2.) I'm unsure of how to replace ElfW(Phdr) in dl_phdr_info - or did
>>> you mean that musl should internally use some opaque type for
>>> dl_phdr_info? Also, should ElfW() go in elf.h?
>>
>> Indeed, I think it's best to just keep using ElfW like you originally
>> did, then.
>>
>> The issue with the types in elf.h is that they are designed for
>> writing a program that processes arbitrary (possibly foreign arch) ELF
>> files, not for writing native code, and thereby they make it so that
>> you have to use ugly macros to get the right native types. Most of
>> musl's dynamic linker just uses size_t for the address/word-size
>> types, and uint32_t and uint16_t for the unconditionally 32/16-bit
>> types, rather than the complex ELF type mess. However, since link.h is
>> a public interface, it's probably best to just follow the ugly
>> precedent there. After all, if somebody takes &foo->dlpi_addr, they
>> expect it to have the right type, and this could break if the ELF type
>> is unsigned long while the uintptr_t type is unsigned int (even if
>> they have the same size/representation).
>>
>> Sorry for wasting your time with this issue.
>>
>>> 3.) The attached dltest.c reveals a problem with dlopen()'d libraries
>>> which seems to be related to the locking strategy (see output below).
>>> If I don't take the lock and check for current == saved_tail, it
>>> "fixes" the example. At the least, I think this reveals a flaw with
>>> dlopen("foo", RTLD_NOW) - shouldn't it hold the lock until the dso
>>> list has been updated?
>>>
>>> This function is used by libunwind and (I think) libgcc_eh for C++
>>> exception support, and it's possible that additional fields in
>>> dl_phdr_info will be necessary in order for those to work unmodified
>>> with musl. I'll look into this today and come up with more tests.
>>> Solaris and FreeBSD, at least, have these appended to struct
>>> dl_phdr_info:
>>>
>>> unsigned long long int dlpi_adds;       /* total # of loads */
>>> unsigned long long int dlpi_subs;       /* total # of unloads */
>>>
>>> The dl_iterate_phdr() callback is passed a size arg, and the callback
>>> is responsible for checking the size to ensure that the additional
>>> fields are present. Don't shoot the messenger! :)
>>
>> That's no problem; they look easy to add. Also, the last two fields
>> for TLS stuff would be easy to add; they're already in struct dso (as
>> tls_image and tls_id).
>>
>>> @@ -57,6 +58,8 @@ struct dso {
>>>  	size_t *dynv;
>>>  	struct dso *next, *prev;
>>>
>>> +        Phdr *phdr;
>>> +        uint16_t phnum;
>>
>> It looks like your patch is mixing tabs and spaces. Please use tabs
>> for indention, and make indention consistent with the rest of musl.
>>
>>>  	if (aux[AT_PHDR]) {
>>>  		size_t interp_off = 0;
>>>  		size_t tls_image = 0;
>>>  		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
>>> -		phdr = (void *)aux[AT_PHDR];
>>> -		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr +
>>> aux[AT_PHENT])) {
>>> +		app->phdr = phdr = (void *)aux[AT_PHDR];
>>> +                app->phnum = aux[AT_PHNUM];
>>> +		for (i=app->phnum; i; i--, phdr=(void *)((char *)phdr +
>>> aux[AT_PHENT]))
>>> {
>>
>> I would avoid changing the for look here. Hopefully the compiler
>> generates the same code either way, but in principle it's
>> easier/faster to read from aux[AT_PHNUM] (a fixed offset from the
>> stack pointer) than through indirection via app->phnum. And avoiding
>> the extra change makes it clear that the patch is not changing much,
>> just saving new values.
>>
>>> +int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t
>>> size, void *data), void *data)
>>> +{
>>> +    struct dso *current, *saved_tail;
>>> +    struct dl_phdr_info info;
>>> +    int ret = 0;
>>> +
>>> +    pthread_rwlock_rdlock(&lock);
>>> +    saved_tail = tail;
>>> +    pthread_rwlock_unlock(&lock);
>>> +
>>> +    for(current = head; ; current = current->next) {
>>> +        info.dlpi_addr  = (uintptr_t)current->base;
>>> +        info.dlpi_name  = current->name;
>>> +        info.dlpi_phdr  = current->phdr;
>>> +        info.dlpi_phnum = current->phnum;
>>> +
>>> +        ret = (callback)(&info, sizeof (info), data);
>>> +        if (ret != 0) break;
>>> +
>>> +        if (current == saved_tail) break;
>>> +    }
>>> +    return ret;
>>> +}
>>>  #else
>>>  void *dlopen(const char *file, int mode)
>>
>> After the #else, there are no-op/dummy versions of all the dl
>> functions for static linked programs. I think it should be possible to
>> do this for dl_iterate_phdr too -- just use __libc.auxv to get the
>> AT_PHDR and AT_PHNUM entries, and there's only a single DSO, the main
>> program.
>>
>> Actually with vsyscall, there's also code running from the vdso, but
>> exceptions cannot occur in that code so only the debugger needs to be
>> aware of its existence. If you want, you could report it with
>> __libc.auxv using AT_SYSINFO_EHDR to get the phdr pointer.
>>
>>> #ifdef _LP64
>>
>> This should still be something like #if UINTPTR_MAX > 0xffffffff
>>
>>> struct dl_phdr_info {
>>>     uintptr_t dlpi_addr;
>>>     const char *dlpi_name;
>>>     const ElfW(Phdr) *dlpi_phdr;
>>>     uint16_t dlpi_phnum;
>>> };
>>
>> And could you use consistent indention (tabs) here?
>>
>> Rich
>>
>

[-- Attachment #2: dynlink.patch --]
[-- Type: application/octet-stream, Size: 4199 bytes --]

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index c3cb611..09b4b82 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <elf.h>
+#include <link.h>
 #include <setjmp.h>
 #include <pthread.h>
 #include <ctype.h>
@@ -30,12 +31,14 @@ static char errbuf[128];
 typedef Elf32_Ehdr Ehdr;
 typedef Elf32_Phdr Phdr;
 typedef Elf32_Sym Sym;
+typedef Elf32_Addr Addr;
 #define R_TYPE(x) ((x)&255)
 #define R_SYM(x) ((x)>>8)
 #else
 typedef Elf64_Ehdr Ehdr;
 typedef Elf64_Phdr Phdr;
 typedef Elf64_Sym Sym;
+typedef Elf64_Addr Addr;
 #define R_TYPE(x) ((x)&0xffffffff)
 #define R_SYM(x) ((x)>>32)
 #endif
@@ -57,6 +60,8 @@ struct dso {
 	size_t *dynv;
 	struct dso *next, *prev;
 
+	Phdr *phdr;
+	uint16_t phnum;
 	int refcnt;
 	Sym *syms;
 	uint32_t *hashtab;
@@ -92,6 +97,7 @@ void *__install_initial_tls(void *);
 
 static struct dso *head, *tail, *libc, *fini_head;
 static char *env_path, *sys_path, *r_path;
+static unsigned long long gencnt = 0;
 static int ssp_used;
 static int runtime;
 static int ldd_mode;
@@ -324,6 +330,8 @@ static void *map_library(int fd, struct dso *dso)
 		eh->e_phoff = sizeof *eh;
 	}
 	ph = (void *)((char *)buf + eh->e_phoff);
+	dso->phdr = ph;
+	dso->phnum = (uint16_t)eh->e_phnum;
 	for (i=eh->e_phnum; i; i--, ph=(void *)((char *)ph+eh->e_phentsize)) {
 		if (ph->p_type == PT_DYNAMIC)
 			dyn = ph->p_vaddr;
@@ -815,18 +823,19 @@ void *__dynlink(int argc, char **argv)
 	lib->name = lib->shortname = "libc.so";
 	lib->global = 1;
 	ehdr = (void *)lib->base;
-	find_map_range((void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize, lib);
-	lib->dynv = (void *)(lib->base + find_dyn(
-		(void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize));
+	lib->phnum = (uint16_t)ehdr->e_phnum;
+	lib->phdr = (void *)(aux[AT_BASE]+ehdr->e_phoff);
+	find_map_range(lib->phdr, ehdr->e_phnum, ehdr->e_phentsize, lib);
+	lib->dynv = (void *)(lib->base + find_dyn(lib->phdr,
+                    ehdr->e_phnum, ehdr->e_phentsize));
 	decode_dyn(lib);
 
 	if (aux[AT_PHDR]) {
 		size_t interp_off = 0;
 		size_t tls_image = 0;
 		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
-		phdr = (void *)aux[AT_PHDR];
+		app->phdr = phdr = (void *)aux[AT_PHDR];
+		app->phnum = aux[AT_PHNUM];
 		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
 			if (phdr->p_type == PT_PHDR)
 				app->base = (void *)(aux[AT_PHDR] - phdr->p_vaddr);
@@ -890,7 +899,8 @@ void *__dynlink(int argc, char **argv)
 	/* Attach to vdso, if provided by the kernel */
 	if (search_vec(auxv, &vdso_base, AT_SYSINFO_EHDR)) {
 		ehdr = (void *)vdso_base;
-		phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phdr = phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phnum = ehdr->e_phnum;
 		for (i=ehdr->e_phnum; i; i--, phdr=(void *)((char *)phdr + ehdr->e_phentsize)) {
 			if (phdr->p_type == PT_DYNAMIC)
 				vdso->dynv = (void *)(vdso_base + phdr->p_offset);
@@ -1051,6 +1061,7 @@ void *dlopen(const char *file, int mode)
 	orig_tail = tail;
 end:
 	__release_ptc();
+	if (errflag == 0) gencnt++;
 	pthread_rwlock_unlock(&lock);
 	if (p) do_init_fini(orig_tail);
 	pthread_setcancelstate(cs, 0);
@@ -1166,6 +1177,33 @@ void *__dlsym(void *restrict p, const char *restrict s, void *restrict ra)
 	pthread_rwlock_unlock(&lock);
 	return res;
 }
+
+int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
+{
+	struct dso *current;
+	struct dl_phdr_info info;
+	int ret = 0;
+	for(current = head; current;) {
+		info.dlpi_addr      = (Addr)current->base;
+		info.dlpi_name      = current->name;
+		info.dlpi_phdr      = current->phdr;
+		info.dlpi_phnum     = current->phnum;
+		info.dlpi_adds      = gencnt;
+		info.dlpi_subs      = 0;
+		info.dlpi_tls_modid = current->tls_id;
+		info.dlpi_tls_data  = current->tls_image;
+
+		ret = (callback)(&info, sizeof (info), data);
+
+		if (ret != 0) break;
+
+		pthread_rwlock_rdlock(&lock);
+		if (current == tail) break;
+		current = current->next;
+		pthread_rwlock_unlock(&lock);
+	}
+	return ret;
+}
 #else
 void *dlopen(const char *file, int mode)
 {

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Fri, Oct 12, 2012 at 08:24:24PM -0500, Alex Caudill wrote:
> No, seriously, this time it's right! ;)

No, still at least one bug and a few style issues.

> On 10/12/12, Alex Caudill <alex.caudill@gmail.com> wrote:
> @@ -30,12 +31,14 @@ static char errbuf[128];
>  typedef Elf32_Ehdr Ehdr;
>  typedef Elf32_Phdr Phdr;
>  typedef Elf32_Sym Sym;
> +typedef Elf32_Addr Addr;

This is not needed.
> +	Phdr *phdr;
> +	uint16_t phnum;

There's no reason to use uint16_t; it just causes gratuitous slowness
and bloat on archs where accessign 16-bit units is painful. Just use
a clean type like int.

> @@ -324,6 +330,8 @@ static void *map_library(int fd, struct dso *dso)
>  		eh->e_phoff = sizeof *eh;
>  	}
>  	ph = (void *)((char *)buf + eh->e_phoff);
> +	dso->phdr = ph;
> +	dso->phnum = (uint16_t)eh->e_phnum;

This cast is also useless.

> @@ -815,18 +823,19 @@ void *__dynlink(int argc, char **argv)
>  	lib->name = lib->shortname = "libc.so";
>  	lib->global = 1;
>  	ehdr = (void *)lib->base;
> -	find_map_range((void *)(aux[AT_BASE]+ehdr->e_phoff),
> -		ehdr->e_phnum, ehdr->e_phentsize, lib);
> -	lib->dynv = (void *)(lib->base + find_dyn(
> -		(void *)(aux[AT_BASE]+ehdr->e_phoff),
> -		ehdr->e_phnum, ehdr->e_phentsize));
> +	lib->phnum = (uint16_t)ehdr->e_phnum;

And so is this one.

> @@ -1051,6 +1061,7 @@ void *dlopen(const char *file, int mode)
>  	orig_tail = tail;
>  end:
>  	__release_ptc();
> +	if (errflag == 0) gencnt++;

Use if (p), not if (errflag == 0). Not sure if it's a bug, but at
least it's an inconsistency with the below check.

>  	pthread_rwlock_unlock(&lock);
>  	if (p) do_init_fini(orig_tail);
>  	pthread_setcancelstate(cs, 0);
> @@ -1166,6 +1177,33 @@ void *__dlsym(void *restrict p, const char *restrict s, void *restrict ra)
>  	pthread_rwlock_unlock(&lock);
>  	return res;
>  }
> +
> +int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
> +{
> +	struct dso *current;
> +	struct dl_phdr_info info;
> +	int ret = 0;
> +	for(current = head; current;) {
> +		info.dlpi_addr      = (Addr)current->base;
> +		info.dlpi_name      = current->name;
> +		info.dlpi_phdr      = current->phdr;
> +		info.dlpi_phnum     = current->phnum;
> +		info.dlpi_adds      = gencnt;
> +		info.dlpi_subs      = 0;
> +		info.dlpi_tls_modid = current->tls_id;
> +		info.dlpi_tls_data  = current->tls_image;
> +
> +		ret = (callback)(&info, sizeof (info), data);
> +
> +		if (ret != 0) break;
> +
> +		pthread_rwlock_rdlock(&lock);
> +		if (current == tail) break;
> +		current = current->next;
> +		pthread_rwlock_unlock(&lock);

This code will return with the lock still held, which is rather
problematic. The idea of locking/unlocking on each iteration was not
to use this break logic, but instead rely on the for loop condition to
exit the loop. Just remove the if (current == tail) break; line and it
should be safe.

Rich

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Fri, Oct 12, 2012 at 08:24:24PM -0500, Alex Caudill wrote:
> No, seriously, this time it's right! ;)

Ping. Do you have any further work, or should I fix up and finish this
to commit it?

Rich

Re: PATCH: dl_iterate_phdr()

[Alex Caudill]

No kidding: I ragequit the entire internet for a solid 48 hours due to
this thread. Turned off my phone, dropped off the grid. I had to step
away and reconsider my entire approach to software and really figure
out if I should even be spending my time on this. It's so humbling
when you think you're beginning to grasp something and then realize
that you know NOTHING.

Anyway, please let me finish this :)

On Sun, Oct 14, 2012 at 10:30 PM, Rich Felker <dalias@aerifal.cx> wrote:
>
> On Fri, Oct 12, 2012 at 08:24:24PM -0500, Alex Caudill wrote:
> > No, seriously, this time it's right! ;)
>
> Ping. Do you have any further work, or should I fix up and finish this
> to commit it?
>
> Rich

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Sun, Oct 14, 2012 at 11:47:38PM -0500, Alex Caudill wrote:
> No kidding: I ragequit the entire internet for a solid 48 hours due to
> this thread. Turned off my phone, dropped off the grid. I had to step
> away and reconsider my entire approach to software and really figure
> out if I should even be spending my time on this. It's so humbling
> when you think you're beginning to grasp something and then realize
> that you know NOTHING.
> 
> Anyway, please let me finish this :)

Certainly. :)

Rich

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Sun, Oct 14, 2012 at 11:47:38PM -0500, Alex Caudill wrote:
> No kidding: I ragequit the entire internet for a solid 48 hours due to
> this thread. Turned off my phone, dropped off the grid. I had to step
> away and reconsider my entire approach to software and really figure
> out if I should even be spending my time on this. It's so humbling
> when you think you're beginning to grasp something and then realize
> that you know NOTHING.
> 
> Anyway, please let me finish this :)

Hope you're not getting discouraged. :)
I'd like to get both this and whatever other changed you need
(sigreturn?) in for the next release. Let me know if you're running
into any more problems or if you just need some time. It's not a hurry
since I'm waiting on some testing for microblaze anyway.

Rich

Re: PATCH: dl_iterate_phdr()

[Alex Caudill]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Sorry for the delay; I *think* this might do it ;)

On 10/18/12, Rich Felker <dalias@aerifal.cx> wrote:
> On Sun, Oct 14, 2012 at 11:47:38PM -0500, Alex Caudill wrote:
>> No kidding: I ragequit the entire internet for a solid 48 hours due to
>> this thread. Turned off my phone, dropped off the grid. I had to step
>> away and reconsider my entire approach to software and really figure
>> out if I should even be spending my time on this. It's so humbling
>> when you think you're beginning to grasp something and then realize
>> that you know NOTHING.
>>
>> Anyway, please let me finish this :)
>
> Hope you're not getting discouraged. :)
> I'd like to get both this and whatever other changed you need
> (sigreturn?) in for the next release. Let me know if you're running
> into any more problems or if you just need some time. It's not a hurry
> since I'm waiting on some testing for microblaze anyway.
>
> Rich
>

[-- Attachment #2: dynlink.patch --]
[-- Type: application/octet-stream, Size: 4132 bytes --]

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index c3cb611..2802c21 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <limits.h>
 #include <elf.h>
+#include <link.h>
 #include <setjmp.h>
 #include <pthread.h>
 #include <ctype.h>
@@ -30,12 +31,14 @@ static char errbuf[128];
 typedef Elf32_Ehdr Ehdr;
 typedef Elf32_Phdr Phdr;
 typedef Elf32_Sym Sym;
+typedef Elf32_Addr Addr;
 #define R_TYPE(x) ((x)&255)
 #define R_SYM(x) ((x)>>8)
 #else
 typedef Elf64_Ehdr Ehdr;
 typedef Elf64_Phdr Phdr;
 typedef Elf64_Sym Sym;
+typedef Elf64_Addr Addr;
 #define R_TYPE(x) ((x)&0xffffffff)
 #define R_SYM(x) ((x)>>32)
 #endif
@@ -57,6 +60,8 @@ struct dso {
 	size_t *dynv;
 	struct dso *next, *prev;
 
+	Phdr *phdr;
+	int phnum;
 	int refcnt;
 	Sym *syms;
 	uint32_t *hashtab;
@@ -92,6 +97,7 @@ void *__install_initial_tls(void *);
 
 static struct dso *head, *tail, *libc, *fini_head;
 static char *env_path, *sys_path, *r_path;
+static unsigned long long gencnt = 0;
 static int ssp_used;
 static int runtime;
 static int ldd_mode;
@@ -324,6 +330,8 @@ static void *map_library(int fd, struct dso *dso)
 		eh->e_phoff = sizeof *eh;
 	}
 	ph = (void *)((char *)buf + eh->e_phoff);
+	dso->phdr = ph;
+	dso->phnum = eh->e_phnum;
 	for (i=eh->e_phnum; i; i--, ph=(void *)((char *)ph+eh->e_phentsize)) {
 		if (ph->p_type == PT_DYNAMIC)
 			dyn = ph->p_vaddr;
@@ -815,18 +823,19 @@ void *__dynlink(int argc, char **argv)
 	lib->name = lib->shortname = "libc.so";
 	lib->global = 1;
 	ehdr = (void *)lib->base;
-	find_map_range((void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize, lib);
-	lib->dynv = (void *)(lib->base + find_dyn(
-		(void *)(aux[AT_BASE]+ehdr->e_phoff),
-		ehdr->e_phnum, ehdr->e_phentsize));
+	lib->phnum = ehdr->e_phnum;
+	lib->phdr = (void *)(aux[AT_BASE]+ehdr->e_phoff);
+	find_map_range(lib->phdr, ehdr->e_phnum, ehdr->e_phentsize, lib);
+	lib->dynv = (void *)(lib->base + find_dyn(lib->phdr,
+                    ehdr->e_phnum, ehdr->e_phentsize));
 	decode_dyn(lib);
 
 	if (aux[AT_PHDR]) {
 		size_t interp_off = 0;
 		size_t tls_image = 0;
 		/* Find load address of the main program, via AT_PHDR vs PT_PHDR. */
-		phdr = (void *)aux[AT_PHDR];
+		app->phdr = phdr = (void *)aux[AT_PHDR];
+		app->phnum = aux[AT_PHNUM];
 		for (i=aux[AT_PHNUM]; i; i--, phdr=(void *)((char *)phdr + aux[AT_PHENT])) {
 			if (phdr->p_type == PT_PHDR)
 				app->base = (void *)(aux[AT_PHDR] - phdr->p_vaddr);
@@ -890,7 +899,8 @@ void *__dynlink(int argc, char **argv)
 	/* Attach to vdso, if provided by the kernel */
 	if (search_vec(auxv, &vdso_base, AT_SYSINFO_EHDR)) {
 		ehdr = (void *)vdso_base;
-		phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phdr = phdr = (void *)(vdso_base + ehdr->e_phoff);
+		vdso->phnum = ehdr->e_phnum;
 		for (i=ehdr->e_phnum; i; i--, phdr=(void *)((char *)phdr + ehdr->e_phentsize)) {
 			if (phdr->p_type == PT_DYNAMIC)
 				vdso->dynv = (void *)(vdso_base + phdr->p_offset);
@@ -1051,6 +1061,7 @@ void *dlopen(const char *file, int mode)
 	orig_tail = tail;
 end:
 	__release_ptc();
+	if (p) gencnt++;
 	pthread_rwlock_unlock(&lock);
 	if (p) do_init_fini(orig_tail);
 	pthread_setcancelstate(cs, 0);
@@ -1166,6 +1177,33 @@ void *__dlsym(void *restrict p, const char *restrict s, void *restrict ra)
 	pthread_rwlock_unlock(&lock);
 	return res;
 }
+
+int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void *data), void *data)
+{
+	struct dso *current;
+	struct dl_phdr_info info;
+	int ret = 0;
+	for(current = head; current;) {
+		info.dlpi_addr      = (Addr)current->base;
+		info.dlpi_name      = current->name;
+		info.dlpi_phdr      = current->phdr;
+		info.dlpi_phnum     = current->phnum;
+		info.dlpi_adds      = gencnt;
+		info.dlpi_subs      = 0;
+		info.dlpi_tls_modid = current->tls_id;
+		info.dlpi_tls_data  = current->tls_image;
+
+		ret = (callback)(&info, sizeof (info), data);
+
+		if (ret != 0) break;
+
+		pthread_rwlock_rdlock(&lock);
+		current = current->next;
+		pthread_rwlock_unlock(&lock);
+	}
+	return ret;
+}
 #else
 void *dlopen(const char *file, int mode)
 {

Re: PATCH: dl_iterate_phdr()

[Rich Felker]

On Wed, Oct 31, 2012 at 01:35:08PM -0500, Alex Caudill wrote:
> Sorry for the delay; I *think* this might do it ;)

Thanks! Committed with very minor changes. It seems to be working fine
based on your original test program (in both dynamic and static linked
cases).

Rich