From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10881 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Reviving planned ldso changes Date: Wed, 4 Jan 2017 14:36:27 -0500 Message-ID: <20170104193627.GO1555@brightrain.aerifal.cx> References: <20170103054351.GA8459@brightrain.aerifal.cx> <20170104060640.GM1555@brightrain.aerifal.cx> <20170104062203.GN1555@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="QNDPHrPUIc00TOLW" X-Trace: blaine.gmane.org 1483558609 3818 195.159.176.226 (4 Jan 2017 19:36:49 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 4 Jan 2017 19:36:49 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-10894-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jan 04 20:36:45 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 1cOrMU-0008Cb-Pa for gllmg-musl@m.gmane.org; Wed, 04 Jan 2017 20:36:38 +0100 Original-Received: (qmail 17568 invoked by uid 550); 4 Jan 2017 19:36:41 -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 17544 invoked from network); 4 Jan 2017 19:36:40 -0000 Content-Disposition: inline In-Reply-To: <20170104062203.GN1555@brightrain.aerifal.cx> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:10881 Archived-At: --QNDPHrPUIc00TOLW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Jan 04, 2017 at 01:22:03AM -0500, Rich Felker wrote: > On Wed, Jan 04, 2017 at 01:06:40AM -0500, Rich Felker wrote: > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > > index c689084..cb82b2c 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; > > @@ -1211,13 +1212,14 @@ 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); > > + while (!p->constructed) { > > + while (p->deps[p->next_dep] && p->deps[p->next_dep]->constructed) > > + p->next_dep++; > > + if (p->deps[p->next_dep]) { > > + p = p->deps[p->next_dep++]; > > + continue; > > + } > > I think this logic is probably broken in the case of circular > dependencies, and will end up skipping ctors for some deps due to the > increment in this line: > > + p = p->deps[p->next_dep++]; > > which happens before the new p's ctors have actually run. Omitting the > increment here, however, would turn it into an infinite loop. We need > some way to detect this case and let the dso with an indirect > dependency on itself run its ctors once all non-circular deps have > been satisfied. I don't now the right algorithm for this right off, > though; suggestions would be welcome. 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 --QNDPHrPUIc00TOLW Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="ctor_dep_order_v2.diff" diff --git a/ldso/dynlink.c b/ldso/dynlink.c index c689084..fd59389 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; @@ -1090,9 +1091,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; @@ -1211,13 +1215,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) --QNDPHrPUIc00TOLW--