[RFC PATCH 0/2] On ldconfig and ld.so.cache

[RFC PATCH 0/2] On ldconfig and ld.so.cache

[Sergey Bugaev]

Hello,

having set up a very basic x86_64-gnu system to debug startup issues, I
was surprised to discover that my self-built ld.so does not look for the
shared libraries in /lib/x86_64-gnu/ (which is where Samuel Thibault's deb
packages place them) at all. I then learned that ld.so.cache and ldconfig
and ld.so.conf support is explicitly getting compiled out in the *-gnu
configurations -- and that this is being done intentionally, since
ldconfig is apparently considered to be a hack and a misfeature [0].

[0]: https://lists.debian.org/debian-hurd/2001/04/msg00179.html

(Might this be just due to misunderstanding of what ldconfig does? It's
not only about creating symlinks, it's primarily about the ld.so.cache and
ld.so.conf...)

<rant>

This doesn't really make sense to me: surely whether ld.so.cache is a
good idea or not is not tied to a particular kernel? Surely the kernel
could not care less about things like the file dystem layout and how
ld.so looks for shared libraries? Predicating ld.so.cache usage on whether
the GNU system is running on the Linux or Hurd kernel sounds like creating
pointless differences to me.

Moreover, Debian GNU/Hurd, the primary Hurd-based distribution, has been
shipping ld.so.cache on Hurd as a downstream patch [1] (note that more
changes would be required for x86_64-gnu because of FLAG_X8664_LIB64).
They don't really have a choice, it seems: with their "multiarch"
filesystem layout, the shared libraries are to be located in
/lib/i386-gnu/, but that is not a path that ld.so searches for libraries
in! This is solved by use_ldconfig=yes, and putting /lib/i386-gnu/ into
/etc/ld.so.conf.d/i386-gnu.conf, where it is then picked up by ldconfig,
which finds all the libraries and bakes their paths into the cache, where
ld.so then picks them up.

[1]: https://salsa.debian.org/glibc-team/glibc/-/raw/sid/debian/patches/hurd-i386/local-enable-ldconfig.diff

This is also another thing that suprises me: how come ldconfig does read
ld.so.conf, but ld.so does not? It's not an "ldconfig.conf" after all...
How is one even supposed to configure library paths with use_ldconfig=no?

</rant>

Anyway, maybe there are valid technical reasons to not enable ldconfig by
default; this is not a hill that I'm willing to die on. But wouldn't it be
nicer if it was easier to enable ldconfig downstream if desired? To that
end, I tweaked ldconfig.c to not use PATH_MAX (sadly, there are still
PATH_MAX usages in other places, e.g. chroot_canon.c), and moved a couple
of files to stop being specific to the Linux port. I hope that both of
these changes are uncontroversial. With these changes it is possible to
just patch in use_ldconfig=yes downstream (is desired, of course) and get
an ld.so.cache-enabled glibc on both i386-gnu and x86_64-gnu.

Sergey

[RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Sergey Bugaev]

ldconfig was allocating PATH_MAX bytes on the stack for the library file
name. The issues with PATH_MAX usage are well documented [0][1]; even if
a program does not rely on paths being limited to PATH_MAX bytes,
allocating 4096 bytes on the stack for paths that are typically rather
short (strlen ("/lib64/libc.so.6") is 16) is wasteful and dangerous.

[0]: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html
[1]: https://eklitzke.org/path-max-is-tricky

Instead, make use of asprintf to dynamically allocate memory of just the
right size on the heap.

Checked on x86_64-linux-gnu and i686-gnu.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 elf/ldconfig.c | 59 +++++++++++++++++---------------------------------
 1 file changed, 20 insertions(+), 39 deletions(-)

diff --git a/elf/ldconfig.c b/elf/ldconfig.c
index 2fc45ad8..7f2e4226 100644
--- a/elf/ldconfig.c
+++ b/elf/ldconfig.c
@@ -677,28 +677,18 @@ search_dir (const struct dir_entry *entry)
 
   char *dir_name;
   char *real_file_name;
-  size_t real_file_name_len;
-  size_t file_name_len = PATH_MAX;
-  char *file_name = alloca (file_name_len);
+  char *file_name;
   if (opt_chroot != NULL)
-    {
-      dir_name = chroot_canon (opt_chroot, entry->path);
-      real_file_name_len = PATH_MAX;
-      real_file_name = alloca (real_file_name_len);
-    }
+    dir_name = chroot_canon (opt_chroot, entry->path);
   else
-    {
-      dir_name = entry->path;
-      real_file_name_len = 0;
-      real_file_name = file_name;
-    }
+    dir_name = entry->path;
 
   DIR *dir;
   if (dir_name == NULL || (dir = opendir (dir_name)) == NULL)
     {
       if (opt_verbose)
 	error (0, errno, _("Can't open directory %s"), entry->path);
-      if (opt_chroot != NULL && dir_name != NULL)
+      if (opt_chroot != NULL)
 	free (dir_name);
       return;
     }
@@ -733,25 +723,11 @@ search_dir (const struct dir_entry *entry)
 			 + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0)
 	    continue;
 	}
