Re: Reviving planned ldso changes

[Rich Felker <dalias@libc.org>]

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

On Sat, Jan 14, 2017 at 03:30:50PM -0600, A. Wilcox wrote:
> On 04/01/17 13:36, Rich Felker wrote:
> > Here's a v2 of the patch with the above issues fixed, and some
> > comments that hopefully make it make sense. I still think there's more
> > logic needed to allow concurrent ctors from unrelated dlopen in
> > multiple threads, though.
> > 
> > Rich
> > 
> 
> 
> Applied to this to Adélie's musl package in a dev overlay and rebooted a
> box with this patch applied.
> 
> What a fantastic little show!
> 
> iv_tls_user_ptr: called on unregistered iv_tls_user
> /etc/init.d/syslog-ng: line 34:  2560 Aborted                 syslog-ng
> -s -f "${SYSLOG_NG_CONFIGFILE}"
>  * ERROR: syslog-ng failed to start
> 
> 
> When X tried to start up, further fireworks:
> 
> 
> /usr/bin/startkde: line 384:  2638 Segmentation fault      kwrapper5
> /usr/bin/ksmserver $KDEWM $KSMSERVEROPTIONS
> 
> 
> Starting program: /usr/bin/kwrapper5 /usr/bin/ksmserver
> process 3281 is executing new program: /usr/bin/ksmserver
> [New LWP 3287]
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff009938b in operator== (s1=..., s2=...) at tools/qstring.cpp:2686
> 2686    tools/qstring.cpp: No such file or directory.
> (gdb) bt
> #0  0x00007ffff009938b in operator== (s1=..., s2=...) at
> tools/qstring.cpp:2686
> #1  0x00007fffe2af2ae4 in operator!= (s2=..., s1=...) at
> /usr/include/qt5/QtCore/qstring.h:632
> #2  KHintsSettings::KHintsSettings (this=0x7fffe65829c0, kdeglobals=...)
> at
> /usr/src/kde-plasma/plasma-integration-5.7.5/work/plasma-integration-5.7.5/src/platformtheme/khintssettings.cpp:70
> 
> 
> Where khintssettings.cpp contains:
> 
> 68    const QString looknfeel = cg.readEntry("LookAndFeelPackage",
> defaultLookAndFeelPackage);
> 70    if (looknfeel != defaultLookAndFeelPackage) {
> 
> 
> And defaultLookAndFeelPackage is defined earlier in the source file as a
> constant:
> 
> static const QString defaultLookAndFeelPackage =
> QStringLiteral("org.kde.breeze.desktop");
> 
> 
> We can see that defaultLookAndFeelPackage was not initialised correctly:
> 
> (gdb) printqs5static looknfeel
> $9 = (Qt5 QString)0xffffdde0 length=22: "org.kde.breeze.desktop"
> (gdb) printqs5static defaultLookAndFeelPackage
> $10 = (Qt5 QString)0xe2d0be90 length=Cannot access memory at address 0x4
> 
> 
> It therefore seems to me that this patch still needs some refining.

Here's a v3 with a couple of issues fixed:

1. I failed to notice that do_init_fini needs to be called with a
pointer to the root of the (new part of) the dependency tree rather
than the tail of the dso list after the changes to its behavior. This
is now fixed.

2. The needed_by for libc.so itself was always null, causing tree
traversal to end immediately after visiting libc.so. It's now set to
the first dso that referenced it.

3. Likewise LD_PRELOAD dsos had a null needed_by. They're now treated
as being "needed by" the main app (as if they appeared in its
DT_NEEDED).

After these changes, your failing test case at
https://bpaste.net/raw/30ec06873fa2, code copied here:

------------------------------------------------------------------------
#include <iostream>

class NeedCXX
{
public:
  NeedCXX() { this->Foo = 1; }
  int GetFoo() { return this->Foo; }
private:
  int Foo;
};
int main()
{
  NeedCXX c;
  std::cout << c.GetFoo() << std::endl;
  return 0;
}
------------------------------------------------------------------------

seems to work as expected. I don't know if other bugs remain but at
least it seems plausible that it's working correctly now.

Rich

[-- Attachment #2: ctor_dep_order_v3.diff --]
[-- Type: text/plain, Size: 3427 bytes --]

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index a03f75e..3e52beb 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -67,6 +67,7 @@ struct dso {
 	char constructed;
 	char kernel_mapped;
 	struct dso **deps, *needed_by;
+	size_t next_dep;
 	char *rpath_orig, *rpath;
 	struct tls_module tls;
 	size_t tls_id;
@@ -934,6 +935,7 @@ static struct dso *load_library(const char *name, struct dso *needed_by)
 		if (!ldso.prev) {
 			tail->next = &ldso;
 			ldso.prev = tail;
+			ldso.needed_by = needed_by;
 			tail = ldso.next ? ldso.next : &ldso;
 		}
 		return &ldso;
@@ -1090,9 +1092,12 @@ static void load_deps(struct dso *p)
 				if (runtime) longjmp(*rtld_fail, 1);
 				continue;
 			}
-			if (runtime) {
-				tmp = realloc(*deps, sizeof(*tmp)*(ndeps+2));
-				if (!tmp) longjmp(*rtld_fail, 1);
+			tmp = realloc(*deps, sizeof(*tmp)*(ndeps+2));
+			if (!tmp) {
+				error("Error allocating dependency data for %s: %m",
+					p->name);
+				if (runtime) longjmp(*rtld_fail, 1);
+			} else {
 				tmp[ndeps++] = dep;
 				tmp[ndeps] = 0;
 				*deps = tmp;
@@ -1110,7 +1115,7 @@ static void load_preload(char *s)
 		for (z=s; *z && !isspace(*z) && *z!=':'; z++);
 		tmp = *z;
 		*z = 0;
-		load_library(s, 0);
+		load_library(s, head);
 		*z = tmp;
 	}
 }
@@ -1211,13 +1216,21 @@ void __libc_exit_fini()
 static void do_init_fini(struct dso *p)
 {
 	size_t dyn[DYN_CNT];
-	int need_locking = libc.threads_minus_1;
-	/* Allow recursive calls that arise when a library calls
-	 * dlopen from one of its constructors, but block any
-	 * other threads until all ctors have finished. */
-	if (need_locking) pthread_mutex_lock(&init_fini_lock);
-	for (; p; p=p->prev) {
-		if (p->constructed) continue;
+	pthread_mutex_lock(&init_fini_lock);
+	/* Construct in dependency order without any recursive state. */
+	while (p && !p->constructed) {
+		/* The following loop descends into the first dependency
+		 * that is neither alredy constructed nor pending
+		 * construction due to circular deps, stopping only
+		 * when it reaches a dso with no remaining dependencies
+		 * to descend into. */
+		while (p->deps && p->deps[p->next_dep]) {
+			if (!p->deps[p->next_dep]->constructed &&
+			    !p->deps[p->next_dep]->next_dep)
+				p = p->deps[p->next_dep++];
+			else
+				p->next_dep++;
+		}
 		p->constructed = 1;
 		decode_vec(p->dynv, dyn, DYN_CNT);
 		if (dyn[0] & ((1<<DT_FINI) | (1<<DT_FINI_ARRAY))) {
@@ -1233,17 +1246,19 @@ static void do_init_fini(struct dso *p)
 			size_t *fn = laddr(p, dyn[DT_INIT_ARRAY]);
 			while (n--) ((void (*)(void))*fn++)();
 		}
-		if (!need_locking && libc.threads_minus_1) {
-			need_locking = 1;
-			pthread_mutex_lock(&init_fini_lock);
-		}
-	}
-	if (need_locking) pthread_mutex_unlock(&init_fini_lock);
+		/* Revisit "parent" dso which caused the just-constructed
+		 * dso to be pulled in as a dependency. On the next loop
+		 * iteration we will either descend to construct a sibling
+		 * of the just-constructed dso, or finish constructing the
+		 * parent if no unfinished deps remain. */
+		p = p->needed_by;
+	}
+	pthread_mutex_unlock(&init_fini_lock);
 }
 
 void __libc_start_init(void)
 {
-	do_init_fini(tail);
+	do_init_fini(head);
 }
 
 static void dl_debug_state(void)
@@ -1731,7 +1746,7 @@ end:
 	__release_ptc();
 	if (p) gencnt++;
 	pthread_rwlock_unlock(&lock);
-	if (p) do_init_fini(orig_tail);
+	if (p) do_init_fini(p);
 	pthread_setcancelstate(cs, 0);
 	return p;
 }