diff mbox series

[RFC,v8,05/20] elf/dl-fini.c: Handle proxy link_map entries in the shutdown path

Message ID 20210209171839.7911-6-vivek@collabora.com
State Superseded
Delegated to: Adhemerval Zanella Netto
Headers
Series Implementation of RTLD_SHARED for dlmopen |

Commit Message

Vivek Dasmohapatra Feb. 9, 2021, 5:18 p.m. UTC 
  When cleaning up before exit we should not call destructors or
otherwise free [most of] the contents of proxied link_map entries
since they share [most of] their contents with the LM_ID_BASE
objects to which they point.
---
 elf/dl-close.c | 43 ++++++++++++++++++++++++++-----------------
 elf/dl-fini.c  |  6 ++++--
 2 files changed, 30 insertions(+), 19 deletions(-)

Comments

Adhemerval Zanella Netto Feb. 15, 2021, 5:08 p.m. UTC | #1 
On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote:
> When cleaning up before exit we should not call destructors or
> otherwise free [most of] the contents of proxied link_map entries
> since they share [most of] their contents with the LM_ID_BASE
> objects to which they point.
> ---
>  elf/dl-close.c | 43 ++++++++++++++++++++++++++-----------------
>  elf/dl-fini.c  |  6 ++++--
>  2 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/elf/dl-close.c b/elf/dl-close.c
> index c51becd06b..a0432b884d 100644
> --- a/elf/dl-close.c
> +++ b/elf/dl-close.c
> @@ -283,8 +283,9 @@ _dl_close_worker (struct link_map *map, bool force)
>  
>  	  /* Call its termination function.  Do not do it for
>  	     half-cooked objects.  Temporarily disable exception
> -	     handling, so that errors are fatal.  */
> -	  if (imap->l_init_called)
> +	     handling, so that errors are fatal.
> +	     Proxies should never have this flag set, but we double check.  */
> +	  if (imap->l_init_called && !imap->l_proxy)
>  	    {
>  	      /* When debugging print a message first.  */
>  	      if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,

Ok.

> @@ -360,7 +361,9 @@ _dl_close_worker (struct link_map *map, bool force)
>  	     one for the terminating NULL pointer.  */
>  	  size_t remain = (new_list != NULL) + 1;
>  	  bool removed_any = false;
> -	  for (size_t cnt = 0; imap->l_scope[cnt] != NULL; ++cnt)
> +	  for (size_t cnt = 0;
> +               imap->l_scope && imap->l_scope[cnt] != NULL;
> +               ++cnt)
>  	    /* This relies on l_scope[] entries being always set either
>  	       to its own l_symbolic_searchlist address, or some map's
>  	       l_searchlist address.  */

Ok.

> @@ -686,8 +689,10 @@ _dl_close_worker (struct link_map *map, bool force)
>  
>  	  /* We can unmap all the maps at once.  We determined the
>  	     start address and length when we loaded the object and
> -	     the `munmap' call does the rest.  */
> -	  DL_UNMAP (imap);
> +	     the `munmap' call does the rest. Proxies do not have
> +             any segments of their own to unmap.  */
> +          if (!imap->l_proxy)
> +            DL_UNMAP (imap);
>  
>  	  /* Finally, unlink the data structure and free it.  */

Ok.

>  #if DL_NNS == 1
> @@ -727,19 +732,23 @@ _dl_close_worker (struct link_map *map, bool force)
>  	    _dl_debug_printf ("\nfile=%s [%lu];  destroying link map\n",
>  			      imap->l_name, imap->l_ns);
>  
> -	  /* This name always is allocated.  */
> -	  free (imap->l_name);
> -	  /* Remove the list with all the names of the shared object.  */
> +          /* Skip structures borrowed by proxies from the real map.  */
> +          if (!imap->l_proxy)
> +            {
> +              /* This name always is allocated.  */
> +              free (imap->l_name);
> +              /* Remove the list with all the names of the shared object.  */
>  
> -	  struct libname_list *lnp = imap->l_libname;
> -	  do
> -	    {
> -	      struct libname_list *this = lnp;
> -	      lnp = lnp->next;
> -	      if (!this->dont_free)
> -		free (this);
> -	    }
> -	  while (lnp != NULL);
> +              struct libname_list *lnp = imap->l_libname;
> +              do
> +                {
> +                  struct libname_list *this = lnp;
> +                  lnp = lnp->next;
> +                  if (!this->dont_free)
> +                    free (this);
> +                }
> +              while (lnp != NULL);
> +            }
>  
>  	  /* Remove the searchlists.  */
>  	  free (imap->l_initfini);

Ok.

> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index 6dbdfe4b3e..10194488bb 100644
> --- a/elf/dl-fini.c
> +++ b/elf/dl-fini.c
> @@ -73,7 +73,7 @@ _dl_fini (void)
>  	  assert (nloaded != 0 || GL(dl_ns)[ns]._ns_loaded == NULL);
>  	  for (l = GL(dl_ns)[ns]._ns_loaded, i = 0; l != NULL; l = l->l_next)
>  	    /* Do not handle ld.so in secondary namespaces.  */
> -	    if (l == l->l_real)
> +	    if (l == l->l_real || l->l_proxy)
>  	      {
>  		assert (i < nloaded);
>  
> @@ -111,7 +111,9 @@ _dl_fini (void)
>  	    {
>  	      struct link_map *l = maps[i];
>  
> -	      if (l->l_init_called)
> +              /* Do not call fini functions via proxies, or for
> +                 objects which are not marked as initialised.  */
> +	      if (l->l_init_called && !l->l_proxy)
>  		{
>  		  /* Make sure nothing happens if we are called twice.  */
>  		  l->l_init_called = 0;
> 

Ok.
diff mbox series

Patch

diff --git a/elf/dl-close.c b/elf/dl-close.c
index c51becd06b..a0432b884d 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -283,8 +283,9 @@  _dl_close_worker (struct link_map *map, bool force)
 
 	  /* Call its termination function.  Do not do it for
 	     half-cooked objects.  Temporarily disable exception
-	     handling, so that errors are fatal.  */
-	  if (imap->l_init_called)
+	     handling, so that errors are fatal.
+	     Proxies should never have this flag set, but we double check.  */
+	  if (imap->l_init_called && !imap->l_proxy)
 	    {
 	      /* When debugging print a message first.  */
 	      if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_IMPCALLS,
@@ -360,7 +361,9 @@  _dl_close_worker (struct link_map *map, bool force)
 	     one for the terminating NULL pointer.  */
 	  size_t remain = (new_list != NULL) + 1;
 	  bool removed_any = false;
-	  for (size_t cnt = 0; imap->l_scope[cnt] != NULL; ++cnt)
+	  for (size_t cnt = 0;
+               imap->l_scope && imap->l_scope[cnt] != NULL;
+               ++cnt)
 	    /* This relies on l_scope[] entries being always set either
 	       to its own l_symbolic_searchlist address, or some map's
 	       l_searchlist address.  */
@@ -686,8 +689,10 @@  _dl_close_worker (struct link_map *map, bool force)
 
 	  /* We can unmap all the maps at once.  We determined the
 	     start address and length when we loaded the object and
-	     the `munmap' call does the rest.  */
-	  DL_UNMAP (imap);
+	     the `munmap' call does the rest. Proxies do not have
+             any segments of their own to unmap.  */
+          if (!imap->l_proxy)
+            DL_UNMAP (imap);
 
 	  /* Finally, unlink the data structure and free it.  */
 #if DL_NNS == 1
@@ -727,19 +732,23 @@  _dl_close_worker (struct link_map *map, bool force)
 	    _dl_debug_printf ("\nfile=%s [%lu];  destroying link map\n",
 			      imap->l_name, imap->l_ns);
 
-	  /* This name always is allocated.  */
-	  free (imap->l_name);
-	  /* Remove the list with all the names of the shared object.  */
+          /* Skip structures borrowed by proxies from the real map.  */
+          if (!imap->l_proxy)
+            {
+              /* This name always is allocated.  */
+              free (imap->l_name);
+              /* Remove the list with all the names of the shared object.  */
 
-	  struct libname_list *lnp = imap->l_libname;
-	  do
-	    {
-	      struct libname_list *this = lnp;
-	      lnp = lnp->next;
-	      if (!this->dont_free)
-		free (this);
-	    }
-	  while (lnp != NULL);
+              struct libname_list *lnp = imap->l_libname;
+              do
+                {
+                  struct libname_list *this = lnp;
+                  lnp = lnp->next;
+                  if (!this->dont_free)
+                    free (this);
+                }
+              while (lnp != NULL);
+            }
 
 	  /* Remove the searchlists.  */
 	  free (imap->l_initfini);
diff --git a/elf/dl-fini.c b/elf/dl-fini.c
index 6dbdfe4b3e..10194488bb 100644
--- a/elf/dl-fini.c
+++ b/elf/dl-fini.c
@@ -73,7 +73,7 @@  _dl_fini (void)
 	  assert (nloaded != 0 || GL(dl_ns)[ns]._ns_loaded == NULL);
 	  for (l = GL(dl_ns)[ns]._ns_loaded, i = 0; l != NULL; l = l->l_next)
 	    /* Do not handle ld.so in secondary namespaces.  */
-	    if (l == l->l_real)
+	    if (l == l->l_real || l->l_proxy)
 	      {
 		assert (i < nloaded);
 
@@ -111,7 +111,9 @@  _dl_fini (void)
 	    {
 	      struct link_map *l = maps[i];
 
-	      if (l->l_init_called)
+              /* Do not call fini functions via proxies, or for
+                 objects which are not marked as initialised.  */
+	      if (l->l_init_called && !l->l_proxy)
 		{
 		  /* Make sure nothing happens if we are called twice.  */
 		  l->l_init_called = 0;