diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index c790de9fe..31505eb6e 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/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 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. */ diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index a88eab8bf..5cbe96c48 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/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 openSlave); + virtual Pid startChild(AutoCloseFD logPTY); kj::Promise> handleRawChild() noexcept; kj::Promise>> handleRawChildStream() noexcept; diff --git a/lix/libstore/platform/linux.cc b/lix/libstore/platform/linux.cc index e39c2e416..cfb75fb66 100644 --- a/lix/libstore/platform/linux.cc +++ b/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 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 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 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. */ diff --git a/lix/libstore/platform/linux.hh b/lix/libstore/platform/linux.hh index e9e19f237..5908f82f5 100644 --- a/lix/libstore/platform/linux.hh +++ b/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 openSlave) override; + Pid startChild(AutoCloseFD logPTY) override; /** * Kill all processes by build user. diff --git a/lix/libutil/file-descriptor.cc b/lix/libutil/file-descriptor.cc index 33f261aa3..5d1d6f1e0 100644 --- a/lix/libutil/file-descriptor.cc +++ b/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) diff --git a/lix/libutil/file-descriptor.hh b/lix/libutil/file-descriptor.hh index e72b93026..15e2bfc77 100644 --- a/lix/libutil/file-descriptor.hh +++ b/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 {};