[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 12 Oct 2012 21:31:06 -0400
From: Rich Felker <dalias@...ifal.cx>
To: musl@...ts.openwall.com
Subject: Re: PATCH: dl_iterate_phdr()
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@...il.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