-      len += strlen (entry->path) + 2;
-      if (len > file_name_len)
-	{
-	  file_name_len = len;
-	  file_name = alloca (file_name_len);
-	  if (!opt_chroot)
-	    real_file_name = file_name;
-	}
-      sprintf (file_name, "%s/%s", entry->path, direntry->d_name);
+      asprintf (&file_name, "%s/%s", entry->path, direntry->d_name);
       if (opt_chroot != NULL)
-	{
-	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
-	  if (len > real_file_name_len)
-	    {
-	      real_file_name_len = len;
-	      real_file_name = alloca (real_file_name_len);
-	    }
-	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
-	}
+	asprintf (&real_file_name, "%s/%s", dir_name, direntry->d_name);
+      else
+	real_file_name = file_name;
 
       struct stat lstat_buf;
       /* We optimize and try to do the lstat call only if needed.  */
@@ -761,7 +737,7 @@ search_dir (const struct dir_entry *entry)
 	if (__glibc_unlikely (lstat (real_file_name, &lstat_buf)))
 	  {
 	    error (0, errno, _("Cannot lstat %s"), file_name);
-	    continue;
+	    goto next;
 	  }
 
       struct stat stat_buf;
@@ -778,7 +754,7 @@ search_dir (const struct dir_entry *entry)
 		{
 		  if (strstr (file_name, ".so") == NULL)
 		    error (0, 0, _("Input file %s not found.\n"), file_name);
-		  continue;
+		  goto next;
 		}
 	    }
 	  if (__glibc_unlikely (stat (target_name, &stat_buf)))
@@ -793,7 +769,7 @@ search_dir (const struct dir_entry *entry)
 	      if (opt_chroot != NULL)
 		free (target_name);
 
-	      continue;
+	      goto next;
 	    }
 
 	  if (opt_chroot != NULL)
@@ -806,7 +782,7 @@ search_dir (const struct dir_entry *entry)
 	  lstat_buf.st_ctime = stat_buf.st_ctime;
 	}
       else if (!S_ISREG (lstat_buf.st_mode))
-	continue;
+	goto next;
 
       char *real_name;
       if (opt_chroot != NULL && is_link)
@@ -816,7 +792,7 @@ search_dir (const struct dir_entry *entry)
 	    {
 	      if (strstr (file_name, ".so") == NULL)
 		error (0, 0, _("Input file %s not found.\n"), file_name);
-	      continue;
+	      goto next;
 	    }
 	}
       else
