From 16db8dc96f64a0facbb620907e571f2dfc8e802e Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Mon, 20 Mar 2023 17:04:57 +0100
Subject: [PATCH] Open slave pseudoterminal before CLONE_NEWUSER

Otherwise, when running as root and user namespaces are enabled,
opening the slave fails with EPERM.

Fixes "opening pseudoterminal slave: Permission denied" followed by a
hang (https://hydra.nixos.org/build/213104244), and "error: getting
sandbox mount namespace: No such file or directory" (#8072), which
happens when the child fails very quickly and consequently reading
/proc/<child>/ns fails.
---
 src/libstore/build/local-derivation-goal.cc | 50 +++++++++++++--------
 src/libstore/build/local-derivation-goal.hh |  2 +-
 src/libutil/util.cc                         |  2 +-
 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index 521117c68..4f21862ca 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -827,6 +827,27 @@ void LocalDerivationGoal::startBuilder()
     if (unlockpt(builderOut.get()))
         throw SysError("unlocking pseudoterminal");
 
+    /* Open the slave side of the pseudoterminal and use it as stderr. */
+    auto openSlave = [&]()
+    {
+        AutoCloseFD builderOut = open(slaveName.c_str(), O_RDWR | O_NOCTTY);
+        if (!builderOut)
+            throw SysError("opening pseudoterminal slave");
+
+        // Put the pt into raw mode to prevent \n -> \r\n translation.
+        struct termios term;
+        if (tcgetattr(builderOut.get(), &term))
+            throw SysError("getting pseudoterminal attributes");
+
+        cfmakeraw(&term);
+
+        if (tcsetattr(builderOut.get(), TCSANOW, &term))
+            throw SysError("putting pseudoterminal into raw mode");
+
+        if (dup2(builderOut.get(), STDERR_FILENO) == -1)
+            throw SysError("cannot pipe standard error into log file");
+    };
+
     buildResult.startTime = time(0);
 
     /* Fork a child to build the package. */
@@ -880,6 +901,11 @@ void LocalDerivationGoal::startBuilder()
         Pid helper = startProcess([&]() {
             sendPid.readSide.close();
 
+            /* We need to open the slave early, before
+               CLONE_NEWUSER. Otherwise we get EPERM when running as
+               root. */
+            openSlave();
+
             /* Drop additional groups here because we can't do it
                after we've created the new user namespace.  FIXME:
                this means that if we're not root in the parent
@@ -898,7 +924,7 @@ void LocalDerivationGoal::startBuilder()
             if (usingUserNamespace)
                 options.cloneFlags |= CLONE_NEWUSER;
 
-            pid_t child = startProcess([&]() { runChild(slaveName); }, options);
+            pid_t child = startProcess([&]() { runChild(); }, options);
 
             writeFull(sendPid.writeSide.get(), fmt("%d\n", child));
             _exit(0);
@@ -974,7 +1000,8 @@ void LocalDerivationGoal::startBuilder()
 #endif
     {
         pid = startProcess([&]() {
-            runChild(slaveName);
+            openSlave();
+            runChild();
         });
     }
 
@@ -1620,7 +1647,7 @@ void setupSeccomp()
 }
 
 
-void LocalDerivationGoal::runChild(const Path & slaveName)
+void LocalDerivationGoal::runChild()
 {
     /* Warning: in the child we should absolutely not make any SQLite
        calls! */
@@ -1629,22 +1656,7 @@ void LocalDerivationGoal::runChild(const Path & slaveName)
 
     try { /* child */
 
-        /* Open the slave side of the pseudoterminal. */
-        AutoCloseFD builderOut = open(slaveName.c_str(), O_RDWR | O_NOCTTY);
-        if (!builderOut)
-            throw SysError("opening pseudoterminal slave");
-
-        // Put the pt into raw mode to prevent \n -> \r\n translation.
-        struct termios term;
-        if (tcgetattr(builderOut.get(), &term))
-            throw SysError("getting pseudoterminal attributes");
-
-        cfmakeraw(&term);
-
-        if (tcsetattr(builderOut.get(), TCSANOW, &term))
-            throw SysError("putting pseudoterminal into raw mode");
-
-        commonChildInit(builderOut.get());
+        commonChildInit(-1);
 
         try {
             setupSeccomp();
diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh
index 4d2f1ac28..c9ecc8828 100644
--- a/src/libstore/build/local-derivation-goal.hh
+++ b/src/libstore/build/local-derivation-goal.hh
@@ -169,7 +169,7 @@ struct LocalDerivationGoal : public DerivationGoal
     int getChildStatus() override;
 
     /* Run the builder's process. */
-    void runChild(const std::string & slaveName);
+    void runChild();
 
     /* Check that the derivation outputs all exist and register them
        as valid. */
diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index c605a33e6..a703c5650 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -1983,7 +1983,7 @@ void commonChildInit(int stderrFd)
         throw SysError("creating a new session");
 
     /* Dup the write side of the logger pipe into stderr. */
-    if (dup2(stderrFd, STDERR_FILENO) == -1)
+    if (stderrFd != -1 && dup2(stderrFd, STDERR_FILENO) == -1)
         throw SysError("cannot pipe standard error into log file");
 
     /* Dup stderr to stdout. */