diff mbox series

[RFC,v8,12/20] Use the new DSO finder helper function since we have it

Message ID 20210209171839.7911-13-vivek@collabora.com
State Changes Requested
Delegated to: Adhemerval Zanella Netto
Headers show
Series Implementation of RTLD_SHARED for dlmopen | expand

Commit Message

Vivek Das Mohapatra Feb. 9, 2021, 5:18 p.m. UTC
---
 elf/dl-load.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

Comments

Adhemerval Zanella Feb. 19, 2021, 2:39 p.m. UTC | #1
On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote:
> ---
>  elf/dl-load.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 780bca99e8..13ac2053b8 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -2133,35 +2133,26 @@ _dl_map_object (struct link_map *loader, const char *name,
>  #endif
>  
>    /* Look for this name among those already loaded.  */
> -  for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
> +  l = _dl_find_dso (name, nsid);
> +
> +  if (l != NULL)
>      {
> -      /* If the requested name matches the soname of a loaded object,
> -	 use that object.  Elide this check for names that have not
> -	 yet been opened.  */
> -      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
> -	continue;
> -      if (!_dl_name_match_p (name, l))
> -	{
> -	  const char *soname;
> -
> -	  if (__glibc_likely (l->l_soname_added)
> -	      || l->l_info[DT_SONAME] == NULL)
> -	    continue;
> -
> -	  soname = ((const char *) D_PTR (l, l_info[DT_STRTAB])
> -		    + l->l_info[DT_SONAME]->d_un.d_val);
> -	  if (strcmp (name, soname) != 0)
> -	    continue;
> -
> -	  /* We have a match on a new name -- cache it.  */
> -	  add_name_to_object (l, soname);
> -	  l->l_soname_added = 1;
> -	}
> -
> -      /* We have a match.  */
> +#ifdef SHARED
> +      /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS
> +         already opened in the target NS but with RTLD_ISOLATE so it WAS NOT
> +         created as a proxy we need to error out since we cannot satisfy the
> +         DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics.  */
> +      if (!(mode & RTLD_ISOLATE) &&
> +          (l->l_ns != LM_ID_BASE) &&
> +          (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) &&
> +          !l->l_proxy)
> +      {
> +        _dl_signal_error (EEXIST, name, NULL,
> +                          N_("object cannot be demoted to a proxy"));
> +      }
> +#endif
>        return l;

I think this patch should reorganized along with the previous, where
first you refactor the code to use find_dso and *then* add the
required l_proxy change in a subsequent patch.

(There are other patch organization that I think should be changed,
I will get back to them once I finish reviewing the set)

>      }
> -

I usually avoid such spurious change.

>    /* Display information if we are debugging.  */
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
>        && loader != NULL)
>
Adhemerval Zanella Feb. 19, 2021, 7:50 p.m. UTC | #2
On 19/02/2021 11:39, Adhemerval Zanella wrote:
> 
> 
> On 09/02/2021 14:18, Vivek Das Mohapatra via Libc-alpha wrote:
>> ---
>>  elf/dl-load.c | 43 +++++++++++++++++--------------------------
>>  1 file changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 780bca99e8..13ac2053b8 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -2133,35 +2133,26 @@ _dl_map_object (struct link_map *loader, const char *name,
>>  #endif
>>  
>>    /* Look for this name among those already loaded.  */
>> -  for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
>> +  l = _dl_find_dso (name, nsid);
>> +
>> +  if (l != NULL)
>>      {
>> -      /* If the requested name matches the soname of a loaded object,
>> -	 use that object.  Elide this check for names that have not
>> -	 yet been opened.  */
>> -      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
>> -	continue;
>> -      if (!_dl_name_match_p (name, l))
>> -	{
>> -	  const char *soname;
>> -
>> -	  if (__glibc_likely (l->l_soname_added)
>> -	      || l->l_info[DT_SONAME] == NULL)
>> -	    continue;
>> -
>> -	  soname = ((const char *) D_PTR (l, l_info[DT_STRTAB])
>> -		    + l->l_info[DT_SONAME]->d_un.d_val);
>> -	  if (strcmp (name, soname) != 0)
>> -	    continue;
>> -
>> -	  /* We have a match on a new name -- cache it.  */
>> -	  add_name_to_object (l, soname);
>> -	  l->l_soname_added = 1;
>> -	}
>> -
>> -      /* We have a match.  */
>> +#ifdef SHARED
>> +      /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS
>> +         already opened in the target NS but with RTLD_ISOLATE so it WAS NOT
>> +         created as a proxy we need to error out since we cannot satisfy the
>> +         DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics.  */
>> +      if (!(mode & RTLD_ISOLATE) &&

Also the new RTLD_ISOLATE flag is only defined later in this set.

>> +          (l->l_ns != LM_ID_BASE) &&
>> +          (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) &&
>> +          !l->l_proxy)
>> +      {
>> +        _dl_signal_error (EEXIST, name, NULL,
>> +                          N_("object cannot be demoted to a proxy"));
>> +      }
>> +#endif
>>        return l;
> 
> I think this patch should reorganized along with the previous, where
> first you refactor the code to use find_dso and *then* add the
> required l_proxy change in a subsequent patch.
> 
> (There are other patch organization that I think should be changed,
> I will get back to them once I finish reviewing the set)
> 
>>      }
>> -
> 
> I usually avoid such spurious change.
> 
>>    /* Display information if we are debugging.  */
>>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
>>        && loader != NULL)
>>
Vivek Das Mohapatra Feb. 22, 2021, 6:44 p.m. UTC | #3
> I think this patch should reorganized along with the previous, where
> first you refactor the code to use find_dso and *then* add the
> required l_proxy change in a subsequent patch.

