Propagate errors from early sandbox initialization to the parent

This should help with issues like DeterminateSystems/nix-installer#1227, which currently just print "unable to start build process".
DeterminateSystems · Oct 9, 2024 · 0be7046 · 0be7046
1 parent 26c3fc1
commit 0be7046
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 33 deletions.
diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc
@@ -433,6 +433,41 @@ static void doBind(const Path & source, const Path & target, bool optional = fal
 };
 #endif
 
+/**
+ * Rethrow the current exception as a subclass of `Error`.
+ */
+static void rethrowExceptionAsError()
+{
+    try {
+        throw;
+    } catch (Error &) {
+        throw;
+    } catch (std::exception & e) {
+        throw Error(e.what());
+    } catch (...) {
+        throw Error("unknown exception");
+    }
+}
+
+/**
+ * Send the current exception to the parent in the format expected by
+ * `LocalDerivationGoal::processSandboxSetupMessages()`.
+ */
+static void handleChildException(bool sendException)
+{
+    try {
+        rethrowExceptionAsError();
+    } catch (Error & e) {
+        if (sendException) {
+            writeFull(STDERR_FILENO, "\1\n");
+            FdSink sink(STDERR_FILENO);
+            sink << e;
+            sink.flush();
+        } else
+            std::cerr << e.msg();
+    }
+}
+
 void LocalDerivationGoal::startBuilder()
 {
     if ((buildUser && buildUser->getUIDCount() != 1)
@@ -949,32 +984,40 @@ void LocalDerivationGoal::startBuilder()
                root. */
             openSlave();
 
-            /* Drop additional groups here because we can't do it
-               after we've created the new user namespace. */
-            if (setgroups(0, 0) == -1) {
-                if (errno != EPERM)
-                    throw SysError("setgroups failed");
-                if (settings.requireDropSupplementaryGroups)
-                    throw Error("setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step.");
-            }
+            try {
+                /* Drop additional groups here because we can't do it
+                   after we've created the new user namespace. */
+                if (setgroups(0, 0) == -1) {
+                    if (errno != EPERM)
+                        throw SysError("setgroups failed");
+                    if (settings.requireDropSupplementaryGroups)
+                        throw Error("setgroups failed. Set the require-drop-supplementary-groups option to false to skip this step.");
+                }
 
-            ProcessOptions options;
-            options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD;
-            if (privateNetwork)
-                options.cloneFlags |= CLONE_NEWNET;
-            if (usingUserNamespace)
-                options.cloneFlags |= CLONE_NEWUSER;
+                ProcessOptions options;
+                options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD;
+                if (privateNetwork)
+                    options.cloneFlags |= CLONE_NEWNET;
+                if (usingUserNamespace)
+                    options.cloneFlags |= CLONE_NEWUSER;
 
-            pid_t child = startProcess([&]() { runChild(); }, options);
+                pid_t child = startProcess([&]() { runChild(); }, options);
 
-            writeFull(sendPid.writeSide.get(), fmt("%d\n", child));
-            _exit(0);
+                writeFull(sendPid.writeSide.get(), fmt("%d\n", child));
+                _exit(0);
+            } catch (...) {
+                handleChildException(true);
+                _exit(1);
+            }
         });
 
         sendPid.writeSide.close();
 
-        if (helper.wait() != 0)
+        if (helper.wait() != 0) {
+            processSandboxSetupMessages();
+            // Only reached if the child process didn't send an exception.
             throw Error("unable to start build process");
+        }
 
         userNamespaceSync.readSide = -1;
 
@@ -1050,7 +1093,12 @@ void LocalDerivationGoal::startBuilder()
     pid.setSeparatePG(true);
     worker.childStarted(shared_from_this(), {builderOut.get()}, true, true);
 
-    /* Check if setting up the build environment failed. */
+    processSandboxSetupMessages();
+}
+
+
+void LocalDerivationGoal::processSandboxSetupMessages()
+{
     std::vector<std::string> msgs;
     while (true) {
         std::string msg = [&]() {
@@ -1078,7 +1126,8 @@ void LocalDerivationGoal::startBuilder()
 }
 
 
-void LocalDerivationGoal::initTmpDir() {
+void LocalDerivationGoal::initTmpDir()
+{
     /* In a sandbox, for determinism, always use the same temporary
        directory. */
 #if __linux__
@@ -2237,14 +2286,8 @@ void LocalDerivationGoal::runChild()
 
         throw SysError("executing '%1%'", drv->builder);
 
-    } catch (Error & e) {
-        if (sendException) {
-            writeFull(STDERR_FILENO, "\1\n");
-            FdSink sink(STDERR_FILENO);
-            sink << e;
-            sink.flush();
-        } else
-            std::cerr << e.msg();
+    } catch (...) {
+        handleChildException(sendException);
         _exit(1);
     }
 }

diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh
@@ -210,6 +210,11 @@ struct LocalDerivationGoal : public DerivationGoal
      */
     void initEnv();
 
+    /**
+     * Process messages send by the sandbox initialization.
+     */
+    void processSandboxSetupMessages();
+
     /**
      * Setup tmp dir location.
      */

diff --git a/tests/functional/supplementary-groups.sh b/tests/functional/supplementary-groups.sh
@@ -9,7 +9,7 @@ needLocalStore "The test uses --store always so we would just be bypassing the d
 
 TODO_NixOS
 
-unshare --mount --map-root-user bash <<EOF
+unshare --mount --map-root-user -- bash -e -x <<EOF
   source common.sh
 
   # Avoid store dir being inside sandbox build-dir
@@ -24,15 +24,13 @@ unshare --mount --map-root-user bash <<EOF
   cmd=(nix-build ./hermetic.nix --arg busybox "$busybox" --arg seed 1 --no-out-link)
 
   # Fails with default setting
-  # TODO better error
   setLocalStore store1
-  expectStderr 1 "\${cmd[@]}" | grepQuiet "unable to start build process"
+  expectStderr 1 "\${cmd[@]}" | grepQuiet "setgroups failed"
 
   # Fails with `require-drop-supplementary-groups`
-  # TODO better error
   setLocalStore store2
   NIX_CONFIG='require-drop-supplementary-groups = true' \
-    expectStderr 1 "\${cmd[@]}" | grepQuiet "unable to start build process"
+    expectStderr 1 "\${cmd[@]}" | grepQuiet "setgroups failed"
 
   # Works without `require-drop-supplementary-groups`
   setLocalStore store3