Merge pull request #8077 from edolstra/pts-hang

Open slave pseudoterminal before CLONE_NEWUSER
NixOS · Mar 20, 2023 · 1de5b0e · 1de5b0e
2 parents 83b977f + 515662a
commit 1de5b0e
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 27 deletions.
diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc
@@ -35,7 +35,10 @@ HookInstance::HookInstance()
     /* Fork the hook. */
     pid = startProcess([&]() {
 
-        commonChildInit(fromHook.writeSide.get());
+        if (dup2(fromHook.writeSide.get(), STDERR_FILENO) == -1)
+            throw SysError("cannot pipe standard error into log file");
+
+        commonChildInit();
 
         if (chdir("/") == -1) throw SysError("changing into /");
 

diff --git 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();
 
         try {
             setupSeccomp();

diff --git 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
@@ -1968,7 +1968,7 @@ std::string showBytes(uint64_t bytes)
 
 
 // FIXME: move to libstore/build
-void commonChildInit(int stderrFd)
+void commonChildInit()
 {
     logger = makeSimpleLogger();
 
@@ -1982,10 +1982,6 @@ void commonChildInit(int stderrFd)
     if (setsid() == -1)
         throw SysError("creating a new session");
 
-    /* Dup the write side of the logger pipe into stderr. */
-    if (dup2(stderrFd, STDERR_FILENO) == -1)
-        throw SysError("cannot pipe standard error into log file");
-
     /* Dup stderr to stdout. */
     if (dup2(STDERR_FILENO, STDOUT_FILENO) == -1)
         throw SysError("cannot dup stderr into stdout");

diff --git a/src/libutil/util.hh b/src/libutil/util.hh
@@ -704,7 +704,7 @@ typedef std::function<bool(const Path & path)> PathFilter;
 extern PathFilter defaultPathFilter;
 
 /* Common initialisation performed in child processes. */
-void commonChildInit(int stderrFd);
+void commonChildInit();
 
 /* Create a Unix domain socket. */
 AutoCloseFD createUnixDomainSocket();