[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20190813150102.GE9017@brightrain.aerifal.cx>
Date: Tue, 13 Aug 2019 11:01:02 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] make relocation time symbol lookup and dlsym
consistent
On Tue, Aug 13, 2019 at 10:24:24AM -0400, Rich Felker wrote:
> On Tue, Aug 13, 2019 at 10:38:44AM +0200, Szabolcs Nagy wrote:
> > * Szabolcs Nagy <nsz@...t70.net> [2019-08-11 01:18:11 +0200]:
> > > static void *do_dlsym(struct dso *p, const char *s, void *ra)
> > > {
> > > - size_t i;
> > > - uint32_t h = 0, gh = 0, *ght;
> > > - Sym *sym;
> > > - if (p == head || p == RTLD_DEFAULT || p == RTLD_NEXT) {
> > > - if (p == RTLD_DEFAULT) {
> > > - p = head;
> > > - } else if (p == RTLD_NEXT) {
> > > - p = addr2dso((size_t)ra);
> > > - if (!p) p=head;
> > > - p = p->next;
> > > - }
> > > - struct symdef def = find_sym(p, s, 0);
> > > - if (!def.sym) goto failed;
> > > - if ((def.sym->st_info&0xf) == STT_TLS)
> > > - return __tls_get_addr((tls_mod_off_t []){def.dso->tls_id, def.sym->st_value-DTP_OFFSET});
> > > - if (DL_FDPIC && (def.sym->st_info&0xf) == STT_FUNC)
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > - return def.dso->funcdescs + (def.sym - def.dso->syms);
> > > - return laddr(def.dso, def.sym->st_value);
> > > - }
> > > - if (__dl_invalid_handle(p))
> > > + int use_deps = 0;
> > > + if (p == head || p == RTLD_DEFAULT) {
> > > + p = head;
> > > + } else if (p == RTLD_NEXT) {
> > > + p = addr2dso((size_t)ra);
> > > + if (!p) p=head;
> > > + p = p->next;
> > > + } else if (__dl_invalid_handle(p)) {
> > > return 0;
> > > - if ((ght = p->ghashtab)) {
> > > - gh = gnu_hash(s);
> > > - sym = gnu_lookup(gh, ght, p, s);
> > > - } else {
> > > - h = sysv_hash(s);
> > > - sym = sysv_lookup(s, h, p);
> > > - }
> > > - if (sym && (sym->st_info&0xf) == STT_TLS)
> > > - return __tls_get_addr((tls_mod_off_t []){p->tls_id, sym->st_value-DTP_OFFSET});
> > > - if (DL_FDPIC && sym && sym->st_shndx && (sym->st_info&0xf) == STT_FUNC)
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > - return p->funcdescs + (sym - p->syms);
> > > - if (sym && sym->st_value && (1<<(sym->st_info&0xf) & OK_TYPES))
> > > - return laddr(p, sym->st_value);
> > > - for (i=0; p->deps[i]; i++) {
> > > - if ((ght = p->deps[i]->ghashtab)) {
> > > - if (!gh) gh = gnu_hash(s);
> > > - sym = gnu_lookup(gh, ght, p->deps[i], s);
> > > - } else {
> > > - if (!h) h = sysv_hash(s);
> > > - sym = sysv_lookup(s, h, p->deps[i]);
> > > - }
> > > - if (sym && (sym->st_info&0xf) == STT_TLS)
> > > - return __tls_get_addr((tls_mod_off_t []){p->deps[i]->tls_id, sym->st_value-DTP_OFFSET});
> > > - if (DL_FDPIC && sym && sym->st_shndx && (sym->st_info&0xf) == STT_FUNC)
> > > - return p->deps[i]->funcdescs + (sym - p->deps[i]->syms);
> > > - if (sym && sym->st_value && (1<<(sym->st_info&0xf) & OK_TYPES))
> > > - return laddr(p->deps[i], sym->st_value);
> > > - }
> > > -failed:
> > > - error("Symbol not found: %s", s);
> > > - return 0;
> > > + } else
> > > + use_deps = 1;
> > > + struct symdef def = find_sym2(p, s, 0, use_deps);
> > > + if (!def.sym) {
> > > + error("Symbol not found: %s", s);
> > > + return 0;
> > > + }
> > > + if ((def.sym->st_info&0xf) == STT_TLS)
> > > + return __tls_get_addr((tls_mod_off_t []){def.dso->tls_id, def.sym->st_value-DTP_OFFSET});
> > > + if (DL_FDPIC && (def.sym->st_info&0xf) == STT_FUNC)
> > > + return def.dso->funcdescs + (def.sym - def.dso->syms);
> >
> > there is another behaviour change i did not notice before:
> >
> > st_shndx is no longer checked in DL_FDPIC case here, i assumed
> > find_sym did that, but there is no fdpic specific logic there.
> >
> > the old code was inconsistent in the RTLD_DEFAULT vs shared lib
> > dlsym case.
> >
> > i dont know if this is relevant for fdpic, i didnt test that case.
>
> Thanks for catching. I'll take a look at this.
commit d47d9a50f2568927af51e21b2f2120409db1ab44 documented that the
logic probably isn't entirely correct.
It looks to me like st_shndx being 0/undef here was being used as a
proxy for the symbol being completely undefined (just a reference),
which is normally (on non-fdpic) determined by the value being 0;
nonzero value but 0/undef section means PLT thunk or copy relocation.
The code in question only applies to functions, not data, and fdpic
does not have PLT thunks that provide definitions since the function
definition is always the *descriptor*, whose existence ldso is
responsible for managing.
So, I think your new code is okay as-is. Let me know if any of this
sounds wrong or even dubious.
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.