Do you want this combined with the preceding patch or reorganised
in some other way?
Adhemerval Zanella Feb. 22, 2021, 6:51 p.m. UTC | #4
On 22/02/2021 15:44, Vivek Das Mohapatra wrote:
>> I think this patch should reorganized along with the previous, where
>> first you refactor the code to use find_dso and *then* add the
>> required l_proxy change in a subsequent patch.
> 
> Do you want this combined with the preceding patch or reorganised
> in some other way?
> 

My plan is to finish the review and send a overall note of how I
think the set should be better organized.
Vivek Das Mohapatra Feb. 22, 2021, 11:50 p.m. UTC | #5
>> Do you want this combined with the preceding patch or reorganised
>> in some other way?
>
> My plan is to finish the review and send a overall note of how I think the set 
> should be better organized.

Ok, thanks.
diff mbox series

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 780bca99e8..13ac2053b8 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -2133,35 +2133,26 @@  _dl_map_object (struct link_map *loader, const char *name,
 #endif
 
   /* Look for this name among those already loaded.  */
-  for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
+  l = _dl_find_dso (name, nsid);
+
+  if (l != NULL)
     {
-      /* If the requested name matches the soname of a loaded object,
-	 use that object.  Elide this check for names that have not
-	 yet been opened.  */
-      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
-	continue;
-      if (!_dl_name_match_p (name, l))
-	{
-	  const char *soname;
-
-	  if (__glibc_likely (l->l_soname_added)
-	      || l->l_info[DT_SONAME] == NULL)
-	    continue;
-
-	  soname = ((const char *) D_PTR (l, l_info[DT_STRTAB])
-		    + l->l_info[DT_SONAME]->d_un.d_val);
-	  if (strcmp (name, soname) != 0)
-	    continue;
-
-	  /* We have a match on a new name -- cache it.  */
-	  add_name_to_object (l, soname);
-	  l->l_soname_added = 1;
-	}
-
-      /* We have a match.  */
+#ifdef SHARED
+      /* If we are trying to load a DF_GNU_1_UNIQUE flagged DSO which WAS
+         already opened in the target NS but with RTLD_ISOLATE so it WAS NOT
+         created as a proxy we need to error out since we cannot satisfy the
+         DF_GNU_1_UNIQUE is-equivalent-to RTLD_SHARED semantics.  */
+      if (!(mode & RTLD_ISOLATE) &&
+          (l->l_ns != LM_ID_BASE) &&
+          (l->l_gnu_flags_1 & DF_GNU_1_UNIQUE) &&
+          !l->l_proxy)
+      {
+        _dl_signal_error (EEXIST, name, NULL,
+                          N_("object cannot be demoted to a proxy"));
+      }
+#endif
       return l;
     }
-
   /* Display information if we are debugging.  */
   if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
       && loader != NULL)