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

Message ID 20180516171124.24962-6-vivek@collabora.com
State New
Headers show
Series
  • Proof-of-Concept implementation of RTLD_SHARED for dlmopen
Related show

Commit Message

Vivek Das Mohapatra May 16, 2018, 5:11 p.m.
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.

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(+)

Comments

Carlos O'Donell May 18, 2018, 7:09 p.m. | #1
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.
Vivek Das Mohapatra May 18, 2018, 7:25 p.m. | #2
>> --- 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.

I think it was there originally but when I was building the
full suite (including the static variant) gcc got upset at me
about the bounds checking of GL(dl_ns)[…]. I'll revisit it
and see if I can figure out what the problem was.

>> +      /* 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.

Hm. Ok, I'll get on that.

>> +
>>        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.

Right. I will work through this and the other changes, hopefully a new patchset
should land next week.

Patch

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 */
+  struct link_map *clone;
+  struct link_map *next;
+
+  if (ns < 0 || ns >= DL_NNS)
+    return;
+
+  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
+}
 
 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);
+
       unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
       /* No need to do anything for empty namespaces or those used for
 	 auditing DSOs.  */