@@ -828,7 +804,7 @@ search_dir (const struct dir_entry *entry)
 	  && __builtin_expect (lstat (real_file_name, &lstat_buf), 0))
 	{
 	  error (0, errno, _("Cannot lstat %s"), file_name);
-	  continue;
+	  goto next;
 	}
 
       /* First search whether the auxiliary cache contains this
@@ -842,7 +818,7 @@ search_dir (const struct dir_entry *entry)
 	    {
 	      if (real_name != real_file_name)
 		free (real_name);
-	      continue;
+	      goto next;
 	    }
 	  else if (opt_build_cache)
 	    add_to_aux_cache (&lstat_buf, flag, isa_level, soname);
@@ -948,6 +924,11 @@ search_dir (const struct dir_entry *entry)
 	  dlib_ptr->next = dlibs;
 	  dlibs = dlib_ptr;
 	}
+
+    next:
+      free (file_name);
+      if (opt_chroot != NULL)
+	free (real_file_name);
     }
 
   closedir (dir);
-- 
2.40.1

[RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific

[Sergey Bugaev]

These files could be useful to any port that wants to use ld.so.cache.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 sysdeps/{unix/sysv/linux => }/x86/readelflib.c  | 0
 sysdeps/{unix/sysv/linux => }/x86_64/dl-cache.h | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename sysdeps/{unix/sysv/linux => }/x86/readelflib.c (100%)
 rename sysdeps/{unix/sysv/linux => }/x86_64/dl-cache.h (100%)

diff --git a/sysdeps/unix/sysv/linux/x86/readelflib.c b/sysdeps/x86/readelflib.c
similarity index 100%
rename from sysdeps/unix/sysv/linux/x86/readelflib.c
rename to sysdeps/x86/readelflib.c
diff --git a/sysdeps/unix/sysv/linux/x86_64/dl-cache.h b/sysdeps/x86_64/dl-cache.h
similarity index 100%
rename from sysdeps/unix/sysv/linux/x86_64/dl-cache.h
rename to sysdeps/x86_64/dl-cache.h
-- 
2.40.1

Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Adhemerval Zanella Netto]

On 17/05/23 15:54, Sergey Bugaev via Libc-alpha wrote:
> ldconfig was allocating PATH_MAX bytes on the stack for the library file
> name. The issues with PATH_MAX usage are well documented [0][1]; even if
> a program does not rely on paths being limited to PATH_MAX bytes,
> allocating 4096 bytes on the stack for paths that are typically rather
> short (strlen ("/lib64/libc.so.6") is 16) is wasteful and dangerous.
> 
> [0]: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html
> [1]: https://eklitzke.org/path-max-is-tricky
> 
> Instead, make use of asprintf to dynamically allocate memory of just the
> right size on the heap.
> 
> Checked on x86_64-linux-gnu and i686-gnu.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

Sounds reasonable and one less alloca usage.

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

> ---
>  elf/ldconfig.c | 59 +++++++++++++++++---------------------------------
>  1 file changed, 20 insertions(+), 39 deletions(-)
> 
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 2fc45ad8..7f2e4226 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -677,28 +677,18 @@ search_dir (const struct dir_entry *entry)
>  
>    char *dir_name;
>    char *real_file_name;
> -  size_t real_file_name_len;
> -  size_t file_name_len = PATH_MAX;
> -  char *file_name = alloca (file_name_len);
> +  char *file_name;
>    if (opt_chroot != NULL)
> -    {
> -      dir_name = chroot_canon (opt_chroot, entry->path);
> -      real_file_name_len = PATH_MAX;
> -      real_file_name = alloca (real_file_name_len);
> -    }
> +    dir_name = chroot_canon (opt_chroot, entry->path);
>    else
> -    {
> -      dir_name = entry->path;
> -      real_file_name_len = 0;
> -      real_file_name = file_name;
> -    }
> +    dir_name = entry->path;
>  
>    DIR *dir;
>    if (dir_name == NULL || (dir = opendir (dir_name)) == NULL)
>      {
>        if (opt_verbose)
>  	error (0, errno, _("Can't open directory %s"), entry->path);
> -      if (opt_chroot != NULL && dir_name != NULL)
> +      if (opt_chroot != NULL)
>  	free (dir_name);
>        return;
>      }
> @@ -733,25 +723,11 @@ search_dir (const struct dir_entry *entry)
>  			 + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0)
>  	    continue;
>  	}
> -      len += strlen (entry->path) + 2;
> -      if (len > file_name_len)
> -	{
> -	  file_name_len = len;
> -	  file_name = alloca (file_name_len);
> -	  if (!opt_chroot)
> -	    real_file_name = file_name;
> -	}
> -      sprintf (file_name, "%s/%s", entry->path, direntry->d_name);
> +      asprintf (&file_name, "%s/%s", entry->path, direntry->d_name);
>        if (opt_chroot != NULL)
> -	{
> -	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
> -	  if (len > real_file_name_len)
> -	    {
> -	      real_file_name_len = len;
> -	      real_file_name = alloca (real_file_name_len);
> -	    }
> -	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
> -	}
> +	asprintf (&real_file_name, "%s/%s", dir_name, direntry->d_name);
> +      else
> +	real_file_name = file_name;
>  
>        struct stat lstat_buf;
>        /* We optimize and try to do the lstat call only if needed.  */
> @@ -761,7 +737,7 @@ search_dir (const struct dir_entry *entry)
>  	if (__glibc_unlikely (lstat (real_file_name, &lstat_buf)))
>  	  {
>  	    error (0, errno, _("Cannot lstat %s"), file_name);
> -	    continue;
> +	    goto next;
>  	  }
>  
>        struct stat stat_buf;
> @@ -778,7 +754,7 @@ search_dir (const struct dir_entry *entry)
>  		{
>  		  if (strstr (file_name, ".so") == NULL)
>  		    error (0, 0, _("Input file %s not found.\n"), file_name);
> -		  continue;
> +		  goto next;
>  		}
>  	    }
>  	  if (__glibc_unlikely (stat (target_name, &stat_buf)))
> @@ -793,7 +769,7 @@ search_dir (const struct dir_entry *entry)
>  	      if (opt_chroot != NULL)
>  		free (target_name);
>  
> -	      continue;
> +	      goto next;
>  	    }
>  
>  	  if (opt_chroot != NULL)
> @@ -806,7 +782,7 @@ search_dir (const struct dir_entry *entry)
>  	  lstat_buf.st_ctime = stat_buf.st_ctime;
>  	}
>        else if (!S_ISREG (lstat_buf.st_mode))
> -	continue;
> +	goto next;
>  
>        char *real_name;
>        if (opt_chroot != NULL && is_link)
> @@ -816,7 +792,7 @@ search_dir (const struct dir_entry *entry)
>  	    {
>  	      if (strstr (file_name, ".so") == NULL)
>  		error (0, 0, _("Input file %s not found.\n"), file_name);
> -	      continue;
> +	      goto next;
>  	    }
>  	}
>        else
> @@ -828,7 +804,7 @@ search_dir (const struct dir_entry *entry)
>  	  && __builtin_expect (lstat (real_file_name, &lstat_buf), 0))
>  	{
>  	  error (0, errno, _("Cannot lstat %s"), file_name);
> -	  continue;
> +	  goto next;
>  	}
>  
>        /* First search whether the auxiliary cache contains this
> @@ -842,7 +818,7 @@ search_dir (const struct dir_entry *entry)
>  	    {
>  	      if (real_name != real_file_name)
>  		free (real_name);
> -	      continue;
> +	      goto next;
>  	    }
>  	  else if (opt_build_cache)
>  	    add_to_aux_cache (&lstat_buf, flag, isa_level, soname);
> @@ -948,6 +924,11 @@ search_dir (const struct dir_entry *entry)
>  	  dlib_ptr->next = dlibs;
>  	  dlibs = dlib_ptr;
>  	}
> +
> +    next:
> +      free (file_name);
> +      if (opt_chroot != NULL)
> +	free (real_file_name);
>      }
>  
>    closedir (dir);

Re: [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific

[Carlos O'Donell]

On 5/17/23 14:54, Sergey Bugaev via Libc-alpha wrote:
> These files could be useful to any port that wants to use ld.so.cache.

I agree, and this is likely cargo-cult across the targets.

At most we look at EI_CLASS, and EM_* machine, and FLAG_ELF_LIBC6 is
defined in sysdeps/generic/ldconfig.h (not Linux specific) and likewise
for the other flags.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  sysdeps/{unix/sysv/linux => }/x86/readelflib.c  | 0
>  sysdeps/{unix/sysv/linux => }/x86_64/dl-cache.h | 0
>  2 files changed, 0 insertions(+), 0 deletions(-)
>  rename sysdeps/{unix/sysv/linux => }/x86/readelflib.c (100%)
>  rename sysdeps/{unix/sysv/linux => }/x86_64/dl-cache.h (100%)
> 
> diff --git a/sysdeps/unix/sysv/linux/x86/readelflib.c b/sysdeps/x86/readelflib.c
> similarity index 100%
> rename from sysdeps/unix/sysv/linux/x86/readelflib.c
> rename to sysdeps/x86/readelflib.c
> diff --git a/sysdeps/unix/sysv/linux/x86_64/dl-cache.h b/sysdeps/x86_64/dl-cache.h
> similarity index 100%
> rename from sysdeps/unix/sysv/linux/x86_64/dl-cache.h
> rename to sysdeps/x86_64/dl-cache.h



-- 
Cheers,
Carlos.

Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache

[Carlos O'Donell]

On 5/17/23 14:54, Sergey Bugaev via Libc-alpha wrote:
> Hello,
> 
> having set up a very basic x86_64-gnu system to debug startup issues, I
> was surprised to discover that my self-built ld.so does not look for the
> shared libraries in /lib/x86_64-gnu/ (which is where Samuel Thibault's deb
> packages place them) at all. I then learned that ld.so.cache and ldconfig
> and ld.so.conf support is explicitly getting compiled out in the *-gnu
> configurations -- and that this is being done intentionally, since
> ldconfig is apparently considered to be a hack and a misfeature [0].
> 
> [0]: https://lists.debian.org/debian-hurd/2001/04/msg00179.html
> 
> (Might this be just due to misunderstanding of what ldconfig does? It's
> not only about creating symlinks, it's primarily about the ld.so.cache and
> ld.so.conf...)
> 
> <rant>
> 
> This doesn't really make sense to me: surely whether ld.so.cache is a
> good idea or not is not tied to a particular kernel? Surely the kernel
> could not care less about things like the file dystem layout and how
> ld.so looks for shared libraries? Predicating ld.so.cache usage on whether
> the GNU system is running on the Linux or Hurd kernel sounds like creating
> pointless differences to me.

I agree that we should reduce differences here, and that the data files used by the
loader should be more-or-less the same across kernels.

Keep in mind that ld.so.cache is not actually a cache, but a data file that contains
the pre-processed contents of /etc/ld.so.conf* in a format that can be easily loaded
by the dynamic loader without having a parser (glob included) in the early startup
code. Deleting the cache can break a system, and perhaps this is what some developers
have objected to and called "a hack."

I filed a bug for this in 2017 to capture the issue:
ld.so's cache should live in /var/cache, and support cache deletion.
https://sourceware.org/bugzilla/show_bug.cgi?id=22359

> Moreover, Debian GNU/Hurd, the primary Hurd-based distribution, has been
> shipping ld.so.cache on Hurd as a downstream patch [1] (note that more
> changes would be required for x86_64-gnu because of FLAG_X8664_LIB64).
> They don't really have a choice, it seems: with their "multiarch"
> filesystem layout, the shared libraries are to be located in
> /lib/i386-gnu/, but that is not a path that ld.so searches for libraries
> in! This is solved by use_ldconfig=yes, and putting /lib/i386-gnu/ into
> /etc/ld.so.conf.d/i386-gnu.conf, where it is then picked up by ldconfig,
> which finds all the libraries and bakes their paths into the cache, where
> ld.so then picks them up.

From 2018 we have this: 
Implement multiarch ldconfig
https://sourceware.org/bugzilla/show_bug.cgi?id=22825


> [1]: https://salsa.debian.org/glibc-team/glibc/-/raw/sid/debian/patches/hurd-i386/local-enable-ldconfig.diff
> 
> This is also another thing that suprises me: how come ldconfig does read
> ld.so.conf, but ld.so does not? It's not an "ldconfig.conf" after all...
> How is one even supposed to configure library paths with use_ldconfig=no?

The file /etc/ld.so.cache is not a cache, it is a data file used by the dynamic loader.

The data file is generated by ldconfig as it parses /etc/ld.so.conf*.

The only other way to configure library paths with `use_ldconfig=` is to use LD_PRELOAD,
LD_LIBRARY_PATH, DT_RPATH, or DT_RUNPATH.


> </rant>
> 
> Anyway, maybe there are valid technical reasons to not enable ldconfig by
> default; this is not a hill that I'm willing to die on. But wouldn't it be
> nicer if it was easier to enable ldconfig downstream if desired? To that
> end, I tweaked ldconfig.c to not use PATH_MAX (sadly, there are still
> PATH_MAX usages in other places, e.g. chroot_canon.c), and moved a couple
> of files to stop being specific to the Linux port. I hope that both of
> these changes are uncontroversial. With these changes it is possible to
> just patch in use_ldconfig=yes downstream (is desired, of course) and get
> an ld.so.cache-enabled glibc on both i386-gnu and x86_64-gnu.

Removing configuration options and making it simple to configure and use glibc is great
goal. I think that ldconfig should always be enabled and I don't see a downside to making
`use_ldconfig=yes` the default, but I think we'd have to check with Guix or Nix to see if
they have any custom changes there. It involves probably a slightly broader distro
discussion.

-- 
Cheers,
Carlos.

Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Florian Weimer]

* Sergey Bugaev via Libc-alpha:

> @@ -733,25 +723,11 @@ search_dir (const struct dir_entry *entry)
>  			 + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0)
>  	    continue;
>  	}
> -      len += strlen (entry->path) + 2;
> -      if (len > file_name_len)
> -	{
> -	  file_name_len = len;
> -	  file_name = alloca (file_name_len);
> -	  if (!opt_chroot)
> -	    real_file_name = file_name;
> -	}
> -      sprintf (file_name, "%s/%s", entry->path, direntry->d_name);
> +      asprintf (&file_name, "%s/%s", entry->path, direntry->d_name);
>        if (opt_chroot != NULL)
> -	{
> -	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
> -	  if (len > real_file_name_len)
> -	    {
> -	      real_file_name_len = len;
> -	      real_file_name = alloca (real_file_name_len);
> -	    }
> -	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
> -	}
> +	asprintf (&real_file_name, "%s/%s", dir_name, direntry->d_name);
> +      else
> +	real_file_name = file_name;
>  
>        struct stat lstat_buf;
>        /* We optimize and try to do the lstat call only if needed.  */

