From 3937eb9ecc62045d9a14e0566d026fc1e4ea2fca Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Wed, 21 Jan 2026 15:59:30 +0100 Subject: [PATCH] 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 | 41 ++++++++++++--------- lix/libstore/build/local-derivation-goal.hh | 2 +- lix/libstore/platform/linux.cc | 11 +++--- lix/libstore/platform/linux.hh | 2 +- lix/libutil/file-descriptor.cc | 9 +++-- lix/libutil/file-descriptor.hh | 4 +- 6 files changed, 38 insertions(+), 31 deletions(-) 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 {};