From: Alex Caudill <alex.caudill@gmail.com>
To: musl@lists.openwall.com
Subject: Re: PATCH: dl_iterate_phdr()
Date: Fri, 12 Oct 2012 20:24:24 -0500 [thread overview]
Message-ID: <CAFXnQt5YAi-E_YUZD0y0JteqavvUoSoGWPvzm0TJ+6cxzH=UnQ@mail.gmail.com> (raw)
In-Reply-To: <CAFXnQt6XPQhDU2-BOwS5Z9mgFcKWY7REJa-jUKA+QYbzGrDs5w@mail.gmail.com>
[-- 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)
{
next prev parent reply other threads:[~2012-10-13 1:24 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-11 14:29 Alex Caudill
2012-10-11 23:42 ` Rich Felker
2012-10-12 0:00 ` Rich Felker
2012-10-12 13:28 ` Alex Caudill
2012-10-12 15:43 ` Rich Felker
2012-10-13 1:04 ` Alex Caudill
2012-10-13 1:24 ` Alex Caudill [this message]
2012-10-13 1:31 ` Rich Felker
2012-10-15 3:30 ` Rich Felker
2012-10-15 4:47 ` Alex Caudill
2012-10-15 12:41 ` Rich Felker
2012-10-18 20:45 ` Rich Felker
2012-10-31 18:35 ` Alex Caudill
2012-11-01 1:33 ` Rich Felker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFXnQt5YAi-E_YUZD0y0JteqavvUoSoGWPvzm0TJ+6cxzH=UnQ@mail.gmail.com' \
--to=alex.caudill@gmail.com \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).