Missing error checking?

I think we use x* functions such a xstrdup elsewhere in ldconfig, so you
could add xasprintf as well.

Thanks,
Florian

Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache

[Florian Weimer]

* Sergey Bugaev via Libc-alpha:

> Moreover, Debian GNU/Hurd, the primary Hurd-based distribution, has been
> shipping ld.so.cache on Hurd as a downstream patch [1] (note that more
> changes would be required for x86_64-gnu because of FLAG_X8664_LIB64).
> They don't really have a choice, it seems: with their "multiarch"
> filesystem layout, the shared libraries are to be located in
> /lib/i386-gnu/, but that is not a path that ld.so searches for libraries
> in! This is solved by use_ldconfig=yes, and putting /lib/i386-gnu/ into
> /etc/ld.so.conf.d/i386-gnu.conf, where it is then picked up by ldconfig,
> which finds all the libraries and bakes their paths into the cache, where
> ld.so then picks them up.
>
> [1]: https://salsa.debian.org/glibc-team/glibc/-/raw/sid/debian/patches/hurd-i386/local-enable-ldconfig.diff
>
> This is also another thing that suprises me: how come ldconfig does read
> ld.so.conf, but ld.so does not? It's not an "ldconfig.conf" after all...
> How is one even supposed to configure library paths with use_ldconfig=no?

