Re: [RFC][PATCH v1 3/5] elf/dl-object.c: Implement a helper function to clone link_map entries

[Carlos O'Donell <carlos at redhat dot com>]

On 05/16/2018 01:11 PM, Vivek Das Mohapatra wrote:
> From: Vivek Das Mohapatra <vivek@collabora.co.uk>
> 
> Provides the minimal functionality needed to take an existing
> link_map entry and create a clone of it in the specified namespace.
> ---
>  elf/dl-object.c            | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>  sysdeps/generic/ldsodefs.h |  6 ++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/elf/dl-object.c b/elf/dl-object.c
> index b37bcc1295..144ba6c993 100644
> --- a/elf/dl-object.c
> +++ b/elf/dl-object.c
> @@ -21,6 +21,7 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <ldsodefs.h>
> +#include <libintl.h>
>  
>  #include <assert.h>
>  
> @@ -50,6 +51,82 @@ _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
>    __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
>  }
>  
> +/* Clone an existing link map entry into a new link map.  */

Existing comment about the use of "clone" apply here too.

> +/* This is based on _dl_new_object, skipping the steps we know we won't need

Both of these comments should be merged into one.

> +   because this is mostly just a shell for the l_real pointer holding the real
> +   link map entry (normally l == l->l_real, but not for ld.so in non-main
> +   link maps or RTLD_SHARED clones).
> +   It also flags the clone by setting l_clone, and sets the the no-delete
> +   flag in the original.  */
> +struct link_map *
> +_dl_clone_object (struct link_map *old, int mode, Lmid_t nsid)
> +{
> +  const char *name;
> +  struct link_map *new;
> +  struct libname_list *newname;
> +#ifdef SHARED
> +  /* See _dl_new_object for how this number is arrived at:  */
> +  unsigned int na = GLRO(dl_naudit) ?: ((mode & __RTLD_OPENEXEC) ? DL_NNS : 0);

This is too complex. It should be expanded out for readability, and likely so anywhere
else you see it, but let's start by making this copy easy to read.

> +  size_t audit_space = na * sizeof (new->l_audit[0]);
> +#else
> +# define audit_space 0
> +#endif
> +
> +  name = old->l_name;
> +
> +  /* Don't clone a clone: Go to the progenitor.  */

I think we'd probably say "Find the original link map from the proxy."

> +  while (old && old->l_clone)
> +    old = old->l_real;
> +
> +  if (old == NULL)
> +    _dl_signal_error (EINVAL, name, NULL, N_("cannot clone NULL link_map"));
> +
> +  /* Object already exists in the target namespace. This should get handled
> +     by dl_open_worker but just in case we get this far, handle it:  */
> +  if (__glibc_unlikely (old->l_ns == nsid))
> +    {
> +      /* Not actually possible, given the sanity checks above.  */
> +      if (old->l_clone)
> +        return old;
> +
> +      _dl_signal_error (EEXIST, name, NULL,
> +                        N_("object cannot be demoted to a clone"));
> +    }
> +
> +  /* Now duplicate as little of _dl_new_object as possible to get a
> +     working cloned object in the target link map.  */
> +  new = (struct link_map *) calloc (sizeof (*new) + audit_space
> +                                    + sizeof (struct link_map *)
> +                                    + sizeof (*newname) + PATH_MAX, 1);

calloc could fail, you need to _dl_signal_error ENOMEM.

> +
> +  /* Specific to the clone.  */
> +  new->l_real = old;
> +  new->l_clone = 1;
> +  new->l_ns = nsid;
> +
> +  /* Copied from the origin.  */
> +  new->l_libname = old->l_libname;
> +  new->l_name = old->l_name;
> +  new->l_type = old->l_type;
> +
> +  if (__glibc_unlikely (mode & RTLD_NODELETE))
> +    new->l_flags_1 |= DF_1_NODELETE;
> +
> +  /* Specific to the origin. Ideally we'd do some accounting here but
> +     for now it's easier to pin the original so the clone remains valid.  */
> +  old->l_flags_1 |= DF_1_NODELETE;
> +
> +  /* Fix up the searchlist so that relocations work.  */
> +  /* TODO: figure out if we should call _dl_map_object_deps
> +     or copy the contents of l_scope, l_searchlist et al.  */
> +  _dl_map_object_deps (new, NULL, 0, 0,
> +		       mode & (__RTLD_DLOPEN | RTLD_DEEPBIND | __RTLD_AUDIT));

Calling _dl_map_object_deps seems like a conservative thing to do here.

> +
> +  /* And finally put the clone into the target namespace.  */
> +  _dl_add_to_namespace_list (new, nsid);
> +
> +  return new;
> +}
>  
>  /* Allocate a `struct link_map' for a new object being loaded,
>     and enter it into the _dl_loaded list.  */
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 5e1b24ecb5..573d24f611 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -913,6 +913,12 @@ extern lookup_t _dl_lookup_symbol_x (const char *undef,
>  extern void _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
>       attribute_hidden;
>  
> +/* Clone an existing link map entry into a new link map */
> +extern struct link_map *_dl_clone_object (struct link_map *old,
> +                                          int mode,
> +                                          Lmid_t nsid)
> +     attribute_hidden;

OK.

But I think we'd call this _dl_proxy_object (...);

> +
>  /* Allocate a `struct link_map' for a new object being loaded.  */
>  extern struct link_map *_dl_new_object (char *realname, const char *libname,
>  					int type, struct link_map *loader,
> 


-- 
Cheers,
Carlos.