elf: Fix LD_DEBUG misattributing R(UN)PATH list (bug 31739)

Message ID LO6P265MB63473097FD2F2A726C7565398BE32@LO6P265MB6347.GBRP265.PROD.OUTLOOK.COM
State Accepted
Headers
Series elf: Fix LD_DEBUG misattributing R(UN)PATH list (bug 31739) |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Tobiasz Laskowski May 14, 2024, 12:13 a.m. UTC
  If the first RUNPATH entry for the current binary has encountered
previously, then `this_dir->where` gives the name of the first binary
which had this RUNPATH entry. This means that the entire RUNPATH list
for the current binary is misattributed to another file by the
`LD_DEBUG=libs` output.

This patch solves this by instead using the `l_name` field of the loader
object from where the RUNPATH list was read.

NB: This all applies to RPATHs as well as RUNPATHs

Signed-off-by: Tobiasz Laskowski <tobil4sk@outlook.com>
---
 elf/dl-load.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)
  

Comments

Adhemerval Zanella Netto May 16, 2024, 7:23 a.m. UTC | #1
On 14/05/24 02:13, Tobiasz Laskowski wrote:
> If the first RUNPATH entry for the current binary has encountered
> previously, then `this_dir->where` gives the name of the first binary
> which had this RUNPATH entry. This means that the entire RUNPATH list
> for the current binary is misattributed to another file by the
> `LD_DEBUG=libs` output.
> 
> This patch solves this by instead using the `l_name` field of the loader
> object from where the RUNPATH list was read.
> 
> NB: This all applies to RPATHs as well as RUNPATHs
> 
> Signed-off-by: Tobiasz Laskowski <tobil4sk@outlook.com>

