From 99d674b78506af08c0a1c2ff8028dfac688faa7d Mon Sep 17 00:00:00 2001 From: eldritch horrors Date: Sun, 25 Jan 2026 15:34:59 +0100 Subject: [PATCH] libstore: despecialize sandbox launching now that builtin builders are regular executables we no longer need to treat them specially during sandbox launch itself, only while we build the command line and environment for the sandboxed process. we are not far from being able to extract platform-dependent sandbox launch code, ideally moving all of it into (much more replaceable) libexec helpers. Change-Id: I9b7041314683c56cd70eec9b1b4eae6de228883f --- lix/libstore/build/local-derivation-goal.cc | 128 +++++++++----------- lix/libstore/build/local-derivation-goal.hh | 16 +-- lix/libstore/platform/linux.cc | 10 +- lix/libstore/platform/linux.hh | 6 +- 4 files changed, 66 insertions(+), 94 deletions(-) diff --git a/lix/libstore/build/local-derivation-goal.cc b/lix/libstore/build/local-derivation-goal.cc index 74d31c6a2..c2927ac1b 100644 --- a/lix/libstore/build/local-derivation-goal.cc +++ b/lix/libstore/build/local-derivation-goal.cc @@ -868,23 +868,68 @@ try { setupConfiguredCertificateAuthority(); } - /* Fill in the environment. */ + Path builder; Strings envStrs; - for (auto & i : env) { - envStrs.push_back(rewriteStrings(i.first + "=" + i.second, inputRewrites)); - } - - /* Fill in the arguments. */ Strings args; - args.push_back(std::string(baseNameOf(drv->builder))); + if (drv->isBuiltin()) { + args.push_back("builtin-builder"); - for (auto & i : drv->args) { - args.push_back(rewriteStrings(i, inputRewrites)); + std::map overriddenSettings; + settings.getSettings(overriddenSettings, true); + + if (!netrcData.empty()) { + overriddenSettings[settings.netrcFile.name].value = tmpDirInSandbox + "/netrc"; + auto path = tmpDir + "/netrc"; + writeFile(path, netrcData, 0600); + chownToBuilder(path); + } + if (!caFileData.empty()) { + overriddenSettings[settings.caFile.name].value = tmpDirInSandbox + "/cafile"; + auto path = tmpDir + "/cafile"; + writeFile(path, caFileData, 0600); + chownToBuilder(path); + } + + for (const auto & [setting, value] : overriddenSettings) { + args.push_back("--" + setting); + args.push_back(escapeNul(value.value)); + } + + args.push_back("--"); + + for (const auto & [k, v] : drv->env) { + args.push_back("--" + escapeNul(k)); + args.push_back(rewriteStrings(escapeNul(v), inputRewrites)); + } + + // we pass on the *entire* daemon env to the builtin builder for historical + // reasons (libcurl inspects some env vars to modify how it behaves, and we + // are not fully sure yet that we pass through all of them to sandboxes. we + // should change this eventually. TODO: investigate curl env var use first) + for (auto & [k, v] : getEnv()) { + envStrs.emplace_back(k + "=" + v); + } + + builder = LIX_LIBEXEC_DIR "/builtin-builder"; + } else { + /* Fill in the environment. */ + for (auto & i : env) { + envStrs.push_back(rewriteStrings(i.first + "=" + i.second, inputRewrites)); + } + + /* Fill in the arguments. */ + args.push_back(std::string(baseNameOf(drv->builder))); + + for (auto & i : drv->args) { + args.push_back(rewriteStrings(i, inputRewrites)); + } + + builder = drv->builder; } /* Fork a child to build the package. */ - pg = ProcessGroup{startChild(netrcData, caFileData, envStrs, args, std::move(builderOut))}; + pg = ProcessGroup{startChild(builder, envStrs, args, std::move(builderOut))}; /* Check if setting up the build environment failed. */ std::vector msgs; @@ -918,11 +963,7 @@ try { } Pid LocalDerivationGoal::startChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args, - AutoCloseFD logPTY + const Path & builder, const Strings & envStrs, const Strings & args, AutoCloseFD logPTY ) { return startProcess([&]() { @@ -930,7 +971,7 @@ Pid LocalDerivationGoal::startChild( throw SysError("failed to redirect build output to log file"); } closeOnExec(STDERR_FILENO, false); - runChild(netrcData, caFileData, envStrs, args); + runChild(builder, envStrs, args); }); } @@ -1165,12 +1206,7 @@ void LocalDerivationGoal::chownToBuilder(const AutoCloseFD & fd) throw SysError("cannot change ownership of file '%1%'", fd.guessOrInventPath()); } -void LocalDerivationGoal::runChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args -) +void LocalDerivationGoal::runChild(const Path & builder, const Strings & envStrs, const Strings & args) { /* Warning: in the child we should absolutely not make any SQLite calls! */ @@ -1231,53 +1267,7 @@ void LocalDerivationGoal::runChild( sendException = false; /* Execute the program. This should not return. */ - if (drv->isBuiltin()) { - Strings args{ - "builtin-builder", - }; - - std::map overriddenSettings; - settings.getSettings(overriddenSettings, true); - - if (!netrcData.empty()) { - auto path = tmpDirInSandbox + "/netrc"; - overriddenSettings[settings.netrcFile.name].value = path; - writeFile(path, netrcData, 0600); - } - if (!caFileData.empty()) { - auto path = tmpDirInSandbox + "/cafile"; - overriddenSettings[settings.caFile.name].value = path; - writeFile(path, caFileData, 0600); - } - - for (const auto & [setting, value] : overriddenSettings) { - args.push_back("--" + setting); - args.push_back(escapeNul(value.value)); - } - - args.push_back("--"); - - for (const auto & [k, v] : drv->env) { - args.push_back("--" + escapeNul(k)); - args.push_back(rewriteStrings(escapeNul(v), inputRewrites)); - } - - // we pass on the *entire* daemon env to the builtin builder for historical - // reasons (libcurl inspects some env vars to modify how it behaves, and we - // are not fully sure yet that we pass through all of them to sandboxes. we - // should change this eventually. TODO: investigate curl env var use first) - Strings envs; - for (auto & [k, v] : getEnv()) { - envs.emplace_back(k + "=" + v); - } - - execBuilder(LIX_LIBEXEC_DIR "/builtin-builder", std::move(args), envs); - - throw Error("builtin builder '%1%' failed to exec", drv->builder.substr(8)); - } - - execBuilder(drv->builder, args, envStrs); - // execBuilder should not return + execBuilder(builder, args, envStrs); throw SysError("executing '%1%'", drv->builder); diff --git a/lix/libstore/build/local-derivation-goal.hh b/lix/libstore/build/local-derivation-goal.hh index 1d34546a0..cc810a441 100644 --- a/lix/libstore/build/local-derivation-goal.hh +++ b/lix/libstore/build/local-derivation-goal.hh @@ -214,12 +214,7 @@ struct LocalDerivationGoal : public DerivationGoal /** * Run the builder's process. */ - void runChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args - ); + void runChild(const Path & builder, const Strings & envStrs, const Strings & args); /** * Check that the derivation outputs all exist and register them @@ -316,13 +311,8 @@ protected: * Create a new process that runs `openSlave` and `runChild` * On some platforms this process is created with sandboxing flags. */ - virtual Pid startChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args, - AutoCloseFD logPTY - ); + virtual Pid + startChild(const Path & builder, const Strings & envStrs, const Strings & args, 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 f69879698..3b11822fc 100644 --- a/lix/libstore/platform/linux.cc +++ b/lix/libstore/platform/linux.cc @@ -1239,11 +1239,7 @@ bool LinuxLocalDerivationGoal::prepareChildSetup() } Pid LinuxLocalDerivationGoal::startChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args, - AutoCloseFD logPTY + const Path & builder, const Strings & envStrs, const Strings & args, AutoCloseFD logPTY ) { #if HAVE_SECCOMP @@ -1255,7 +1251,7 @@ Pid LinuxLocalDerivationGoal::startChild( // If we're not sandboxing no need to faff about, use the fallback if (!useChroot) { - return LocalDerivationGoal::startChild(netrcData, caFileData, envStrs, args, std::move(logPTY)); + return LocalDerivationGoal::startChild(builder, envStrs, args, std::move(logPTY)); } /* Set up private namespaces for the build: @@ -1337,7 +1333,7 @@ Pid LinuxLocalDerivationGoal::startChild( pid_t child = startProcess([&]() { if (prctl(PR_SET_PDEATHSIG, SIGKILL) == -1) throw SysError("setting death signal"); - runChild(netrcData, caFileData, envStrs, args); + runChild(builder, envStrs, args); }, options).release(); writeFull(sendPid.writeSide.get(), fmt("%d\n", child)); diff --git a/lix/libstore/platform/linux.hh b/lix/libstore/platform/linux.hh index dba85bca8..dd619df42 100644 --- a/lix/libstore/platform/linux.hh +++ b/lix/libstore/platform/linux.hh @@ -74,11 +74,7 @@ private: * create /etc/passwd and /etc/group based on discovered uid/gid */ Pid startChild( - const std::string & netrcData, - const std::string & caFileData, - const Strings & envStrs, - const Strings & args, - AutoCloseFD logPTY + const Path & builder, const Strings & envStrs, const Strings & args, AutoCloseFD logPTY ) override; /**