07/09: pack: Relocatable wrapper leaves root available to child processes.

[guix-commits]

civodul pushed a commit to branch master
in repository guix.

commit bfe82fe2f6e9f34c0774fe2114cdc7e937ba8bd2
Author: Ludovic Courtès <ludo@gnu.org>
AuthorDate: Sat Oct 31 23:02:33 2020 +0100

    pack: Relocatable wrapper leaves root available to child processes.
    
    Fixes <https://bugs.gnu.org/44261>.
    Reported by Jan Nieuwenhuizen <janneke@gnu.org>.
    
    * gnu/packages/aux-files/run-in-namespace.c (exec_in_user_namespace):
    Add call to 'prctl'.  Call 'mount' for NEW_ROOT and define 'is_tmpfs'.
    When IS_TMPFS is true, call 'umount' and 'rmdir' after 'waitpid';
    otherwise, call 'rm_rf' only when 'waitpid' returns -1 the second time.
    (exec_with_loader): Call 'prctl'.  Remove NEW_ROOT only when 'waitpid'
    returns -1 the second time, otherwise leave it behind.
    * tests/guix-pack-relocatable.sh (wait_for_file): New function.
    Add test.
---
 gnu/packages/aux-files/run-in-namespace.c | 52 ++++++++++++++++++----
 tests/guix-pack-relocatable.sh            | 73 +++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/gnu/packages/aux-files/run-in-namespace.c 
b/gnu/packages/aux-files/run-in-namespace.c
index 947ff02..44c3c5a 100644
--- a/gnu/packages/aux-files/run-in-namespace.c
+++ b/gnu/packages/aux-files/run-in-namespace.c
@@ -41,6 +41,7 @@
 #include <fcntl.h>
 #include <dirent.h>
 #include <sys/syscall.h>
+#include <sys/prctl.h>
 
 /* Whether we're building the ld.so/libfakechroot wrapper.  */
 #define HAVE_EXEC_WITH_LOADER                                          \
