libstore: open builder log pty in parent

there's no need to do it in the fork, we're not changing security
domains before opening the pty anyway. we do change who owns this
pty device, but since we change it via chown we can just *not* do
that before we have an open fd to the pty. in practice this isn't
even necessary because the daemon runs as root, but if we ever do
run the daemon as an unprivileged user we'd need this to be split

Change-Id: I35264ab2954c7ba2c9c24c927366d64acada6772

lix/libstore/build/local-derivation-goal.cc

@@ -802,9 +802,7 @@ try {
             throw SysError("changing mode of pseudoterminal slave");
         }
 
-        if (sys::chown(slaveName, buildUser->getUID(), 0)) {
-            throw SysError("changing owner of pseudoterminal slave");
-        }
+        // don't chown yet so we can open the pty without DAC override capabilities
     }
 #if __APPLE__
     else {
@@ -816,13 +814,22 @@ try {
     if (unlockpt(builderOutPTY.get()))
         throw SysError("unlocking pseudoterminal");
 
-    /* Open the slave side of the pseudoterminal and use it as stderr. */
-    auto openSlave = [&]() {
-        AutoCloseFD builderOut{sys::open(slaveName, O_RDWR | O_NOCTTY)};
-        if (!builderOut)
-            throw SysError("opening pseudoterminal slave");
+    /* We need to open the slave early, before CLONE_NEWUSER. Otherwise we get
+       EPERM when running as root. */
+    AutoCloseFD builderOut{sys::open(slaveName, O_RDWR | O_NOCTTY | O_CLOEXEC)};
+    if (!builderOut) {
+        throw SysError("opening pseudoterminal slave");
+    }
 
-        // Put the pt into raw mode to prevent \n -> \r\n translation.
+    // *now* we chown the pty device node for sandbox processes
+    if (buildUser) {
+        if (sys::chown(slaveName, buildUser->getUID(), 0)) {
+            throw SysError("changing owner of 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");
@@ -831,15 +838,12 @@ try {
 
         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. */
-    pid = startChild(openSlave);
+    pid = startChild(std::move(builderOut));
 
     /* parent */
     pid.setSeparatePG(true);
@@ -875,14 +879,17 @@ try {
     co_return result::current_exception();
 }
 
-Pid LocalDerivationGoal::startChild(std::function<void()> openSlave) {
+Pid LocalDerivationGoal::startChild(AutoCloseFD logPTY)
+{
     return startProcess([&]() {
-        openSlave();
+        if (dup2(logPTY.get(), STDERR_FILENO) == -1) {
+            throw SysError("failed to redirect build output to log file");
+        }
+        closeOnExec(STDERR_FILENO, false);
         runChild();
     });
 }
 
-
 void LocalDerivationGoal::initTmpDir() {
     /* In a sandbox, for determinism, always use the same temporary
        directory. */

lix/libstore/build/local-derivation-goal.hh

@@ -316,7 +316,7 @@ protected:
      * Create a new process that runs `openSlave` and `runChild`
      * On some platforms this process is created with sandboxing flags.
      */
-    virtual Pid startChild(std::function<void()> openSlave);
+    virtual Pid startChild(AutoCloseFD logPTY);
 
     kj::Promise<Result<WorkResult>> handleRawChild() noexcept;
     kj::Promise<Result<std::optional<WorkResult>>> handleRawChildStream() noexcept;

lix/libstore/platform/linux.cc

@@ -898,7 +898,7 @@ std::string LinuxLocalDerivationGoal::rewriteResolvConf(std::string fromHost)
     return std::regex_replace(fromHost, lineRegex, "") + nsInSandbox;
 }
 
-Pid LinuxLocalDerivationGoal::startChild(std::function<void()> openSlave)
+Pid LinuxLocalDerivationGoal::startChild(AutoCloseFD logPTY)
 {
 #if HAVE_SECCOMP
     // Our seccomp filter program is surprisingly expensive to compile (~10ms).
@@ -909,7 +909,7 @@ Pid LinuxLocalDerivationGoal::startChild(std::function<void()> openSlave)
 
     // If we're not sandboxing no need to faff about, use the fallback
     if (!useChroot) {
-        return LocalDerivationGoal::startChild(openSlave);
+        return LocalDerivationGoal::startChild(std::move(logPTY));
     }
     /* Set up private namespaces for the build:
 
@@ -962,10 +962,9 @@ Pid LinuxLocalDerivationGoal::startChild(std::function<void()> openSlave)
     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();
+        if (dup2(logPTY.get(), STDERR_FILENO) == -1) {
+            throw SysError("failed to redirect build output to log file");
+        }
 
         /* Drop additional groups here because we can't do it
            after we've created the new user namespace. */

lix/libstore/platform/linux.hh

@@ -73,7 +73,7 @@ private:
      * Start child process in new namespaces,
      * create /etc/passwd and /etc/group based on discovered uid/gid
      */
-    Pid startChild(std::function<void()> openSlave) override;
+    Pid startChild(AutoCloseFD logPTY) override;
 
     /**
      * Kill all processes by build user.

lix/libutil/file-descriptor.cc

@@ -314,13 +314,14 @@ void closeExtraFDs()
         close(fd); /* ignore result */
 }
 
-
-void closeOnExec(int fd)
+void closeOnExec(int fd, bool doClose)
 {
     int prev;
-    if ((prev = fcntl(fd, F_GETFD, 0)) == -1 ||
-        fcntl(fd, F_SETFD, prev | FD_CLOEXEC) == -1)
+    if ((prev = fcntl(fd, F_GETFD, 0)) == -1
+        || fcntl(fd, F_SETFD, doClose ? prev | FD_CLOEXEC : prev & ~FD_CLOEXEC) == -1)
+    {
         throw SysError("setting close-on-exec flag");
+    }
 }
 
 FdBlockingState makeNonBlocking(int fd)

lix/libutil/file-descriptor.hh

@@ -108,9 +108,9 @@ private:
 void closeExtraFDs();
 
 /**
- * Set the close-on-exec flag for the given file descriptor.
+ * Set or clear the close-on-exec flag for the given file descriptor.
  */
-void closeOnExec(int fd);
+void closeOnExec(int fd, bool doClose = true);
 
 enum class FdBlockingState : int {};