The cache isn't really a cache, it's required for correct operation.

Debian hasn't upstreamed there multi-arch path layouts.  We could
implement multi-arch ldconfig in /etc/ld.so.cache, but with the paths
that Debian currently uses, it's not easy because there's no automated
way ldconfig can recognize the relevant subdirectories.  There's no
intermediate directly like “…/glibc-hwcaps/…” that could serve as a
marker.  I suppose we could look for subdirectories containing libc.so.6
files and its variants as an approximation …

Multi-arch /etc/ld.so.cache needs some new on-disk data structures.

Thanks,
Florian

Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache

[Sergey Bugaev]

Hello,

On Fri, May 19, 2023 at 3:30 PM Florian Weimer <fweimer@redhat.com> wrote:
> Debian hasn't upstreamed there multi-arch path layouts.  We could
> implement multi-arch ldconfig in /etc/ld.so.cache, but with the paths
> that Debian currently uses, it's not easy because there's no automated
> way ldconfig can recognize the relevant subdirectories.  There's no
> intermediate directly like “…/glibc-hwcaps/…” that could serve as a
> marker.  I suppose we could look for subdirectories containing libc.so.6
> files and its variants as an approximation …

Since you're taking my little rant seriously: I'm not arguing that
multi-arch path layouts should be upstreamed or that ld.so should
support that natively likely it does hwcaps. What I'm saying is:

* There is a clear need to configure custom (downstream- or installation-
  specific) paths where shared libraries are to be loaded from, other
  than just /lib and /lib64;

* /etc/ld.so.conf is the proper, well-supported, and popular way to do
  just that, and it is being used by many (if not all) major distros
  for this exact purpose -- in Debian's case, for enabling multiarch
  layout, among other things;

* with use_ldconfig=no, which is the upstream default for non-Linux,
  there does not seem to be a way to achieve that (without resorting to
  LD_LIBRARY_PATH or something like that).

So if use_ldconfig=no is to be kept, there should be another
recommended and supported way to configure search directories. Making
ld.so read ld.so.conf (parsing/globbing questions notwithstanding)
might be one such way.

Or maybe use_ldconfig should just get enabled everywhere :)

On Fri, May 19, 2023 at 2:52 PM Carlos O'Donell <carlos@redhat.com> wrote:
> Removing configuration options and making it simple to configure and use glibc is great
> goal. I think that ldconfig should always be enabled and I don't see a downside to making
> `use_ldconfig=yes` the default, but I think we'd have to check with Guix or Nix to see if
> they have any custom changes there. It involves probably a slightly broader distro
> discussion.