@@ -258,11 +259,20 @@ exec_in_user_namespace (const char *store, int argc, char 
*argv[])
 {
   /* Spawn @WRAPPED_PROGRAM@ in a separate namespace where STORE is
      bind-mounted in the right place.  */
-  int err;
+  int err, is_tmpfs;
   char *new_root = mkdtemp (strdup ("/tmp/guix-exec-XXXXXX"));
   char *new_store = concat (new_root, original_store);
   char *cwd = get_current_dir_name ();
 
+  /* Become the new parent of grand-children when their parent dies.  */
+  prctl (PR_SET_CHILD_SUBREAPER, 1);
+
+  /* Optionally, make NEW_ROOT a tmpfs.  That way, if we have to leave it
+     behind because there are sub-processes still running when this wrapper
+     exits, it's OK.  */
+  err = mount ("none", new_root, "tmpfs", 0, NULL);
+  is_tmpfs = (err == 0);
+
   /* Create a child with separate namespaces and set up bind-mounts from
      there.  That way, bind-mounts automatically disappear when the child
      exits, which simplifies cleanup for the parent.  Note: clone is more
@@ -300,6 +310,7 @@ exec_in_user_namespace (const char *store, int argc, char 
*argv[])
       /* Failure: user namespaces not supported.  */
       fprintf (stderr, "%s: error: 'clone' failed: %m\n", argv[0]);
       rm_rf (new_root);
+      free (new_root);
       break;
 
     default:
@@ -312,10 +323,25 @@ exec_in_user_namespace (const char *store, int argc, char 
*argv[])
        write_id_map (child, "uid_map", getuid ());
        write_id_map (child, "gid_map", getgid ());
 
-       int status;
+       int status, status_other;
        waitpid (child, &status, 0);
-       chdir ("/");                      /* avoid EBUSY */
-       rm_rf (new_root);
+
+       chdir ("/");                              /* avoid EBUSY */
+       if (is_tmpfs)
+         {
+           /* NEW_ROOT lives on in child processes and we no longer need it
+              to exist as an empty directory in the global namespace.  */
+           umount (new_root);
+           rmdir (new_root);
+         }
+       /* Check whether there are child processes left.  If there are none,
+          we can remove NEW_ROOT just fine.  Conversely, if there are
+          processes left (for example because this wrapper's child forked),
+          we have to leave NEW_ROOT behind so that those processes can still
+          access their root file system (XXX).  */
+       else if (waitpid (-1 , &status_other, WNOHANG) == -1)
+         rm_rf (new_root);
+
        free (new_root);
 
        if (WIFEXITED (status))
@@ -490,6 +516,9 @@ exec_with_loader (const char *store, int argc, char *argv[])
 
   setenv ("FAKECHROOT_BASE", new_root, 1);
 
+  /* Become the new parent of grand-children when their parent dies.  */
+  prctl (PR_SET_CHILD_SUBREAPER, 1);
+
   pid_t child = fork ();
   switch (child)
     {
@@ -507,12 +536,19 @@ exec_with_loader (const char *store, int argc, char 
*argv[])
 
     default:
       {
-       int status;
+       int status, status_other;
        waitpid (child, &status, 0);
-       chdir ("/");                      /* avoid EBUSY */
-       rm_rf (new_root);
-       free (new_root);
 
+       /* If there are child processes still running, leave NEW_ROOT around
+          so they can still access it.  XXX: In that case NEW_ROOT is left
+          behind.  */
+       if (waitpid (-1 , &status_other, WNOHANG) == -1)
+         {
+           chdir ("/");                          /* avoid EBUSY */
+           rm_rf (new_root);
+         }
+
+       free (new_root);
        close (2);                      /* flushing stderr should be silent */
 
        if (WIFEXITED (status))
diff --git a/tests/guix-pack-relocatable.sh b/tests/guix-pack-relocatable.sh
index eb04231..2beb1b1 100644
--- a/tests/guix-pack-relocatable.sh
+++ b/tests/guix-pack-relocatable.sh
@@ -59,6 +59,19 @@ run_without_store ()
     fi
 }
 
+# Wait for the given file to show up.  Error out if it doesn't show up in a
+# timely fashion.
+wait_for_file ()
+{
+    i=0
+    while ! test -f "$1" && test $i -lt 20
+    do
+       sleep 0.3
+       i=`expr $i + 1`
+    done
+    test -f "$1"
+}
+
 test_directory="`mktemp -d`"
 export test_directory
 trap 'chmod -Rf +w "$test_directory"; rm -rf "$test_directory"' EXIT
@@ -131,6 +144,66 @@ case "`uname -m`" in
        ;;
 esac
 
+if unshare -r true
+then
+    # Check what happens if the wrapped binary forks and leaves child
+    # processes behind, like a daemon.  The root file system should remain
+    # available to those child processes.  See <https://bugs.gnu.org/44261>.
+    cat > "$test_directory/manifest.scm" <<EOF
+(use-modules (guix))
+
+(define daemon
+  (program-file "daemon"
+                #~(begin
+                    (use-modules (ice-9 match)
+                                 (ice-9 ftw))
+
+                    (call-with-output-file "parent-store"
+                      (lambda (port)
+                        (write (scandir (ungexp (%store-prefix)))
+                               port)))
+
+                    (match (primitive-fork)
+                      (0 (sigaction SIGHUP (const #t))
+                         (call-with-output-file "pid"
+                           (lambda (port)
+                             (display (getpid) port)))
+                         (pause)
+                         (call-with-output-file "child-store"
+                           (lambda (port)
+                             (write (scandir (ungexp (%store-prefix)))
+                                    port))))
+                      (_ #t)))))
+
+(define package
+  (computed-file "package"
+                 #~(let ((out (ungexp output)))
+                     (mkdir out)
+                     (mkdir (string-append out "/bin"))
+                     (symlink (ungexp daemon)
+                              (string-append out "/bin/daemon")))))
+
+(manifest (list (manifest-entry
+                  (name "daemon")
+                  (version "0")
+                  (item package))))
+EOF
+
+    tarball="$(guix pack -S /bin=bin -R -m "$test_directory/manifest.scm")"
+    (cd "$test_directory"; tar xf "$tarball")
+
+    # Run '/bin/daemon', which forks, then wait for the child, send it SIGHUP
+    # so that it dumps its view of the store, and make sure the child and
+    # parent both see the same store contents.
+    (cd "$test_directory"; run_without_store ./bin/daemon)
+    wait_for_file "$test_directory/pid"
+    kill -HUP $(cat "$test_directory/pid")
+    wait_for_file "$test_directory/child-store"
+    diff -u "$test_directory/parent-store" "$test_directory/child-store"
+
+    chmod -Rf +w "$test_directory"; rm -rf "$test_directory"/*
+fi
+
 # Ensure '-R' works with outputs other than "out".
 tarball="`guix pack -R -S /share=share groff:doc`"
 (cd "$test_directory"; tar xf "$tarball")