Re: Reviving planned ldso changes

[Szabolcs Nagy <nsz@port70.net>]

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

* Szabolcs Nagy <nsz@port70.net> [2017-01-04 11:51:09 +0100]:
> * Rich Felker <dalias@libc.org> [2017-01-03 00:43:51 -0500]:
> > One item that's been on the agenda for a long time (maybe 1.5 years
> > now?) is planned dynamic linker improvements. The thread "Further
> > dynamic linker optimizations" from summer 2015 covered parts but maybe
> > not all of what I had in mind.
...
> - symbol lookup logic is duplicated between do_dlsym and find_sym,
>   it would be nice to do that with common code path if possible.

i looked into this and found some issues

i attached my current patches for comments, but the bugs may be
fixed separately from the refactoring.

[-- Attachment #2: 0001-treat-STB_WEAK-and-STB_GNU_UNIQUE-like-STB_GLOBAL-in.patch --]
[-- Type: text/x-diff, Size: 1447 bytes --]

From 5dd896dcdf53e50fc2d7c1957d62df7b8930ff01 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Sat, 3 Dec 2016 20:52:43 +0000
Subject: [PATCH 1/2] treat STB_WEAK and STB_GNU_UNIQUE like STB_GLOBAL in
 find_sym

A weak symbol definition is not special during dynamic linking, so
don't let a strong definition in a later module override it.
(glibc dynamic linker allows overriding weak definitions if
LD_DYNAMIC_WEAK is set, musl does not.)

STB_GNU_UNIQUE means that the symbol is global, even if it is in a
module that's loaded with RTLD_LOCAL, and all references resolve to
the same definition. This semantics is only relevant for c++ plugin
systems and even there it's often not what the user wants (so it can
be turned off in g++ by -fno-gnu-unique when the c++ shared lib is
compiled). In musl just treat it like STB_GLOBAL.
---
 ldso/dynlink.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index c6890845..476cd6a8 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -286,11 +286,9 @@ static struct symdef find_sym(struct dso *dso, const char *s, int need_def)
 				continue;
 		if (!(1<<(sym->st_info&0xf) & OK_TYPES)) continue;
 		if (!(1<<(sym->st_info>>4) & OK_BINDS)) continue;
-
-		if (def.sym && sym->st_info>>4 == STB_WEAK) continue;
 		def.sym = sym;
 		def.dso = dso;
-		if (sym->st_info>>4 == STB_GLOBAL) break;
+		break;
 	}
 	return def;
 }
-- 
2.11.0


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

From b5e20dbf30e67a12bc0931f12126553d036bbd31 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Sat, 3 Dec 2016 23:08:44 +0000
Subject: [PATCH 2/2] 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).

TODO: not benchmarked, common code should be inlined probably.
---
 ldso/dynlink.c | 83 +++++++++++++++++++---------------------------------------
 1 file changed, 27 insertions(+), 56 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 476cd6a8..e5a382b0 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -256,14 +256,15 @@ 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)
+static struct symdef find_sym(struct dso *dso, const char *s, int need_def, int use_deps)
 {
 	uint32_t h = 0, gh, gho, *ght;
 	size_t ghm = 0;
 	struct symdef def = {0};
-	for (; dso; dso=dso->next) {
+	struct dso **deps = use_deps ? dso->deps : 0;
+	for (; dso; dso=use_deps ? *deps++ : dso->next) {
 		Sym *sym;
-		if (!dso->global) continue;
+		if (!dso->global && !use_deps) continue;
 		if ((ght = dso->ghashtab)) {
 			if (!ghm) {
 				gh = gnu_hash(s);
@@ -332,7 +333,7 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
 			ctx = type==REL_COPY ? head->next : head;
 			def = (sym->st_info&0xf) == STT_SECTION
 				? (struct symdef){ .dso = dso, .sym = sym }
-				: find_sym(ctx, name, type==REL_PLT);
+				: find_sym(ctx, name, type==REL_PLT, 0);
 			if (!def.sym && (sym->st_shndx != SHN_UNDEF
 			    || sym->st_info>>4 != STB_WEAK)) {
 				error("Error relocating %s: %s: symbol not found",
@@ -1377,7 +1378,7 @@ void __dls2(unsigned char *base, size_t *sp)
 	/* Call dynamic linker stage-3, __dls3, looking it up
 	 * symbolically as a barrier against moving the address
 	 * load across the above relocation processing. */
-	struct symdef dls3_def = find_sym(&ldso, "__dls3", 0);
+	struct symdef dls3_def = find_sym(&ldso, "__dls3", 0, 0);
 	if (DL_FDPIC) ((stage3_func)&ldso.funcdescs[dls3_def.sym-ldso.syms])(sp);
 	else ((stage3_func)laddr(&ldso, dls3_def.sym->st_value))(sp);
 }
@@ -1770,58 +1771,28 @@ void *__tls_get_addr(size_t *);
 
 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((size_t []){def.dso->tls_id, def.sym->st_value});
-		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))
+	struct symdef def;
+	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((size_t []){p->tls_id, sym->st_value});
-	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);
-	if (p->deps) 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((size_t []){p->deps[i]->tls_id, sym->st_value});
-		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;
+	def = find_sym(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((size_t []){def.dso->tls_id, def.sym->st_value});
+	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, Dl_info *info)
-- 
2.11.0