bug#43491: Fakechroot execution engine can fail to find libraries

[Ludovic Courtès]

Ludovic Courtès <ludo@gnu.org> skribis:

> Ludovic Courtès <ludovic.courtes@inria.fr> skribis:
>
>> Indeed, we can see ‘stat’ calls passed raw /gnu/store file names from
>> RUNPATH entries (instead of /tmp/fakechroot-test/gnu/store), suggesting
>> that ‘la_objsearch’ didn’t have a chance to rewrite them:
>
> This is probably an ld.so bug:
>
>   https://sourceware.org/bugzilla/show_bug.cgi?id=26634

The patch below provides a fix/workaround for glibc, confirming the
hypothesis above.  (I don’t think we should apply this patch though,
rather we’ll work around the issue in ‘guix pack’.)

Ludo’.

diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm
index c83775d8ee..fa4da801af 100644
--- a/gnu/packages/base.scm
+++ b/gnu/packages/base.scm
@@ -685,6 +685,7 @@ the store.")
   (package
    (name "glibc")
    (version "2.31")
+   (replacement glibc-2.31/fixed)
    (source (origin
             (method url-fetch)
             (uri (string-append "mirror://gnu/glibc/glibc-" version ".tar.xz"))
@@ -933,6 +934,15 @@ with the Linux kernel.")
    (license lgpl2.0+)
    (home-page "https://www.gnu.org/software/libc/";)))
 
+(define-public glibc-2.31/fixed
+  (package
+    (inherit glibc)
+    (version "2.3A")
+    (source (origin
+              (inherit (package-source glibc))
+              (patches (append (search-patches "glibc-audit-stat.patch")
+                               (origin-patches (package-source glibc))))))))
+
 ;; Below are old libc versions, which we use mostly to build locale data in
 ;; the old format (which the new libc cannot cope with.)
 
--- /dev/null
+++ b/gnu/packages/patches/glibc-audit-stat.patch
@@ -0,0 +1,126 @@
+diff --git a/elf/dl-load.c b/elf/dl-load.c
+index a6b80f9395..9daa32f76b 100644
+--- a/elf/dl-load.c
++++ b/elf/dl-load.c
+@@ -1461,11 +1461,15 @@ print_search_path (struct r_search_path_elem **list,
+    user might want to know about this.
+ 
+    If FD is not -1, then the file is already open and FD refers to it.
+-   In that case, FD is consumed for both successful and error returns.  */
++   In that case, FD is consumed for both successful and error returns.
++
++   Set *CHANGED_BY_AUDIT to true if the audit module provided a file name
++   different from NAME.  */
+ static int
+ open_verify (const char *name, int fd,
+              struct filebuf *fbp, struct link_map *loader,
+-           int whatcode, int mode, bool *found_other_class, bool free_name)
++           int whatcode, int mode, bool *found_other_class, bool free_name,
++           bool *changed_by_audit)
+ {
+   /* This is the expected ELF header.  */
+ #define ELF32_CLASS ELFCLASS32
+@@ -1500,6 +1504,8 @@ open_verify (const char *name, int fd,
+   const char *errstring = NULL;
+   int errval = 0;
+ 
++  *changed_by_audit = false;
++
+ #ifdef SHARED
+   /* Give the auditing libraries a chance.  */
+   if (__glibc_unlikely (GLRO(dl_naudit) > 0) && whatcode != 0
+@@ -1521,12 +1527,19 @@ open_verify (const char *name, int fd,
+         afct = afct->next;
+       }
+ 
+-      if (fd != -1 && name != original_name && strcmp (name, original_name))
++      if (name != original_name && strcmp (name, original_name))
+         {
+-          /* An audit library changed what we're supposed to open,
+-             so FD no longer matches it.  */
+-          __close_nocancel (fd);
+-          fd = -1;
++        /* Tell the caller we're looking at something different from
++           ORIGINAL_NAME.  */
++        *changed_by_audit = true;
++
++        if (fd != -1)
++          {
++            /* An audit library changed what we're supposed to open, so FD
++               no longer matches it.  */
++            __close_nocancel (fd);
++            fd = -1;
++          }
+         }
+     }
+ #endif
+@@ -1782,6 +1795,7 @@ open_path (const char *name, size_t namelen, int mode,
+       char *edp;
+       int here_any = 0;
+       int err;
++      bool changed_by_audit;
+ 
+       /* If we are debugging the search for libraries print the path
+        now if it hasn't happened now.  */
+@@ -1810,16 +1824,19 @@ open_path (const char *name, size_t namelen, int mode,
+           _dl_debug_printf ("  trying file=%s\n", buf);
+ 
+         fd = open_verify (buf, -1, fbp, loader, whatcode, mode,
+-                          found_other_class, false);
++                          found_other_class, false,
++                          &changed_by_audit);
+         if (this_dir->status[cnt] == unknown)
+           {
+             if (fd != -1)
+               this_dir->status[cnt] = existing;
+             /* Do not update the directory information when loading
+                auditing code.  We must try to disturb the program as
+-               little as possible.  */
+-            else if (loader == NULL
+-                     || GL(dl_ns)[loader->l_ns]._ns_loaded->l_auditing == 0)
++               little as possible.  Additionally, if the audit module
++               change the file name, keep directory information as is.  */
++            else if ((loader == NULL
++                      || GL(dl_ns)[loader->l_ns]._ns_loaded->l_auditing == 0)
++                     && !changed_by_audit)
+               {
+                 /* We failed to open machine dependent library.  Let's
+                    test whether there is any directory at all.  */
+@@ -2064,10 +2081,11 @@ _dl_map_object (struct link_map *loader, const char 
*name,
+           realname = _dl_sysdep_open_object (name, namelen, &fd);
+           if (realname != NULL)
+             {
++            bool changed_by_audit;
+               fd = open_verify (realname, fd,
+                                 &fb, loader ?: GL(dl_ns)[nsid]._ns_loaded,
+                                 LA_SER_CONFIG, mode, &found_other_class,
+-                                false);
++                                false, &changed_by_audit);
+               if (fd == -1)
+                 free (realname);
+             }
+@@ -2118,10 +2136,11 @@ _dl_map_object (struct link_map *loader, const char 
*name,
+ 
+             if (cached != NULL)
+               {
++                bool changed_by_audit;
+                 fd = open_verify (cached, -1,
+                                   &fb, loader ?: GL(dl_ns)[nsid]._ns_loaded,
+                                   LA_SER_CONFIG, mode, &found_other_class,
+-                                  false);
++                                  false, &changed_by_audit);
+                 if (__glibc_likely (fd != -1))
+                   realname = cached;
+                 else
+@@ -2153,9 +2172,10 @@ _dl_map_object (struct link_map *loader, const char 
*name,
+       fd = -1;
+       else
+       {
++        bool changed_by_audit;
+         fd = open_verify (realname, -1, &fb,
+                           loader ?: GL(dl_ns)[nsid]._ns_loaded, 0, mode,
+-                          &found_other_class, true);
++                          &found_other_class, true, &changed_by_audit);
+         if (__glibc_unlikely (fd == -1))
+           free (realname);
+       }