From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/2091 Path: news.gmane.org!not-for-mail From: Alex Caudill Newsgroups: gmane.linux.lib.musl.general Subject: Re: PATCH: dl_iterate_phdr() Date: Fri, 12 Oct 2012 20:24:24 -0500 Message-ID: References: <20121011234255.GB254@brightrain.aerifal.cx> <20121012000034.GC254@brightrain.aerifal.cx> <20121012154354.GE254@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary=0016e6dedd83355a0d04cbe6aa7a X-Trace: ger.gmane.org 1350091482 30872 80.91.229.3 (13 Oct 2012 01:24:42 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sat, 13 Oct 2012 01:24:42 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-2092-gllmg-musl=m.gmane.org@lists.openwall.com Sat Oct 13 03:24:50 2012 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1TMqT5-00046Z-KH for gllmg-musl@plane.gmane.org; Sat, 13 Oct 2012 03:24:43 +0200 Original-Received: (qmail 15627 invoked by uid 550); 13 Oct 2012 01:24:37 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 15619 invoked from network); 13 Oct 2012 01:24:36 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; bh=LJqvahGTXywR2UA0C8K4pyA4zG8f0fZUxoJUA9Z5loY=; b=KHNt0ZYTqULn5UvzndOZKuO4vPSoA1W+0ZbU+Mgc709SALn/g/VHnqEngm6mM2uI6x 2olC7JHUj7pwMyh0WzZTriMVXs4N91VWcmVHAZm97xrf45PxrrRtltQ7RmtMdKrLCjtg Ngbk+bq+iXa7lLEpddWf+E8dwryllr3j29krW3pfVPxTvA/QNOhUxXEUFTMSTZxJ/nQu xUZYJZGhqLxRS/6SYe8ewO2UxYFGcB3n2MA5PG9CZ9E9zDvuv+MB/qd0HL3J8r7UWsh9 cJppz3Ioqhuw4lqOQcndlm3pZwqCjQ8ierplQwmxUNfw36/mukglfXPXYnzODWV40Lfq Xgrw== In-Reply-To: Xref: news.gmane.org gmane.linux.lib.musl.general:2091 Archived-At: --0016e6dedd83355a0d04cbe6aa7a Content-Type: text/plain; charset=ISO-8859-1 No, seriously, this time it's right! ;) On 10/12/12, Alex Caudill 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 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 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 >> > --0016e6dedd83355a0d04cbe6aa7a Content-Type: application/octet-stream; name="dynlink.patch" Content-Disposition: attachment; filename="dynlink.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: file1 ZGlmZiAtLWdpdCBhL3NyYy9sZHNvL2R5bmxpbmsuYyBiL3NyYy9sZHNvL2R5bmxpbmsuYwppbmRl eCBjM2NiNjExLi4wOWI0YjgyIDEwMDY0NAotLS0gYS9zcmMvbGRzby9keW5saW5rLmMKKysrIGIv c3JjL2xkc28vZHlubGluay5jCkBAIC0xMyw2ICsxMyw3IEBACiAjaW5jbHVkZSA8ZXJybm8uaD4K ICNpbmNsdWRlIDxsaW1pdHMuaD4KICNpbmNsdWRlIDxlbGYuaD4KKyNpbmNsdWRlIDxsaW5rLmg+ CiAjaW5jbHVkZSA8c2V0am1wLmg+CiAjaW5jbHVkZSA8cHRocmVhZC5oPgogI2luY2x1ZGUgPGN0 eXBlLmg+CkBAIC0zMCwxMiArMzEsMTQgQEAgc3RhdGljIGNoYXIgZXJyYnVmWzEyOF07CiB0eXBl ZGVmIEVsZjMyX0VoZHIgRWhkcjsKIHR5cGVkZWYgRWxmMzJfUGhkciBQaGRyOwogdHlwZWRlZiBF bGYzMl9TeW0gU3ltOwordHlwZWRlZiBFbGYzMl9BZGRyIEFkZHI7CiAjZGVmaW5lIFJfVFlQRSh4 KSAoKHgpJjI1NSkKICNkZWZpbmUgUl9TWU0oeCkgKCh4KT4+OCkKICNlbHNlCiB0eXBlZGVmIEVs ZjY0X0VoZHIgRWhkcjsKIHR5cGVkZWYgRWxmNjRfUGhkciBQaGRyOwogdHlwZWRlZiBFbGY2NF9T eW0gU3ltOwordHlwZWRlZiBFbGY2NF9BZGRyIEFkZHI7CiAjZGVmaW5lIFJfVFlQRSh4KSAoKHgp JjB4ZmZmZmZmZmYpCiAjZGVmaW5lIFJfU1lNKHgpICgoeCk+PjMyKQogI2VuZGlmCkBAIC01Nyw2 ICs2MCw4IEBAIHN0cnVjdCBkc28gewogCXNpemVfdCAqZHludjsKIAlzdHJ1Y3QgZHNvICpuZXh0 LCAqcHJldjsKIAorCVBoZHIgKnBoZHI7CisJdWludDE2X3QgcGhudW07CiAJaW50IHJlZmNudDsK IAlTeW0gKnN5bXM7CiAJdWludDMyX3QgKmhhc2h0YWI7CkBAIC05Miw2ICs5Nyw3IEBAIHZvaWQg Kl9faW5zdGFsbF9pbml0aWFsX3Rscyh2b2lkICopOwogCiBzdGF0aWMgc3RydWN0IGRzbyAqaGVh ZCwgKnRhaWwsICpsaWJjLCAqZmluaV9oZWFkOwogc3RhdGljIGNoYXIgKmVudl9wYXRoLCAqc3lz X3BhdGgsICpyX3BhdGg7CitzdGF0aWMgdW5zaWduZWQgbG9uZyBsb25nIGdlbmNudCA9IDA7CiBz dGF0aWMgaW50IHNzcF91c2VkOwogc3RhdGljIGludCBydW50aW1lOwogc3RhdGljIGludCBsZGRf bW9kZTsKQEAgLTMyNCw2ICszMzAsOCBAQCBzdGF0aWMgdm9pZCAqbWFwX2xpYnJhcnkoaW50IGZk LCBzdHJ1Y3QgZHNvICpkc28pCiAJCWVoLT5lX3Bob2ZmID0gc2l6ZW9mICplaDsKIAl9CiAJcGgg PSAodm9pZCAqKSgoY2hhciAqKWJ1ZiArIGVoLT5lX3Bob2ZmKTsKKwlkc28tPnBoZHIgPSBwaDsK Kwlkc28tPnBobnVtID0gKHVpbnQxNl90KWVoLT5lX3BobnVtOwogCWZvciAoaT1laC0+ZV9waG51 bTsgaTsgaS0tLCBwaD0odm9pZCAqKSgoY2hhciAqKXBoK2VoLT5lX3BoZW50c2l6ZSkpIHsKIAkJ aWYgKHBoLT5wX3R5cGUgPT0gUFRfRFlOQU1JQykKIAkJCWR5biA9IHBoLT5wX3ZhZGRyOwpAQCAt ODE1LDE4ICs4MjMsMTkgQEAgdm9pZCAqX19keW5saW5rKGludCBhcmdjLCBjaGFyICoqYXJndikK IAlsaWItPm5hbWUgPSBsaWItPnNob3J0bmFtZSA9ICJsaWJjLnNvIjsKIAlsaWItPmdsb2JhbCA9 IDE7CiAJZWhkciA9ICh2b2lkICopbGliLT5iYXNlOwotCWZpbmRfbWFwX3JhbmdlKCh2b2lkICop KGF1eFtBVF9CQVNFXStlaGRyLT5lX3Bob2ZmKSwKLQkJZWhkci0+ZV9waG51bSwgZWhkci0+ZV9w aGVudHNpemUsIGxpYik7Ci0JbGliLT5keW52ID0gKHZvaWQgKikobGliLT5iYXNlICsgZmluZF9k eW4oCi0JCSh2b2lkICopKGF1eFtBVF9CQVNFXStlaGRyLT5lX3Bob2ZmKSwKLQkJZWhkci0+ZV9w aG51bSwgZWhkci0+ZV9waGVudHNpemUpKTsKKwlsaWItPnBobnVtID0gKHVpbnQxNl90KWVoZHIt PmVfcGhudW07CisJbGliLT5waGRyID0gKHZvaWQgKikoYXV4W0FUX0JBU0VdK2VoZHItPmVfcGhv ZmYpOworCWZpbmRfbWFwX3JhbmdlKGxpYi0+cGhkciwgZWhkci0+ZV9waG51bSwgZWhkci0+ZV9w aGVudHNpemUsIGxpYik7CisJbGliLT5keW52ID0gKHZvaWQgKikobGliLT5iYXNlICsgZmluZF9k eW4obGliLT5waGRyLAorICAgICAgICAgICAgICAgICAgICBlaGRyLT5lX3BobnVtLCBlaGRyLT5l X3BoZW50c2l6ZSkpOwogCWRlY29kZV9keW4obGliKTsKIAogCWlmIChhdXhbQVRfUEhEUl0pIHsK IAkJc2l6ZV90IGludGVycF9vZmYgPSAwOwogCQlzaXplX3QgdGxzX2ltYWdlID0gMDsKIAkJLyog RmluZCBsb2FkIGFkZHJlc3Mgb2YgdGhlIG1haW4gcHJvZ3JhbSwgdmlhIEFUX1BIRFIgdnMgUFRf UEhEUi4gKi8KLQkJcGhkciA9ICh2b2lkICopYXV4W0FUX1BIRFJdOworCQlhcHAtPnBoZHIgPSBw aGRyID0gKHZvaWQgKilhdXhbQVRfUEhEUl07CisJCWFwcC0+cGhudW0gPSBhdXhbQVRfUEhOVU1d OwogCQlmb3IgKGk9YXV4W0FUX1BITlVNXTsgaTsgaS0tLCBwaGRyPSh2b2lkICopKChjaGFyICop cGhkciArIGF1eFtBVF9QSEVOVF0pKSB7CiAJCQlpZiAocGhkci0+cF90eXBlID09IFBUX1BIRFIp CiAJCQkJYXBwLT5iYXNlID0gKHZvaWQgKikoYXV4W0FUX1BIRFJdIC0gcGhkci0+cF92YWRkcik7 CkBAIC04OTAsNyArODk5LDggQEAgdm9pZCAqX19keW5saW5rKGludCBhcmdjLCBjaGFyICoqYXJn dikKIAkvKiBBdHRhY2ggdG8gdmRzbywgaWYgcHJvdmlkZWQgYnkgdGhlIGtlcm5lbCAqLwogCWlm IChzZWFyY2hfdmVjKGF1eHYsICZ2ZHNvX2Jhc2UsIEFUX1NZU0lORk9fRUhEUikpIHsKIAkJZWhk ciA9ICh2b2lkICopdmRzb19iYXNlOwotCQlwaGRyID0gKHZvaWQgKikodmRzb19iYXNlICsgZWhk ci0+ZV9waG9mZik7CisJCXZkc28tPnBoZHIgPSBwaGRyID0gKHZvaWQgKikodmRzb19iYXNlICsg ZWhkci0+ZV9waG9mZik7CisJCXZkc28tPnBobnVtID0gZWhkci0+ZV9waG51bTsKIAkJZm9yIChp PWVoZHItPmVfcGhudW07IGk7IGktLSwgcGhkcj0odm9pZCAqKSgoY2hhciAqKXBoZHIgKyBlaGRy LT5lX3BoZW50c2l6ZSkpIHsKIAkJCWlmIChwaGRyLT5wX3R5cGUgPT0gUFRfRFlOQU1JQykKIAkJ CQl2ZHNvLT5keW52ID0gKHZvaWQgKikodmRzb19iYXNlICsgcGhkci0+cF9vZmZzZXQpOwpAQCAt MTA1MSw2ICsxMDYxLDcgQEAgdm9pZCAqZGxvcGVuKGNvbnN0IGNoYXIgKmZpbGUsIGludCBtb2Rl KQogCW9yaWdfdGFpbCA9IHRhaWw7CiBlbmQ6CiAJX19yZWxlYXNlX3B0YygpOworCWlmIChlcnJm bGFnID09IDApIGdlbmNudCsrOwogCXB0aHJlYWRfcndsb2NrX3VubG9jaygmbG9jayk7CiAJaWYg KHApIGRvX2luaXRfZmluaShvcmlnX3RhaWwpOwogCXB0aHJlYWRfc2V0Y2FuY2Vsc3RhdGUoY3Ms IDApOwpAQCAtMTE2Niw2ICsxMTc3LDMzIEBAIHZvaWQgKl9fZGxzeW0odm9pZCAqcmVzdHJpY3Qg cCwgY29uc3QgY2hhciAqcmVzdHJpY3Qgcywgdm9pZCAqcmVzdHJpY3QgcmEpCiAJcHRocmVhZF9y d2xvY2tfdW5sb2NrKCZsb2NrKTsKIAlyZXR1cm4gcmVzOwogfQorCitpbnQgZGxfaXRlcmF0ZV9w aGRyKGludCgqY2FsbGJhY2spKHN0cnVjdCBkbF9waGRyX2luZm8gKmluZm8sIHNpemVfdCBzaXpl LCB2b2lkICpkYXRhKSwgdm9pZCAqZGF0YSkKK3sKKwlzdHJ1Y3QgZHNvICpjdXJyZW50OworCXN0 cnVjdCBkbF9waGRyX2luZm8gaW5mbzsKKwlpbnQgcmV0ID0gMDsKKwlmb3IoY3VycmVudCA9IGhl YWQ7IGN1cnJlbnQ7KSB7CisJCWluZm8uZGxwaV9hZGRyICAgICAgPSAoQWRkciljdXJyZW50LT5i YXNlOworCQlpbmZvLmRscGlfbmFtZSAgICAgID0gY3VycmVudC0+bmFtZTsKKwkJaW5mby5kbHBp X3BoZHIgICAgICA9IGN1cnJlbnQtPnBoZHI7CisJCWluZm8uZGxwaV9waG51bSAgICAgPSBjdXJy ZW50LT5waG51bTsKKwkJaW5mby5kbHBpX2FkZHMgICAgICA9IGdlbmNudDsKKwkJaW5mby5kbHBp X3N1YnMgICAgICA9IDA7CisJCWluZm8uZGxwaV90bHNfbW9kaWQgPSBjdXJyZW50LT50bHNfaWQ7 CisJCWluZm8uZGxwaV90bHNfZGF0YSAgPSBjdXJyZW50LT50bHNfaW1hZ2U7CisKKwkJcmV0ID0g KGNhbGxiYWNrKSgmaW5mbywgc2l6ZW9mIChpbmZvKSwgZGF0YSk7CisKKwkJaWYgKHJldCAhPSAw KSBicmVhazsKKworCQlwdGhyZWFkX3J3bG9ja19yZGxvY2soJmxvY2spOworCQlpZiAoY3VycmVu dCA9PSB0YWlsKSBicmVhazsKKwkJY3VycmVudCA9IGN1cnJlbnQtPm5leHQ7CisJCXB0aHJlYWRf cndsb2NrX3VubG9jaygmbG9jayk7CisJfQorCXJldHVybiByZXQ7Cit9CiAjZWxzZQogdm9pZCAq ZGxvcGVuKGNvbnN0IGNoYXIgKmZpbGUsIGludCBtb2RlKQogewo= --0016e6dedd83355a0d04cbe6aa7a--