LGTM, thanks.  How hard would to add a regression test for it?

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-load.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a34cb3559c..2362a4aa99 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1786,9 +1786,9 @@ open_verify (const char *name, int fd,
>  
>  static int
>  open_path (const char *name, size_t namelen, int mode,
> -	   struct r_search_path_struct *sps, char **realname,
> -	   struct filebuf *fbp, struct link_map *loader, int whatcode,
> -	   bool *found_other_class)
> +	   struct r_search_path_struct *sps, const char* search_path_source,
> +	   char **realname, struct filebuf *fbp, struct link_map *loader,
> +	   int whatcode, bool *found_other_class)
>  {
>    struct r_search_path_elem **dirs = sps->dirs;
>    char *buf;
> @@ -1816,7 +1816,7 @@ open_path (const char *name, size_t namelen, int mode,
>  	  && current_what != this_dir->what)
>  	{
>  	  current_what = this_dir->what;
> -	  print_search_path (dirs, current_what, this_dir->where);
> +	  print_search_path (dirs, current_what, search_path_source);
>  	}
>  
>        edp = (char *) __mempcpy (buf, this_dir->dirname, this_dir->dirnamelen);
> @@ -2038,10 +2038,9 @@ _dl_map_object (struct link_map *loader, const char *name,
>  	  for (l = loader; l; l = l->l_loader)
>  	    if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH"))
>  	      {
> -		fd = open_path (name, namelen, mode,
> -				&l->l_rpath_dirs,
> -				&realname, &fb, loader, LA_SER_RUNPATH,
> -				&found_other_class);
> +		fd = open_path (name, namelen, mode, &l->l_rpath_dirs,
> +				l->l_name, &realname, &fb, loader,
> +				LA_SER_RUNPATH, &found_other_class);
>  		if (fd != -1)
>  		  break;
>  
> @@ -2054,9 +2053,9 @@ _dl_map_object (struct link_map *loader, const char *name,
>  	      && main_map != NULL && main_map->l_type != lt_loaded
>  	      && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH,
>  			      "RPATH"))
> -	    fd = open_path (name, namelen, mode,
> -			    &main_map->l_rpath_dirs,
> -			    &realname, &fb, loader ?: main_map, LA_SER_RUNPATH,
> +	    fd = open_path (name, namelen, mode, &main_map->l_rpath_dirs,
> +			    main_map->l_name, &realname, &fb,
> +			    loader ?: main_map, LA_SER_RUNPATH,
>  			    &found_other_class);
>  
>  	  /* Also try DT_RUNPATH in the executable for LD_AUDIT dlopen
> @@ -2070,14 +2069,15 @@ _dl_map_object (struct link_map *loader, const char *name,
>  	      if (cache_rpath (main_map, &l_rpath_dirs,
>  			       DT_RUNPATH, "RUNPATH"))
>  		fd = open_path (name, namelen, mode, &l_rpath_dirs,
> -				&realname, &fb, loader ?: main_map,
> -				LA_SER_RUNPATH, &found_other_class);
> +				main_map->l_name, &realname, &fb,
> +				loader ?: main_map, LA_SER_RUNPATH,
> +				&found_other_class);
>  	    }
>  	}
>  
>        /* Try the LD_LIBRARY_PATH environment variable.  */
>        if (fd == -1 && __rtld_env_path_list.dirs != (void *) -1)
> -	fd = open_path (name, namelen, mode, &__rtld_env_path_list,
> +	fd = open_path (name, namelen, mode, &__rtld_env_path_list, NULL,
>  			&realname, &fb,
>  			loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded,
>  			LA_SER_LIBPATH, &found_other_class);
> @@ -2086,8 +2086,8 @@ _dl_map_object (struct link_map *loader, const char *name,
>        if (fd == -1 && loader != NULL
>  	  && cache_rpath (loader, &loader->l_runpath_dirs,
>  			  DT_RUNPATH, "RUNPATH"))
> -	fd = open_path (name, namelen, mode,
> -			&loader->l_runpath_dirs, &realname, &fb, loader,
> +	fd = open_path (name, namelen, mode, &loader->l_runpath_dirs,
> +			loader->l_name, &realname, &fb, loader,
>  			LA_SER_RUNPATH, &found_other_class);
>  
>  #ifdef USE_LDCONFIG
> @@ -2153,7 +2153,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>  	  && ((l = loader ?: GL(dl_ns)[nsid]._ns_loaded) == NULL
>  	      || __glibc_likely (!(l->l_flags_1 & DF_1_NODEFLIB)))
>  	  && __rtld_search_dirs.dirs != (void *) -1)
> -	fd = open_path (name, namelen, mode, &__rtld_search_dirs,
> +	fd = open_path (name, namelen, mode, &__rtld_search_dirs, NULL,
>  			&realname, &fb, l, LA_SER_DEFAULT, &found_other_class);
>  
>        /* Add another newline when we are tracing the library loading.  */
  
Carlos O'Donell May 16, 2024, 11:24 a.m. UTC | #2
On 5/16/24 3:23 AM, Adhemerval Zanella Netto wrote:
> 
> 
> On 14/05/24 02:13, Tobiasz Laskowski wrote:
>> If the first RUNPATH entry for the current binary has encountered
>> previously, then `this_dir->where` gives the name of the first binary
>> which had this RUNPATH entry. This means that the entire RUNPATH list
>> for the current binary is misattributed to another file by the
>> `LD_DEBUG=libs` output.
>>
>> This patch solves this by instead using the `l_name` field of the loader
>> object from where the RUNPATH list was read.
>>
>> NB: This all applies to RPATHs as well as RUNPATHs
>>
>> Signed-off-by: Tobiasz Laskowski <tobil4sk@outlook.com>
> 
> LGTM, thanks.  How hard would to add a regression test for it?

My guidance for a regression test would be:

Build two binaries that exhibit this behaviour via RPATH and RUNPATH.

Run a top-level test via support_capture_subprocess() and run the sub-process,
and then look for the result to contain what you need.

Examples: elf/tst-pldd.c (container test), elf/tst-audit18.c (normal test) etc.

I would avoid containerizing the test so it can run in as many configurations
as possible.
  

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index a34cb3559c..2362a4aa99 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1786,9 +1786,9 @@  open_verify (const char *name, int fd,
 
 static int
 open_path (const char *name, size_t namelen, int mode,
-	   struct r_search_path_struct *sps, char **realname,
-	   struct filebuf *fbp, struct link_map *loader, int whatcode,
-	   bool *found_other_class)
+	   struct r_search_path_struct *sps, const char* search_path_source,
+	   char **realname, struct filebuf *fbp, struct link_map *loader,
+	   int whatcode, bool *found_other_class)
 {
   struct r_search_path_elem **dirs = sps->dirs;
   char *buf;
@@ -1816,7 +1816,7 @@  open_path (const char *name, size_t namelen, int mode,
 	  && current_what != this_dir->what)
 	{
 	  current_what = this_dir->what;
-	  print_search_path (dirs, current_what, this_dir->where);
+	  print_search_path (dirs, current_what, search_path_source);
 	}
 
       edp = (char *) __mempcpy (buf, this_dir->dirname, this_dir->dirnamelen);
@@ -2038,10 +2038,9 @@  _dl_map_object (struct link_map *loader, const char *name,
 	  for (l = loader; l; l = l->l_loader)
 	    if (cache_rpath (l, &l->l_rpath_dirs, DT_RPATH, "RPATH"))
 	      {
-		fd = open_path (name, namelen, mode,
-				&l->l_rpath_dirs,
-				&realname, &fb, loader, LA_SER_RUNPATH,
-				&found_other_class);
+		fd = open_path (name, namelen, mode, &l->l_rpath_dirs,
+				l->l_name, &realname, &fb, loader,
+				LA_SER_RUNPATH, &found_other_class);
 		if (fd != -1)
 		  break;
 
@@ -2054,9 +2053,9 @@  _dl_map_object (struct link_map *loader, const char *name,
 	      && main_map != NULL && main_map->l_type != lt_loaded
 	      && cache_rpath (main_map, &main_map->l_rpath_dirs, DT_RPATH,
 			      "RPATH"))
-	    fd = open_path (name, namelen, mode,
-			    &main_map->l_rpath_dirs,
-			    &realname, &fb, loader ?: main_map, LA_SER_RUNPATH,
+	    fd = open_path (name, namelen, mode, &main_map->l_rpath_dirs,
+			    main_map->l_name, &realname, &fb,
+			    loader ?: main_map, LA_SER_RUNPATH,
 			    &found_other_class);
 
 	  /* Also try DT_RUNPATH in the executable for LD_AUDIT dlopen
@@ -2070,14 +2069,15 @@  _dl_map_object (struct link_map *loader, const char *name,
 	      if (cache_rpath (main_map, &l_rpath_dirs,
 			       DT_RUNPATH, "RUNPATH"))
 		fd = open_path (name, namelen, mode, &l_rpath_dirs,
-				&realname, &fb, loader ?: main_map,
-				LA_SER_RUNPATH, &found_other_class);
+				main_map->l_name, &realname, &fb,
+				loader ?: main_map, LA_SER_RUNPATH,
+				&found_other_class);
 	    }
 	}
 
       /* Try the LD_LIBRARY_PATH environment variable.  */
       if (fd == -1 && __rtld_env_path_list.dirs != (void *) -1)
-	fd = open_path (name, namelen, mode, &__rtld_env_path_list,
+	fd = open_path (name, namelen, mode, &__rtld_env_path_list, NULL,
 			&realname, &fb,
 			loader ?: GL(dl_ns)[LM_ID_BASE]._ns_loaded,
 			LA_SER_LIBPATH, &found_other_class);
@@ -2086,8 +2086,8 @@  _dl_map_object (struct link_map *loader, const char *name,
       if (fd == -1 && loader != NULL
 	  && cache_rpath (loader, &loader->l_runpath_dirs,
 			  DT_RUNPATH, "RUNPATH"))
-	fd = open_path (name, namelen, mode,
-			&loader->l_runpath_dirs, &realname, &fb, loader,
+	fd = open_path (name, namelen, mode, &loader->l_runpath_dirs,
+			loader->l_name, &realname, &fb, loader,
 			LA_SER_RUNPATH, &found_other_class);
 
 #ifdef USE_LDCONFIG
@@ -2153,7 +2153,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 	  && ((l = loader ?: GL(dl_ns)[nsid]._ns_loaded) == NULL
 	      || __glibc_likely (!(l->l_flags_1 & DF_1_NODEFLIB)))
 	  && __rtld_search_dirs.dirs != (void *) -1)
-	fd = open_path (name, namelen, mode, &__rtld_search_dirs,
+	fd = open_path (name, namelen, mode, &__rtld_search_dirs, NULL,
 			&realname, &fb, l, LA_SER_DEFAULT, &found_other_class);
 
       /* Add another newline when we are tracing the library loading.  */