Re: [RFC][PATCH v1 5/5] elf/dl-fini.c: Handle cloned link_map entries in the shutdown path

[Carlos O'Donell <carlos@redhat.com>]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> When cleaning up before exit we should not call destructors or
> otherwise free [most of] the contents of a cloned link_map entry
> since they share [most of] their contents with the LM_ID_BASE
> object from which they were cloned.

s/clones/proxies/g

> Instead we do the minimal cleanup necessary and remove them from
> the namespace linked list(s) before the main cleanup code path
> is triggered: This prevemts double-frees and double-invocation
> of any destructors (which might not be idempotent).
> ---
>  elf/dl-fini.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index 3cfc262400..643a68504e 100644
> --- a/elf/dl-fini.c
> +++ b/elf/dl-fini.c
> @@ -24,6 +24,50 @@
>  /* Type of the constructor functions.  */
>  typedef void (*fini_t) (void);
>  
> +/* Remove (and free) cloned entries from the namespace specifid by `ns'.  */
> +void
> +_dl_forget_clones (Lmid_t ns)
> +{
> +#ifdef SHARED /* Obviously none of this applies if */

Move the #ifdef up to the caller please.

> +  struct link_map *clone;
> +  struct link_map *next;
> +
> +  if (ns < 0 || ns >= DL_NNS)
> +    return;

Make this an assert. It is an internal error to have an invalid ns at 
this point. We should not return and ignore this.

> +
> +  for (clone = GL(dl_ns)[ns]._ns_loaded; clone != NULL; clone = next)
> +    {
> +      next = clone->l_next;
> +
> +      /* Not actually clone, don't care.  */
> +      if (!clone->l_clone)
> +        continue;
> +
> +      if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS))
> +        _dl_debug_printf ("\nclone removed %s [%lu]\n", clone->l_name, ns);
> +
> +      /* If item is a clone, slice it out of the linked list.  */
> +      if (clone == GL(dl_ns)[ns]._ns_loaded)
> +        GL(dl_ns)[ns]._ns_loaded = clone->l_next;
> +      else
> +        clone->l_prev->l_next = clone->l_next;
> +
> +      /* Remember to fix up the links in both directions.  */
> +      if (clone->l_next)
> +        clone->l_next->l_prev = clone->l_prev;
> +
> +      /* Don't need to do most destructor handling for clones.  */
> +      if (clone->l_free_initfini)
> +        free (clone->l_initfini);
> +
> +      /* Do need to fix the global load count which is updated in
> +         _dl_add_to_namespace_list.  */
> +      GL(dl_ns)[ns]._ns_nloaded--;
> +
> +      free (clone);
> +    }
> +#endif
> +}

OK.
  
>  void
>  _dl_fini (void)
> @@ -52,6 +96,12 @@ _dl_fini (void)
>        /* Protect against concurrent loads and unloads.  */
>        __rtld_lock_lock_recursive (GL(dl_load_lock));
>  
> +      /* We need to remove any pointers to cloned entries (link_map
> +         structs that are copied from one namespace to another to
> +         implement RTLD_SHARED semantics) before the regular cleanup
> +         code gets to them.  */
> +      _dl_forget_clones (ns);

While this is easy to implement like this, I think we are going to run
into use cases where this doesn't work.

Consider this test case:

- Only a fini in a library.
- Lazy binding.
- fini calls a libc.so.6 function for the first time.
- When fini is called we jump into ld.so to find the symbol to call,
  but the libc.so.6 proxy has been removed, and so we can no longer
  resolve calls to it.

You will, IMO, have to remove the proxies at the point the library
would have been unloaded normally from the namespace, but avoid doing
all the other work.

> +
>        unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
>        /* No need to do anything for empty namespaces or those used for
>  	 auditing DSOs.  */
> 

In summary I think we need a v2 of this patch which implements a more complex
free-at-unload support.

-- 
Cheers,
Carlos.