Re: PATCH: dl_iterate_phdr()

[Rich Felker <dalias@aerifal.cx>]

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