Re: PATCH: dl_iterate_phdr()

[Alex Caudill <alex.caudill@gmail.com>]

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