mailing list of musl libc
 help / color / mirror / code / Atom feed
* [PATCH] make relocation time symbol lookup and dlsym consistent
@ 2019-08-10 23:18 Szabolcs Nagy
  2019-08-12 22:26 ` Rich Felker
  2019-08-13  8:38 ` Szabolcs Nagy
  0 siblings, 2 replies; 5+ messages in thread
From: Szabolcs Nagy @ 2019-08-10 23:18 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 69 bytes --]

rebased the old patch.

changed a bit to avoid performance concerns.

[-- Attachment #2: 0001-make-relocation-time-symbol-lookup-and-dlsym-consist.patch --]
[-- Type: text/x-diff, Size: 5055 bytes --]

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] make relocation time symbol lookup and dlsym consistent
  2019-08-10 23:18 [PATCH] make relocation time symbol lookup and dlsym consistent Szabolcs Nagy
@ 2019-08-12 22:26 ` Rich Felker
  2019-08-13  8:38 ` Szabolcs Nagy
  1 sibling, 0 replies; 5+ messages in thread
From: Rich Felker @ 2019-08-12 22:26 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] make relocation time symbol lookup and dlsym consistent
  2019-08-10 23:18 [PATCH] make relocation time symbol lookup and dlsym consistent Szabolcs Nagy
  2019-08-12 22:26 ` Rich Felker
@ 2019-08-13  8:38 ` Szabolcs Nagy
  2019-08-13 14:24   ` Rich Felker
  1 sibling, 1 reply; 5+ messages in thread
From: Szabolcs Nagy @ 2019-08-13  8:38 UTC (permalink / raw)
  To: musl

* 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
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] make relocation time symbol lookup and dlsym consistent
  2019-08-13  8:38 ` Szabolcs Nagy
@ 2019-08-13 14:24   ` Rich Felker
  2019-08-13 15:01     ` Rich Felker
  0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2019-08-13 14:24 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] make relocation time symbol lookup and dlsym consistent
  2019-08-13 14:24   ` Rich Felker
@ 2019-08-13 15:01     ` Rich Felker
  0 siblings, 0 replies; 5+ messages in thread
From: Rich Felker @ 2019-08-13 15:01 UTC (permalink / raw)
  To: musl

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-08-13 15:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-10 23:18 [PATCH] make relocation time symbol lookup and dlsym consistent Szabolcs Nagy
2019-08-12 22:26 ` Rich Felker
2019-08-13  8:38 ` Szabolcs Nagy
2019-08-13 14:24   ` Rich Felker
2019-08-13 15:01     ` Rich Felker

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).