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