* [RFC PATCH 0/2] On ldconfig and ld.so.cache @ 2023-05-17 18:54 Sergey Bugaev 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Sergey Bugaev @ 2023-05-17 18:54 UTC (permalink / raw) To: libc-alpha, bug-hurd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-17 18:54 [RFC PATCH 0/2] On ldconfig and ld.so.cache Sergey Bugaev @ 2023-05-17 18:54 ` Sergey Bugaev 2023-05-18 19:13 ` Adhemerval Zanella Netto 2023-05-19 12:25 ` Florian Weimer 2023-05-17 18:54 ` [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific Sergey Bugaev ` (2 subsequent siblings) 3 siblings, 2 replies; 13+ messages in thread From: Sergey Bugaev @ 2023-05-17 18:54 UTC (permalink / raw) To: libc-alpha, bug-hurd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev @ 2023-05-18 19:13 ` Adhemerval Zanella Netto 2023-05-19 12:25 ` Florian Weimer 1 sibling, 0 replies; 13+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-18 19:13 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha, bug-hurd 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); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev 2023-05-18 19:13 ` Adhemerval Zanella Netto @ 2023-05-19 12:25 ` Florian Weimer 2023-05-20 19:03 ` Sergey Bugaev 1 sibling, 1 reply; 13+ messages in thread From: Florian Weimer @ 2023-05-19 12:25 UTC (permalink / raw) To: Sergey Bugaev via Libc-alpha; +Cc: bug-hurd, Sergey Bugaev * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-19 12:25 ` Florian Weimer @ 2023-05-20 19:03 ` Sergey Bugaev 2023-05-23 17:57 ` Adhemerval Zanella Netto 2023-05-25 8:07 ` Florian Weimer 0 siblings, 2 replies; 13+ messages in thread From: Sergey Bugaev @ 2023-05-20 19:03 UTC (permalink / raw) To: libc-alpha, Florian Weimer; +Cc: bug-hurd, Adhemerval Zanella Netto 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-20 19:03 ` Sergey Bugaev @ 2023-05-23 17:57 ` Adhemerval Zanella Netto 2023-05-25 8:07 ` Florian Weimer 1 sibling, 0 replies; 13+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-23 17:57 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha, Florian Weimer; +Cc: bug-hurd 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); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths 2023-05-20 19:03 ` Sergey Bugaev 2023-05-23 17:57 ` Adhemerval Zanella Netto @ 2023-05-25 8:07 ` Florian Weimer 1 sibling, 0 replies; 13+ messages in thread From: Florian Weimer @ 2023-05-25 8:07 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha, bug-hurd, Adhemerval Zanella Netto * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific 2023-05-17 18:54 [RFC PATCH 0/2] On ldconfig and ld.so.cache Sergey Bugaev 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev @ 2023-05-17 18:54 ` Sergey Bugaev 2023-05-19 11:36 ` Carlos O'Donell 2023-05-19 11:52 ` [RFC PATCH 0/2] On ldconfig and ld.so.cache Carlos O'Donell 2023-05-19 12:30 ` Florian Weimer 3 siblings, 1 reply; 13+ messages in thread From: Sergey Bugaev @ 2023-05-17 18:54 UTC (permalink / raw) To: libc-alpha, bug-hurd 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific 2023-05-17 18:54 ` [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific Sergey Bugaev @ 2023-05-19 11:36 ` Carlos O'Donell 0 siblings, 0 replies; 13+ messages in thread From: Carlos O'Donell @ 2023-05-19 11:36 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha, bug-hurd 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache 2023-05-17 18:54 [RFC PATCH 0/2] On ldconfig and ld.so.cache Sergey Bugaev 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev 2023-05-17 18:54 ` [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific Sergey Bugaev @ 2023-05-19 11:52 ` Carlos O'Donell 2023-05-19 12:30 ` Florian Weimer 3 siblings, 0 replies; 13+ messages in thread From: Carlos O'Donell @ 2023-05-19 11:52 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha, bug-hurd 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache 2023-05-17 18:54 [RFC PATCH 0/2] On ldconfig and ld.so.cache Sergey Bugaev ` (2 preceding siblings ...) 2023-05-19 11:52 ` [RFC PATCH 0/2] On ldconfig and ld.so.cache Carlos O'Donell @ 2023-05-19 12:30 ` Florian Weimer 2023-05-19 13:21 ` Sergey Bugaev 3 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2023-05-19 12:30 UTC (permalink / raw) To: Sergey Bugaev via Libc-alpha; +Cc: bug-hurd, Sergey Bugaev * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache 2023-05-19 12:30 ` Florian Weimer @ 2023-05-19 13:21 ` Sergey Bugaev 2023-05-24 14:09 ` Ludovic Courtès 0 siblings, 1 reply; 13+ messages in thread From: Sergey Bugaev @ 2023-05-19 13:21 UTC (permalink / raw) To: Florian Weimer, Carlos O'Donell Cc: bug-hurd, libc-alpha, Ludovic Courtès 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC PATCH 0/2] On ldconfig and ld.so.cache 2023-05-19 13:21 ` Sergey Bugaev @ 2023-05-24 14:09 ` Ludovic Courtès 0 siblings, 0 replies; 13+ messages in thread From: Ludovic Courtès @ 2023-05-24 14:09 UTC (permalink / raw) To: Sergey Bugaev; +Cc: Florian Weimer, Carlos O'Donell, bug-hurd, libc-alpha 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’. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-05-25 8:08 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-17 18:54 [RFC PATCH 0/2] On ldconfig and ld.so.cache Sergey Bugaev 2023-05-17 18:54 ` [RFC PATCH 1/2] elf: Port ldconfig away from stack-allocated paths Sergey Bugaev 2023-05-18 19:13 ` Adhemerval Zanella Netto 2023-05-19 12:25 ` Florian Weimer 2023-05-20 19:03 ` Sergey Bugaev 2023-05-23 17:57 ` Adhemerval Zanella Netto 2023-05-25 8:07 ` Florian Weimer 2023-05-17 18:54 ` [RFC PATCH 2/2] x86: Make dl-cache.h and readelflib.c not Linux-specific Sergey Bugaev 2023-05-19 11:36 ` Carlos O'Donell 2023-05-19 11:52 ` [RFC PATCH 0/2] On ldconfig and ld.so.cache Carlos O'Donell 2023-05-19 12:30 ` Florian Weimer 2023-05-19 13:21 ` Sergey Bugaev 2023-05-24 14:09 ` Ludovic Courtès
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).