cc'ing Ludovic; but again, why would it be convenient for Nix/Guix to
have use_ldconfig enabled when building their distro with Linux but
not with the Hurd? Surely they'd rather prefer it to be consistent?

I don't think the reason for use_ldconfig=no default is about Guix...
but rather about the ideas that the original Hurd developers have had
for the design of the GNU system (see also: /usr vs /). That hasn't
really panned out; but I understand if the upstream defaults are still
kept according to those plans. Still, using use_ldconfig=yes with Hurd
needs to be better supported, since that's what Hurd distros actually
do in practice (I think -- well, at least Debian does), and if
use_ldconfig=no is to be kept supported, there has to be a way to
configure library search directories without ldconfig.

Sergey

Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Sergey Bugaev]

Hello,

On Fri, May 19, 2023 at 3:25 PM Florian Weimer <fweimer@redhat.com> wrote:
> Missing error checking?

Indeed, thanks. Fixed.

> I think we use x* functions such a xstrdup elsewhere in ldconfig, so you
> could add xasprintf as well.

Apparently the existing code didn't #include <support/support.h> and link
to libsupport (xstrdup must be coming from somewhere else.) I opted to
check for asprintf returning < 0 and calling error () explicitly instead,
like other code in this same file does already.

Sergey

-- >8 --
Subject: [PATCH 1/2 v2] elf: Port ldconfig away from stack-allocated paths

ldconfig was allocating PATH_MAX bytes on the stack for the library file
name. The issues with PATH_MAX usage are well documented [0][1]; even if
a program does not rely on paths being limited to PATH_MAX bytes,
allocating 4096 bytes on the stack for paths that are typically rather
short (strlen ("/lib64/libc.so.6") is 16) is wasteful and dangerous.

[0]: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html
[1]: https://eklitzke.org/path-max-is-tricky

Instead, make use of asprintf to dynamically allocate memory of just the
right size on the heap.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 elf/ldconfig.c | 60 +++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 37 deletions(-)

diff --git a/elf/ldconfig.c b/elf/ldconfig.c
index 2fc45ad8..e643dd57 100644
--- a/elf/ldconfig.c
+++ b/elf/ldconfig.c
@@ -677,28 +677,18 @@ search_dir (const struct dir_entry *entry)
 
   char *dir_name;
   char *real_file_name;
-  size_t real_file_name_len;
-  size_t file_name_len = PATH_MAX;
-  char *file_name = alloca (file_name_len);
+  char *file_name;
   if (opt_chroot != NULL)
-    {
-      dir_name = chroot_canon (opt_chroot, entry->path);
-      real_file_name_len = PATH_MAX;
-      real_file_name = alloca (real_file_name_len);
-    }
+    dir_name = chroot_canon (opt_chroot, entry->path);
   else
-    {
-      dir_name = entry->path;
-      real_file_name_len = 0;
-      real_file_name = file_name;
-    }
+    dir_name = entry->path;
 
   DIR *dir;
   if (dir_name == NULL || (dir = opendir (dir_name)) == NULL)
     {
       if (opt_verbose)
 	error (0, errno, _("Can't open directory %s"), entry->path);
-      if (opt_chroot != NULL && dir_name != NULL)
+      if (opt_chroot != NULL)
 	free (dir_name);
       return;
     }
@@ -733,25 +723,16 @@ search_dir (const struct dir_entry *entry)
 			 + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0)
 	    continue;
 	}
-      len += strlen (entry->path) + 2;
-      if (len > file_name_len)
-	{
-	  file_name_len = len;
-	  file_name = alloca (file_name_len);
-	  if (!opt_chroot)
-	    real_file_name = file_name;
-	}
-      sprintf (file_name, "%s/%s", entry->path, direntry->d_name);
+      if (asprintf (&file_name, "%s/%s", entry->path, direntry->d_name) < 0)
+	error (EXIT_FAILURE, errno, _("Could not form library path"));
       if (opt_chroot != NULL)
 	{
-	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
-	  if (len > real_file_name_len)
-	    {
-	      real_file_name_len = len;
-	      real_file_name = alloca (real_file_name_len);
-	    }
-	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
+	  if (asprintf (&real_file_name, "%s/%s",
+			dir_name, direntry->d_name) < 0)
+	    error (EXIT_FAILURE, errno, _("Could not form library path"));
 	}
+      else
+	real_file_name = file_name;
 
       struct stat lstat_buf;
       /* We optimize and try to do the lstat call only if needed.  */
@@ -761,7 +742,7 @@ search_dir (const struct dir_entry *entry)
 	if (__glibc_unlikely (lstat (real_file_name, &lstat_buf)))
 	  {
 	    error (0, errno, _("Cannot lstat %s"), file_name);
-	    continue;
+	    goto next;
 	  }
 
       struct stat stat_buf;
@@ -778,7 +759,7 @@ search_dir (const struct dir_entry *entry)
 		{
 		  if (strstr (file_name, ".so") == NULL)
 		    error (0, 0, _("Input file %s not found.\n"), file_name);
-		  continue;
+		  goto next;
 		}
 	    }
 	  if (__glibc_unlikely (stat (target_name, &stat_buf)))
