Re: [PATCH] make relocation time symbol lookup and dlsym consistent

[Rich Felker <dalias@libc.org>]

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@port70.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