From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10932 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Reviving planned ldso changes Date: Sun, 15 Jan 2017 12:44:38 -0500 Message-ID: <20170115174438.GD1533@brightrain.aerifal.cx> References: <20170103054351.GA8459@brightrain.aerifal.cx> <20170104060640.GM1555@brightrain.aerifal.cx> <20170104062203.GN1555@brightrain.aerifal.cx> <20170104193627.GO1555@brightrain.aerifal.cx> <587A988A.50105@Wilcox-Tech.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="OgqxwSJOaUobr8KG" Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1484502300 28649 195.159.176.226 (15 Jan 2017 17:45:00 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 15 Jan 2017 17:45:00 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-10945-gllmg-musl=m.gmane.org@lists.openwall.com Sun Jan 15 18:44:55 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1cSorL-0006oj-FW for gllmg-musl@m.gmane.org; Sun, 15 Jan 2017 18:44:51 +0100 Original-Received: (qmail 10157 invoked by uid 550); 15 Jan 2017 17:44:53 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 10139 invoked from network); 15 Jan 2017 17:44:52 -0000 Content-Disposition: inline In-Reply-To: <587A988A.50105@Wilcox-Tech.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:10932 Archived-At: --OgqxwSJOaUobr8KG Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 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 --OgqxwSJOaUobr8KG Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="ctor_dep_order_v3.diff" 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<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; } --OgqxwSJOaUobr8KG--