@@ -793,7 +774,7 @@ search_dir (const struct dir_entry *entry)
 	      if (opt_chroot != NULL)
 		free (target_name);
 
-	      continue;
+	      goto next;
 	    }
 
 	  if (opt_chroot != NULL)
@@ -806,7 +787,7 @@ search_dir (const struct dir_entry *entry)
 	  lstat_buf.st_ctime = stat_buf.st_ctime;
 	}
       else if (!S_ISREG (lstat_buf.st_mode))
-	continue;
+	goto next;
 
       char *real_name;
       if (opt_chroot != NULL && is_link)
@@ -816,7 +797,7 @@ search_dir (const struct dir_entry *entry)
 	    {
 	      if (strstr (file_name, ".so") == NULL)
 		error (0, 0, _("Input file %s not found.\n"), file_name);
-	      continue;
+	      goto next;
 	    }
 	}
       else
@@ -828,7 +809,7 @@ search_dir (const struct dir_entry *entry)
 	  && __builtin_expect (lstat (real_file_name, &lstat_buf), 0))
 	{
 	  error (0, errno, _("Cannot lstat %s"), file_name);
-	  continue;
+	  goto next;
 	}
 
       /* First search whether the auxiliary cache contains this
@@ -842,7 +823,7 @@ search_dir (const struct dir_entry *entry)
 	    {
 	      if (real_name != real_file_name)
 		free (real_name);
-	      continue;
+	      goto next;
 	    }
 	  else if (opt_build_cache)
 	    add_to_aux_cache (&lstat_buf, flag, isa_level, soname);
@@ -948,6 +929,11 @@ search_dir (const struct dir_entry *entry)
 	  dlib_ptr->next = dlibs;
 	  dlibs = dlib_ptr;
 	}
+
+    next:
+      free (file_name);
+      if (opt_chroot != NULL)
+	free (real_file_name);
     }
 
   closedir (dir);
-- 
2.40.1

Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Adhemerval Zanella Netto]

On 20/05/23 16:03, Sergey Bugaev wrote:
> Hello,
> 
> On Fri, May 19, 2023 at 3:25 PM Florian Weimer <fweimer@redhat.com> wrote:
>> Missing error checking?
> 
> Indeed, thanks. Fixed.
> 
>> I think we use x* functions such a xstrdup elsewhere in ldconfig, so you
>> could add xasprintf as well.
> 
> Apparently the existing code didn't #include <support/support.h> and link
> to libsupport (xstrdup must be coming from somewhere else.) I opted to
> check for asprintf returning < 0 and calling error () explicitly instead,
> like other code in this same file does already.
> 
> Sergey
> 
> -- >8 --
> Subject: [PATCH 1/2 v2] elf: Port ldconfig away from stack-allocated paths
> 
> ldconfig was allocating PATH_MAX bytes on the stack for the library file
> name. The issues with PATH_MAX usage are well documented [0][1]; even if
> a program does not rely on paths being limited to PATH_MAX bytes,
> allocating 4096 bytes on the stack for paths that are typically rather
> short (strlen ("/lib64/libc.so.6") is 16) is wasteful and dangerous.
> 
> [0]: https://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html
> [1]: https://eklitzke.org/path-max-is-tricky
> 
> Instead, make use of asprintf to dynamically allocate memory of just the
> right size on the heap.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

Still looks good to me, thanks.

> ---
>  elf/ldconfig.c | 60 +++++++++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 37 deletions(-)
> 
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 2fc45ad8..e643dd57 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -677,28 +677,18 @@ search_dir (const struct dir_entry *entry)
>  
>    char *dir_name;
>    char *real_file_name;
> -  size_t real_file_name_len;
> -  size_t file_name_len = PATH_MAX;
> -  char *file_name = alloca (file_name_len);
> +  char *file_name;
>    if (opt_chroot != NULL)
> -    {
> -      dir_name = chroot_canon (opt_chroot, entry->path);
> -      real_file_name_len = PATH_MAX;
> -      real_file_name = alloca (real_file_name_len);
> -    }
> +    dir_name = chroot_canon (opt_chroot, entry->path);
>    else
> -    {
> -      dir_name = entry->path;
> -      real_file_name_len = 0;
> -      real_file_name = file_name;
> -    }
> +    dir_name = entry->path;
>  
>    DIR *dir;
>    if (dir_name == NULL || (dir = opendir (dir_name)) == NULL)
>      {
>        if (opt_verbose)
>  	error (0, errno, _("Can't open directory %s"), entry->path);
> -      if (opt_chroot != NULL && dir_name != NULL)
> +      if (opt_chroot != NULL)
>  	free (dir_name);
>        return;
>      }
> @@ -733,25 +723,16 @@ search_dir (const struct dir_entry *entry)
>  			 + 1, ".#prelink#.", sizeof (".#prelink#.") - 1) == 0)
>  	    continue;
>  	}
> -      len += strlen (entry->path) + 2;
> -      if (len > file_name_len)
> -	{
> -	  file_name_len = len;
> -	  file_name = alloca (file_name_len);
> -	  if (!opt_chroot)
> -	    real_file_name = file_name;
> -	}
> -      sprintf (file_name, "%s/%s", entry->path, direntry->d_name);
> +      if (asprintf (&file_name, "%s/%s", entry->path, direntry->d_name) < 0)
> +	error (EXIT_FAILURE, errno, _("Could not form library path"));
>        if (opt_chroot != NULL)
>  	{
> -	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
> -	  if (len > real_file_name_len)
> -	    {
> -	      real_file_name_len = len;
> -	      real_file_name = alloca (real_file_name_len);
> -	    }
> -	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
> +	  if (asprintf (&real_file_name, "%s/%s",
> +			dir_name, direntry->d_name) < 0)
> +	    error (EXIT_FAILURE, errno, _("Could not form library path"));
>  	}
> +      else
> +	real_file_name = file_name;
>  
>        struct stat lstat_buf;
>        /* We optimize and try to do the lstat call only if needed.  */
> @@ -761,7 +742,7 @@ search_dir (const struct dir_entry *entry)
>  	if (__glibc_unlikely (lstat (real_file_name, &lstat_buf)))
>  	  {
>  	    error (0, errno, _("Cannot lstat %s"), file_name);
> -	    continue;
> +	    goto next;
>  	  }
>  
>        struct stat stat_buf;
> @@ -778,7 +759,7 @@ search_dir (const struct dir_entry *entry)
>  		{
>  		  if (strstr (file_name, ".so") == NULL)
>  		    error (0, 0, _("Input file %s not found.\n"), file_name);
> -		  continue;
> +		  goto next;
>  		}
>  	    }
>  	  if (__glibc_unlikely (stat (target_name, &stat_buf)))
> @@ -793,7 +774,7 @@ search_dir (const struct dir_entry *entry)
>  	      if (opt_chroot != NULL)
>  		free (target_name);
>  
> -	      continue;
> +	      goto next;
>  	    }
>  
>  	  if (opt_chroot != NULL)
> @@ -806,7 +787,7 @@ search_dir (const struct dir_entry *entry)
>  	  lstat_buf.st_ctime = stat_buf.st_ctime;
>  	}
>        else if (!S_ISREG (lstat_buf.st_mode))
> -	continue;
> +	goto next;
>  
>        char *real_name;
>        if (opt_chroot != NULL && is_link)
> @@ -816,7 +797,7 @@ search_dir (const struct dir_entry *entry)
>  	    {
>  	      if (strstr (file_name, ".so") == NULL)
>  		error (0, 0, _("Input file %s not found.\n"), file_name);
> -	      continue;
> +	      goto next;
>  	    }
>  	}
>        else
> @@ -828,7 +809,7 @@ search_dir (const struct dir_entry *entry)
>  	  && __builtin_expect (lstat (real_file_name, &lstat_buf), 0))
>  	{
>  	  error (0, errno, _("Cannot lstat %s"), file_name);
> -	  continue;
> +	  goto next;
>  	}
>  
>        /* First search whether the auxiliary cache contains this
> @@ -842,7 +823,7 @@ search_dir (const struct dir_entry *entry)
>  	    {
>  	      if (real_name != real_file_name)
>  		free (real_name);
> -	      continue;
> +	      goto next;
>  	    }
>  	  else if (opt_build_cache)
>  	    add_to_aux_cache (&lstat_buf, flag, isa_level, soname);
> @@ -948,6 +929,11 @@ search_dir (const struct dir_entry *entry)
>  	  dlib_ptr->next = dlibs;
>  	  dlibs = dlib_ptr;
>  	}
> +
> +    next:
> +      free (file_name);
> +      if (opt_chroot != NULL)
> +	free (real_file_name);
>      }
>  
>    closedir (dir);

Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache

[Ludovic Courtès]

Hi,

Sergey Bugaev <bugaevc@gmail.com> skribis:

> On Fri, May 19, 2023 at 2:52 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> Removing configuration options and making it simple to configure and use glibc is great
>> goal. I think that ldconfig should always be enabled and I don't see a downside to making
>> `use_ldconfig=yes` the default, but I think we'd have to check with Guix or Nix to see if
>> they have any custom changes there. It involves probably a slightly broader distro
>> discussion.

I missed the beginning of the discussion and I fear I’m slightly
changing topics :-), but to me we should work toward per-application
loader caches:

  https://guix.gnu.org/en/blog/2021/taming-the-stat-storm-with-a-loader-cache/
  https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/patches/glibc-dl-cache.patch?id=6d0571215d661d21cac2150ca45906e77a79a5fb

This is the one custom change we have in Guix.

(And IMO it should work the same on Linux and on the Hurd.)

Ludo’.

Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths

[Florian Weimer]

* Sergey Bugaev:

> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 2fc45ad8..e643dd57 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c

> -	  len = strlen (dir_name) + strlen (direntry->d_name) + 2;
> -	  if (len > real_file_name_len)
> -	    {
> -	      real_file_name_len = len;
> -	      real_file_name = alloca (real_file_name_len);
> -	    }
> -	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
> +	  if (asprintf (&real_file_name, "%s/%s",
> +			dir_name, direntry->d_name) < 0)
> +	    error (EXIT_FAILURE, errno, _("Could not form library path"));
>  	}
> +      else
> +	real_file_name = file_name;

Maybe use xstrdup (file_name) here and free unconditionally below?
It makes it easier to analyze the code for use-after-free errors.

Rest looks okay.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian