diff mbox

[bug#41189,0/3] Add Fakechroot engine for 'guix pack -RR'

Message ID 87r1vpbhce.fsf@gnu.org
State Accepted
Headers show

Commit Message

Ludovic Courtès May 12, 2020, 10:03 a.m. UTC 
Hi Carlos,

Carlos O'Donell <carlos@redhat.com> skribis:

> There are two issues at hand:
> * Standard namespace issues (conformance)
> * PLT avoidance issues (performance)
>
> See:
> https://sourceware.org/glibc/wiki/Style_and_Conventions#Double-underscore_names_for_public_API_functions
>
> It is an internal implementation detail that open(2) is being called by
> the library, and as such glibc bypasses the ELF interposable symbol
> open, and instead calls open directly without this being visible to the
> application.
>
> There are many such cases where we bypass the ELF interposable symbol to
> provide standard namespace cleanliness, performance, and so provide consistent
> behaviour.

It makes sense to me, thanks for explaining.

> Yes, in your case this means you cannot override the behaviour of the
> interface without using some kind of bind mount, or mount namespace
> (to provide an alternate view of the filesystem).

Agreed, unprivileged user namespaces with bind mounts are the preferred
solution; the LD_PRELOAD hack discussed here is for when they’re
unavailable and PRoot is too slow.

> We would have to argue upstream that some minimal subset of the filesystem
> access should be interposable via open/close/read/write, but that's going
> to get difficult quickly and have significant performance problems.

Yes, understood.  (I wasn’t going to suggest it.  :-))

> It would be simpler, IMO, to set LOCPATH and GCONV_PATH appropriately and
> alter the runtime behaviour that way. If that doesn't work, perhaps because
> of setuid, then we can discuss further.

Yes, setting ‘GCONV_PATH’ in particular seems like something the wrapper
could automatically do.  The attached patch does that and now Guile runs
fine with the ld.so/fakechroot “engine”.

One thing that won’t work is dlopen because our ‘--library-path’
argument is computed statically based on the RUNPATH of the wrapped
program.  So for instance if you try to load guile-readline.so from
Guile, it eventually fails because libreadline.so isn’t found
(libreadline.so is in the RUNPATH of guile-readline.so, but the loader
uses non-interposable calls here as well.)  Probably no simple solution
to that one.

Thanks for your feedback, Carlos!

Ludo’.

Comments

Carlos O'Donell May 12, 2020, 12:09 p.m. UTC | #1 
On 5/12/20 6:03 AM, Ludovic Courtès wrote:
> One thing that won’t work is dlopen because our ‘--library-path’
> argument is computed statically based on the RUNPATH of the wrapped
> program.  So for instance if you try to load guile-readline.so from
> Guile, it eventually fails because libreadline.so isn’t found
> (libreadline.so is in the RUNPATH of guile-readline.so, but the loader
> uses non-interposable calls here as well.)  Probably no simple solution
> to that one.

There is a simple solution. You need to write a dynamic loader audit module
that handles la_objsearch() and inject your lookup path. See man 7 rtld-audit.
The dynamic loader audit modules allow you to alter the loader's core behaviour
with a plugin.
diff mbox

Patch

diff --git a/gnu/packages/aux-files/run-in-namespace.c b/gnu/packages/aux-files/run-in-namespace.c
index ed72a169f2..c56c35a510 100644
--- a/gnu/packages/aux-files/run-in-namespace.c
+++ b/gnu/packages/aux-files/run-in-namespace.c
@@ -425,6 +427,15 @@  exec_with_loader (const char *store, int argc, char *argv[])
   mkdir_p (new_store_parent);
   symlink (store, new_store);
 
+#ifdef GCONV_DIRECTORY
+  /* Tell libc where to find its gconv modules.  This is necessary because
+     gconv uses non-interposable 'open' calls.  */
+  char *gconv_path = concat (store,
+			     GCONV_DIRECTORY + sizeof "@STORE_DIRECTORY@");
+  setenv ("GCONV_PATH", gconv_path, 1);
+  free (gconv_path);
+#endif
+
   setenv ("FAKECHROOT_BASE", new_root, 1);
 
   pid_t child = fork ();
diff --git a/guix/scripts/pack.scm b/guix/scripts/pack.scm
index 2d856066b2..2b37bf5027 100644
--- a/guix/scripts/pack.scm
+++ b/guix/scripts/pack.scm
@@ -739,6 +739,12 @@  last resort for relocation."
                                    bv 0 (bytevector-length bv))
                  (utf8->string bv)))))
 
+          (define (gconv-directory directory)
+            ;; Return DIRECTORY/gconv if it exists as a directory.
+            (let ((gconv (string-append directory "/gconv")))
+              (and (directory-exists? gconv)
+                   gconv)))
+
           (define (elf-loader-compile-flags program)
             ;; Return the cpp flags defining macros for the ld.so/fakechroot
             ;; wrapper of PROGRAM.
@@ -750,8 +756,9 @@  last resort for relocation."
                   (match (elf-dynamic-info elf)
                     (#f '())
                     (dyninfo
-                     (let ((runpath (elf-dynamic-info-runpath dyninfo))
-                           (interp  (elf-interpreter elf)))
+                     (let* ((runpath (elf-dynamic-info-runpath dyninfo))
+                            (gconv   (any gconv-directory runpath))
+                            (interp  (elf-interpreter elf)))
                        (if interp
                            (list (string-append "-DPROGRAM_INTERPRETER=\""
                                                 interp "\"")
@@ -762,7 +769,12 @@  last resort for relocation."
                                                  ", ")
                                                 ", NULL }")
                                  (string-append "-DFAKECHROOT_LIBRARY=\""
-                                                #$(fakechroot-library) "\""))
+                                                #$(fakechroot-library) "\"")
+
+                                 (if gconv
+                                     (string-append "-DGCONV_DIRECTORY=\""
+                                                    gconv "\"")
+                                     "-UGCONV_DIRECTORY"))
                            '())))))
                 '()))