Re: Reviving planned ldso changes

[Szabolcs Nagy <nsz@port70.net>]

* Rich Felker <dalias@libc.org> [2017-03-02 20:30:26 -0500]:
> Here's a v4 of the patch that saves the "init parent" we descended
> from so that it can return where it left off. There are a couple
> gratuitous hunks left over adding setting of "needed_by" where it made
> sense to be set, but it's not actually used anymore. They could be
> dropped if desired but are probably nice to keep for the sake of
> consistency of data, even thoough it's data we don't use.
> 
> I believe this can be extended to allow concurrent dlopen by amending
> the case in the tree-walk where a dependency isn't constructed yet but
> already has an "init parent" to check whether it's
> pending-construction in the calling thread (recursive dlopen from a
> ctor) or another thread; in the former case (as now) treat it as
> already-constructed; in the latter, wait on a condvar that gets
> signaled at the end of each construction, then continue the loop
> without advancing p. There are probably some subtleties I'm missing,
> though.
...
>  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]->init_parent) {
> +				p->deps[p->next_dep]->init_parent = p;
> +				p = p->deps[p->next_dep++];

i think the root may be visited twice because it
has no init_parent, which may be problematic with
the concurrent dlopen (and can cause unexpected
ctor order: the root node is not constructed last
if there is a cycle through it)

i think only checking init_parent of a dep is
enough and the root node can have a dummy parent
that is guaranteed to be not a dependency (ldso?)
and constructed so it stops the loop.

> +			} 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 +1248,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->init_parent;
> +	}
> +	pthread_mutex_unlock(&init_fini_lock);
>  }