From: Daan De Meyer <daan.j.demeyer@gmail.com>
Date: Sun, 22 Dec 2024 11:47:06 +0000 (+0100)
Subject: Wait until sandbox exec()'s specified command before continuing
X-Git-Tag: v25~96^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F3298%2Fhead;p=thirdparty%2Fmkosi.git

Wait until sandbox exec()'s specified command before continuing

Let's wait in spawn() until all setup logic has completed before
continuing if a sandbox is used. When using spawn() without run(),
this helps us fail early if the setup logic fails instead of later
on if we're not checking the result of the spawn() command immediately
afterwards.

As a side effect, this also acts as a synchronization point when using
systemd-run --scope where when spawn() returns, we can be 100% sure that
the scope has been created, which is important when calling RegisterMachine
from systemd-machined which does the wrong thing if the scope for the specified
machine does not yet exist and ends up killing the parent unit (often the user
session) instead of just the virtual machine.
---

diff --git a/mkosi/resources/man/mkosi-sandbox.1.md b/mkosi/resources/man/mkosi-sandbox.1.md
index b171e250a..e2103b512 100644
--- a/mkosi/resources/man/mkosi-sandbox.1.md
+++ b/mkosi/resources/man/mkosi-sandbox.1.md
@@ -112,6 +112,11 @@ host system.
 `--unshare-ipc`
 :   Specifying this option makes `mkosi-sandbox` unshare an IPC namespace if possible.
 
+`--exec-fd FD`
+:   The specified `FD` will be closed when `mkosi-sandbox` calls `execvp()`. This is useful
+    to wait until all setup logic has completed before continuing execution in the parent
+    process invoking `mkosi-sandbox`.
+
 `--version`
 :   Show package version.
 
diff --git a/mkosi/run.py b/mkosi/run.py
index fb483319d..34d8d4687 100644
--- a/mkosi/run.py
+++ b/mkosi/run.py
@@ -167,6 +167,12 @@ def run(
     return CompletedProcess(cmdline, process.returncode, out, err)
 
 
+def fd_move_above(fd: int, above: int) -> int:
+    dup = fcntl.fcntl(fd, fcntl.F_DUPFD, above)
+    os.close(fd)
+    return dup
+
+
 def preexec(
     *,
     foreground: bool,
@@ -262,11 +268,23 @@ def spawn(
         prefix = [os.fspath(x) for x in sbx]
 
         if prefix:
-            prefix += ["--"]
+            prfd, pwfd = os.pipe2(os.O_CLOEXEC)
+
+            # Make sure the write end of the pipe (which we pass to the subprocess) is higher than all the
+            # file descriptors we'll pass to the subprocess, so that it doesn't accidentally get closed by
+            # the logic in preexec().
+            if pass_fds:
+                pwfd = fd_move_above(pwfd, list(pass_fds)[-1])
+
+            exec_prefix = ["--exec-fd", f"{SD_LISTEN_FDS_START + len(pass_fds)}", "--"]
+            pass_fds = [*pass_fds, pwfd]
+        else:
+            exec_prefix = []
+            prfd, pwfd = None, None
 
         try:
             with subprocess.Popen(
-                [*prefix, *cmdline],
+                [*prefix, *exec_prefix, *cmdline],
                 stdin=stdin,
                 stdout=stdout,
                 stderr=stderr,
@@ -285,15 +303,28 @@ def spawn(
                     pass_fds=pass_fds,
                 ),
             ) as proc:
+                if pwfd is not None:
+                    os.close(pwfd)
+
+                if prfd is not None:
+                    os.read(prfd, 1)
+                    os.close(prfd)
+
+                def failed() -> bool:
+                    return check and (rc := proc.poll()) is not None and rc not in success_exit_status
+
                 try:
-                    yield proc
+                    # Don't bother yielding if we've already failed by the time we get here. We'll raise an
+                    # exception later on so it's not a problem that we don't yield at all.
+                    if not failed():
+                        yield proc
                 except BaseException:
                     proc.terminate()
                     raise
                 finally:
                     returncode = proc.wait()
 
-                if check and returncode not in success_exit_status:
+                if failed():
                     if log:
                         log_process_failure(prefix, cmd, returncode)
                     if ARG_DEBUG_SHELL.get():
diff --git a/mkosi/sandbox.py b/mkosi/sandbox.py
index 51ae6a3cf..834337204 100755
--- a/mkosi/sandbox.py
+++ b/mkosi/sandbox.py
@@ -30,6 +30,9 @@ CLONE_NEWNS = 0x00020000
 CLONE_NEWUSER = 0x10000000
 EPERM = 1
 ENOENT = 2
+F_GETFD = 1
+F_SETFD = 2
+FD_CLOEXEC = 1
 LINUX_CAPABILITY_U32S_3 = 2
 LINUX_CAPABILITY_VERSION_3 = 0x20080522
 MNT_DETACH = 2
@@ -93,6 +96,7 @@ libc.pivot_root.argtypes = (ctypes.c_char_p, ctypes.c_char_p)
 libc.umount2.argtypes = (ctypes.c_char_p, ctypes.c_int)
 libc.capget.argtypes = (ctypes.c_void_p, ctypes.c_void_p)
 libc.capset.argtypes = (ctypes.c_void_p, ctypes.c_void_p)
+libc.fcntl.argtypes = (ctypes.c_int, ctypes.c_int, ctypes.c_int)
 
 
 def oserror(filename: str = "") -> None:
@@ -410,6 +414,11 @@ def is_relative_to(one: str, two: str) -> bool:
     return os.path.commonpath((one, two)) == two
 
 
+def fd_make_cloexec(fd: int) -> None:
+    flags = libc.fcntl(fd, F_GETFD, 0)
+    libc.fcntl(fd, F_SETFD, flags | FD_CLOEXEC)
+
+
 class FSOperation:
     def __init__(self, dst: str) -> None:
         self.dst = dst
@@ -694,6 +703,7 @@ mkosi-sandbox [OPTIONS...] COMMAND [ARGUMENTS...]
      --suppress-chown             Make chown() syscalls in the sandbox a noop
      --unshare-net                Unshare the network namespace if possible
      --unshare-ipc                Unshare the IPC namespace if possible
+     --exec-fd FD                 Close FD before execve()
 
 See the mkosi-sandbox(1) man page for details.\
 """
@@ -780,6 +790,8 @@ def main() -> None:
             unshare_net = True
         elif arg == "--unshare-ipc":
             unshare_ipc = True
+        elif arg == "--exec-fd":
+            fd_make_cloexec(int(argv.pop()))
         elif arg.startswith("-"):
             raise ValueError(f"Unrecognized option {arg}")
         else: