make relocation time symbol lookup and dlsym consistent

Submitted by Szabolcs Nagy on Aug. 10, 2019, 11:18 p.m.

Details

Message ID 20190810231811.GK22009@port70.net
State New
Series "make relocation time symbol lookup and dlsym consistent"
Headers show

Commit Message

Szabolcs Nagy Aug. 10, 2019, 11:18 p.m.
rebased the old patch.

changed a bit to avoid performance concerns.

Patch hide | download patch | download mbox

From a57cd35acf26ba6202ed6534a57f496464f431a1 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Sat, 10 Aug 2019 23:14:40 +0000
Subject: [PATCH] make relocation time symbol lookup and dlsym consistent

Using common code path for all symbol lookups fixes three dlsym issues:

- st_shndx of STT_TLS symbols were not checked and thus an undefined
  tls symbol reference could be incorrectly treated as a definition
  (the sysv hash lookup returns undefined symbols, gnu does not, so should
  be rare in practice).

- symbol binding was not checked so a hidden symbol may be returned
  (in principle STB_LOCAL symbols may appear in the dynamic symbol table
  for hidden symbols, but linkers most likely don't produce it).

- mips specific behaviour was not applied (ARCH_SYM_REJECT_UND) so
  undefined symbols may be returned on mips.

always_inline is used to avoid relocation performance regression, the
code generation for find_sym should not be affected.
---
 ldso/dynlink.c | 84 +++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 53 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index d1edb131..07871869 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -283,12 +283,16 @@  static Sym *gnu_lookup_filtered(uint32_t h1, uint32_t *hashtab, struct dso *dso,
 #define ARCH_SYM_REJECT_UND(s) 0
 #endif
 
-static struct symdef find_sym(struct dso *dso, const char *s, int need_def)
+#if defined(__GNUC__)
+__attribute__((always_inline))
+#endif
+static inline struct symdef find_sym2(struct dso *dso, const char *s, int need_def, int use_deps)
 {
 	uint32_t h = 0, gh = gnu_hash(s), gho = gh / (8*sizeof(size_t)), *ght;
 	size_t ghm = 1ul << gh % (8*sizeof(size_t));
 	struct symdef def = {0};
-	for (; dso; dso=dso->syms_next) {
+	struct dso **deps = use_deps ? dso->deps : 0;
+	for (; dso; dso=use_deps ? *deps++ : dso->syms_next) {
 		Sym *sym;
 		if ((ght = dso->ghashtab)) {
 			sym = gnu_lookup_filtered(gh, ght, dso, s, gho, ghm);
@@ -313,6 +317,11 @@  static struct symdef find_sym(struct dso *dso, const char *s, int need_def)
 	return def;
 }
 
+static struct symdef find_sym(struct dso *dso, const char *s, int need_def)
+{
+	return find_sym2(dso, s, need_def, 0);
+}
+
 static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stride)
 {
 	unsigned char *base = dso->base;
@@ -2118,58 +2127,27 @@  static void *addr2dso(size_t a)
 
 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);
+	return laddr(def.dso, def.sym->st_value);
 }
 
 int dladdr(const void *addr_arg, Dl_info *info)
-- 
2.21.0


Comments

Rich Felker Aug. 12, 2019, 10:26 p.m.
On Sun, Aug 11, 2019 at 01:18:11AM +0200, Szabolcs Nagy wrote:
> rebased the old patch.
> 
> changed a bit to avoid performance concerns.

> >From a57cd35acf26ba6202ed6534a57f496464f431a1 Mon Sep 17 00:00:00 2001
> From: Szabolcs Nagy <nsz@port70.net>
> Date: Sat, 10 Aug 2019 23:14:40 +0000
> Subject: [PATCH] make relocation time symbol lookup and dlsym consistent
> 
> Using common code path for all symbol lookups fixes three dlsym issues:
> 
> - st_shndx of STT_TLS symbols were not checked and thus an undefined
>   tls symbol reference could be incorrectly treated as a definition
>   (the sysv hash lookup returns undefined symbols, gnu does not, so should
>   be rare in practice).
> 
> - symbol binding was not checked so a hidden symbol may be returned
>   (in principle STB_LOCAL symbols may appear in the dynamic symbol table
>   for hidden symbols, but linkers most likely don't produce it).
> 
> - mips specific behaviour was not applied (ARCH_SYM_REJECT_UND) so
>   undefined symbols may be returned on mips.
> 
> always_inline is used to avoid relocation performance regression, the
> code generation for find_sym should not be affected.
> ---
>  ldso/dynlink.c | 84 +++++++++++++++++++-------------------------------
>  1 file changed, 31 insertions(+), 53 deletions(-)

Thanks for rebasing this! Applying.

Rich
Szabolcs Nagy Aug. 13, 2019, 8:38 a.m.
* 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.


> +	return laddr(def.dso, def.sym->st_value);
>  }
>  
>  int dladdr(const void *addr_arg, Dl_info *info)
> -- 
> 2.21.0
>
Rich Felker Aug. 13, 2019, 2:24 p.m.
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.

Rich
Rich Felker Aug. 13, 2019, 3:01 p.m.
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