diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index a32a714e3..e88f6bda7 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -649,7 +649,7 @@ Goal::Co DerivationGoal::tryToBuild() buildResult.startTime = time(0); // inexact started(); co_await Suspend{}; - co_return buildDone(); + co_return hookDone(); case rpPostpone: /* Not now; wait until at least one child finishes or the wake-up timeout expires. */ @@ -810,55 +810,6 @@ void replaceValidPath(const Path & storePath, const Path & tmpPath) } -int DerivationGoal::getChildStatus() -{ -#ifndef _WIN32 // TODO enable build hook on Windows - return hook->pid.kill(); -#else - return 0; -#endif -} - - -void DerivationGoal::closeReadPipes() -{ -#ifndef _WIN32 // TODO enable build hook on Windows - hook->builderOut.readSide.close(); - hook->fromHook.readSide.close(); -#endif -} - - -void DerivationGoal::cleanupHookFinally() -{ -} - - -void DerivationGoal::cleanupPreChildKill() -{ -} - - -void DerivationGoal::cleanupPostChildKill() -{ -} - - -bool DerivationGoal::cleanupDecideWhetherDiskFull() -{ - return false; -} - - -void DerivationGoal::cleanupPostOutputsRegisteredModeCheck() -{ -} - - -void DerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() -{ -} - void runPostBuildHook( Store & store, Logger & logger, @@ -917,21 +868,53 @@ void runPostBuildHook( }); } -Goal::Co DerivationGoal::buildDone() + +void appendLogTailErrorMsg( + Worker & worker, + const StorePath & drvPath, + const std::list & logTail, + std::string & msg) { - trace("build done"); + if (!logger->isVerbose() && !logTail.empty()) { + msg += fmt(";\nlast %d log lines:\n", logTail.size()); + for (auto & line : logTail) { + msg += "> "; + msg += line; + msg += "\n"; + } + auto nixLogCommand = experimentalFeatureSettings.isEnabled(Xp::NixCommand) + ? "nix log" + : "nix-store -l"; + // The command is on a separate line for easy copying, such as with triple click. + // This message will be indented elsewhere, so removing the indentation before the + // command will not put it at the start of the line unfortunately. + msg += fmt("For full logs, run:\n " ANSI_BOLD "%s %s" ANSI_NORMAL, + nixLogCommand, + worker.store.printStorePath(drvPath)); + } +} - Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); - cleanupPreChildKill(); +Goal::Co DerivationGoal::hookDone() +{ +#ifndef _WIN32 + assert(hook); +#endif + + trace("hook build done"); /* Since we got an EOF on the logger pipe, the builder is presumed to have terminated. In fact, the builder could also have simply have closed its end of the pipe, so just to be sure, kill it. */ - int status = getChildStatus(); + int status = +#ifndef _WIN32 // TODO enable build hook on Windows + hook->pid.kill(); +#else + 0; +#endif - debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); + debug("build hook for '%s' finished", worker.store.printStorePath(drvPath)); buildResult.timesBuilt++; buildResult.stopTime = time(0); @@ -940,108 +923,59 @@ Goal::Co DerivationGoal::buildDone() worker.childTerminated(this); /* Close the read side of the logger pipe. */ - closeReadPipes(); +#ifndef _WIN32 // TODO enable build hook on Windows + hook->builderOut.readSide.close(); + hook->fromHook.readSide.close(); +#endif /* Close the log file. */ closeLogFile(); - cleanupPostChildKill(); + /* Check the exit status. */ + if (!statusOk(status)) { + auto msg = fmt("builder for '%s' %s", + Magenta(worker.store.printStorePath(drvPath)), + statusToString(status)); - if (buildResult.cpuUser && buildResult.cpuSystem) { - debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", - worker.store.printStorePath(drvPath), - status, - ((double) buildResult.cpuUser->count()) / 1000000, - ((double) buildResult.cpuSystem->count()) / 1000000); - } + appendLogTailErrorMsg(worker, drvPath, logTail, msg); - bool diskFull = false; - - try { - - /* Check the exit status. */ - if (!statusOk(status)) { - - diskFull |= cleanupDecideWhetherDiskFull(); - - auto msg = fmt("builder for '%s' %s", - Magenta(worker.store.printStorePath(drvPath)), - statusToString(status)); - - if (!logger->isVerbose() && !logTail.empty()) { - msg += fmt(";\nlast %d log lines:\n", logTail.size()); - for (auto & line : logTail) { - msg += "> "; - msg += line; - msg += "\n"; - } - auto nixLogCommand = experimentalFeatureSettings.isEnabled(Xp::NixCommand) - ? "nix log" - : "nix-store -l"; - // The command is on a separate line for easy copying, such as with triple click. - // This message will be indented elsewhere, so removing the indentation before the - // command will not put it at the start of the line unfortunately. - msg += fmt("For full logs, run:\n " ANSI_BOLD "%s %s" ANSI_NORMAL, - nixLogCommand, - worker.store.printStorePath(drvPath)); - } - - if (diskFull) - msg += "\nnote: build failure may have been caused by lack of free disk space"; - - throw BuildError(msg); - } - - /* Compute the FS closure of the outputs and register them as - being valid. */ - auto builtOutputs = registerOutputs(); - - StorePathSet outputPaths; - for (auto & [_, output] : builtOutputs) - outputPaths.insert(output.outPath); - runPostBuildHook( - worker.store, - *logger, - drvPath, - outputPaths - ); - - cleanupPostOutputsRegisteredModeNonCheck(); - - /* It is now safe to delete the lock files, since all future - lockers will see that the output paths are valid; they will - not create new lock files with the same names as the old - (unlinked) lock files. */ - outputLocks.setDeletion(true); outputLocks.unlock(); - co_return done(BuildResult::Built, std::move(builtOutputs)); + /* TODO (once again) support fine-grained error codes, see issue #12641. */ - } catch (BuildError & e) { - outputLocks.unlock(); - - BuildResult::Status st = BuildResult::MiscFailure; - -#ifndef _WIN32 // TODO abstract over proc exit status - if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) - st = BuildResult::TimedOut; - - else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { - } - - else -#endif - { - assert(derivationType); - st = - dynamic_cast(&e) ? BuildResult::NotDeterministic : - statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : - BuildResult::PermanentFailure; - } - - co_return done(st, {}, std::move(e)); + co_return done(BuildResult::MiscFailure, {}, BuildError(msg)); } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = + /* When using a build hook, the build hook can register the output + as valid (by doing `nix-store --import'). If so we don't have + to do anything here. + + We can only early return when the outputs are known a priori. For + floating content-addressing derivations this isn't the case. + */ + assertPathValidity(); + + StorePathSet outputPaths; + for (auto & [_, output] : builtOutputs) + outputPaths.insert(output.outPath); + runPostBuildHook( + worker.store, + *logger, + drvPath, + outputPaths + ); + + /* It is now safe to delete the lock files, since all future + lockers will see that the output paths are valid; they will + not create new lock files with the same names as the old + (unlinked) lock files. */ + outputLocks.setDeletion(true); + outputLocks.unlock(); + + co_return done(BuildResult::Built, std::move(builtOutputs)); } Goal::Co DerivationGoal::resolvedFinished() @@ -1093,7 +1027,7 @@ Goal::Co DerivationGoal::resolvedFinished() : worker.store; newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore); } - signRealisation(newRealisation); + worker.store.signRealisation(newRealisation); worker.store.registerDrvOutput(newRealisation); } outputPaths.insert(realisation.outPath); @@ -1228,18 +1162,6 @@ HookReply DerivationGoal::tryBuildHook() } -SingleDrvOutputs DerivationGoal::registerOutputs() -{ - /* When using a build hook, the build hook can register the output - as valid (by doing `nix-store --import'). If so we don't have - to do anything here. - - We can only early return when the outputs are known a priori. For - floating content-addressing derivations this isn't the case. - */ - return assertPathValidity(); -} - Path DerivationGoal::openLogFile() { logSize = 0; diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 7f594b92c..25ab90183 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -55,6 +55,20 @@ struct InitialOutput { std::optional known; }; +/** Used internally */ +void runPostBuildHook( + Store & store, + Logger & logger, + const StorePath & drvPath, + const StorePathSet & outputPaths); + +/** Used internally */ +void appendLogTailErrorMsg( + Worker & worker, + const StorePath & drvPath, + const std::list & logTail, + std::string & msg); + /** * A goal for building some or all of the outputs of a derivation. */ @@ -232,7 +246,7 @@ struct DerivationGoal : public Goal Co gaveUpOnSubstitution(); Co tryToBuild(); virtual Co tryLocalBuild(); - Co buildDone(); + Co hookDone(); Co resolvedFinished(); @@ -241,44 +255,16 @@ struct DerivationGoal : public Goal */ HookReply tryBuildHook(); - virtual int getChildStatus(); - - /** - * Check that the derivation outputs all exist and register them - * as valid. - */ - virtual SingleDrvOutputs registerOutputs(); - /** * Open a log file and a pipe to it. */ Path openLogFile(); - /** - * Sign the newly built realisation if the store allows it - */ - virtual void signRealisation(Realisation&) {} - /** * Close the log file. */ void closeLogFile(); - /** - * Close the read side of the logger pipe. - */ - virtual void closeReadPipes(); - - /** - * Cleanup hooks for buildDone() - */ - virtual void cleanupHookFinally(); - virtual void cleanupPreChildKill(); - virtual void cleanupPostChildKill(); - virtual bool cleanupDecideWhetherDiskFull(); - virtual void cleanupPostOutputsRegisteredModeCheck(); - virtual void cleanupPostOutputsRegisteredModeNonCheck(); - virtual bool isReadDesc(Descriptor fd); /** diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index b54903432..2fd160dd4 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1585,19 +1585,6 @@ void LocalStore::addSignatures(const StorePath & storePath, const StringSet & si } -void LocalStore::signRealisation(Realisation & realisation) -{ - // FIXME: keep secret keys in memory. - - auto secretKeyFiles = settings.secretKeyFiles; - - for (auto & secretKeyFile : secretKeyFiles.get()) { - SecretKey secretKey(readFile(secretKeyFile)); - LocalSigner signer(std::move(secretKey)); - realisation.sign(signer); - } -} - void LocalStore::signPathInfo(ValidPathInfo & info) { // FIXME: keep secret keys in memory. diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 83154d651..07d9cec0a 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -401,7 +401,6 @@ private: * specified by the ‘secret-key-files’ option. */ void signPathInfo(ValidPathInfo & info); - void signRealisation(Realisation &); void addBuildLog(const StorePath & drvPath, std::string_view log) override; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index fc3fbcc0f..e99e96aac 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -1274,6 +1274,19 @@ Derivation Store::readDerivation(const StorePath & drvPath) Derivation Store::readInvalidDerivation(const StorePath & drvPath) { return readDerivationCommon(*this, drvPath, false); } +void Store::signRealisation(Realisation & realisation) +{ + // FIXME: keep secret keys in memory. + + auto secretKeyFiles = settings.secretKeyFiles; + + for (auto & secretKeyFile : secretKeyFiles.get()) { + SecretKey secretKey(readFile(secretKeyFile)); + LocalSigner signer(std::move(secretKey)); + realisation.sign(signer); + } +} + } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 2eba88ea0..83ca8344b 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -622,6 +622,8 @@ public: virtual void addSignatures(const StorePath & storePath, const StringSet & sigs) { unsupported("addSignatures"); } + void signRealisation(Realisation &); + /* Utility functions. */ /** diff --git a/src/libstore/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 61a36dd51..7f6bace86 100644 --- a/src/libstore/unix/build/local-derivation-goal.cc +++ b/src/libstore/unix/build/local-derivation-goal.cc @@ -184,6 +184,8 @@ void LocalDerivationGoal::killSandbox(bool getStats) Goal::Co LocalDerivationGoal::tryLocalBuild() { + assert(!hook); + unsigned int curBuilds = worker.getNrLocalBuilds(); if (curBuilds >= settings.maxBuildJobs) { worker.waitForBuildSlot(shared_from_this()); @@ -263,8 +265,122 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() started(); co_await Suspend{}; - // after EOF on child - co_return buildDone(); + + trace("build done"); + + Finally releaseBuildUser([&](){ + /* Release the build user at the end of this function. We don't do + it right away because we don't want another build grabbing this + uid and then messing around with our output. */ + buildUser.reset(); + }); + + sandboxMountNamespace = -1; + sandboxUserNamespace = -1; + + /* Since we got an EOF on the logger pipe, the builder is presumed + to have terminated. In fact, the builder could also have + simply have closed its end of the pipe, so just to be sure, + kill it. */ + int status = pid.kill(); + + debug("builder process for '%s' finished", worker.store.printStorePath(drvPath)); + + buildResult.timesBuilt++; + buildResult.stopTime = time(0); + + /* So the child is gone now. */ + worker.childTerminated(this); + + /* Close the read side of the logger pipe. */ + builderOut.close(); + + /* Close the log file. */ + closeLogFile(); + + /* When running under a build user, make sure that all processes + running under that uid are gone. This is to prevent a + malicious user from leaving behind a process that keeps files + open and modifies them after they have been chown'ed to + root. */ + killSandbox(true); + + /* Terminate the recursive Nix daemon. */ + stopDaemon(); + + if (buildResult.cpuUser && buildResult.cpuSystem) { + debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", + worker.store.printStorePath(drvPath), + status, + ((double) buildResult.cpuUser->count()) / 1000000, + ((double) buildResult.cpuSystem->count()) / 1000000); + } + + bool diskFull = false; + + try { + + /* Check the exit status. */ + if (!statusOk(status)) { + + diskFull |= cleanupDecideWhetherDiskFull(); + + auto msg = fmt("builder for '%s' %s", + Magenta(worker.store.printStorePath(drvPath)), + statusToString(status)); + + appendLogTailErrorMsg(worker, drvPath, logTail, msg); + + if (diskFull) + msg += "\nnote: build failure may have been caused by lack of free disk space"; + + throw BuildError(msg); + } + + /* Compute the FS closure of the outputs and register them as + being valid. */ + auto builtOutputs = registerOutputs(); + + StorePathSet outputPaths; + for (auto & [_, output] : builtOutputs) + outputPaths.insert(output.outPath); + runPostBuildHook( + worker.store, + *logger, + drvPath, + outputPaths + ); + + /* Delete unused redirected outputs (when doing hash rewriting). */ + for (auto & i : redirectedOutputs) + deletePath(worker.store.Store::toRealPath(i.second)); + + /* Delete the chroot (if we were using one). */ + autoDelChroot.reset(); /* this runs the destructor */ + + deleteTmpDir(true); + + /* It is now safe to delete the lock files, since all future + lockers will see that the output paths are valid; they will + not create new lock files with the same names as the old + (unlinked) lock files. */ + outputLocks.setDeletion(true); + outputLocks.unlock(); + + co_return done(BuildResult::Built, std::move(builtOutputs)); + + } catch (BuildError & e) { + outputLocks.unlock(); + + assert(derivationType); + BuildResult::Status st = + dynamic_cast(&e) ? BuildResult::NotDeterministic : + statusOk(status) ? BuildResult::OutputRejected : + !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + BuildResult::PermanentFailure; + + co_return done(st, {}, std::move(e)); + } } static void chmod_(const Path & path, mode_t mode) @@ -296,50 +412,6 @@ static void movePath(const Path & src, const Path & dst) extern void replaceValidPath(const Path & storePath, const Path & tmpPath); -int LocalDerivationGoal::getChildStatus() -{ - return hook ? DerivationGoal::getChildStatus() : pid.kill(); -} - -void LocalDerivationGoal::closeReadPipes() -{ - if (hook) { - DerivationGoal::closeReadPipes(); - } else - builderOut.close(); -} - - -void LocalDerivationGoal::cleanupHookFinally() -{ - /* Release the build user at the end of this function. We don't do - it right away because we don't want another build grabbing this - uid and then messing around with our output. */ - buildUser.reset(); -} - - -void LocalDerivationGoal::cleanupPreChildKill() -{ - sandboxMountNamespace = -1; - sandboxUserNamespace = -1; -} - - -void LocalDerivationGoal::cleanupPostChildKill() -{ - /* When running under a build user, make sure that all processes - running under that uid are gone. This is to prevent a - malicious user from leaving behind a process that keeps files - open and modifies them after they have been chown'ed to - root. */ - killSandbox(true); - - /* Terminate the recursive Nix daemon. */ - stopDaemon(); -} - - bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() { bool diskFull = false; @@ -380,24 +452,6 @@ bool LocalDerivationGoal::cleanupDecideWhetherDiskFull() } -void LocalDerivationGoal::cleanupPostOutputsRegisteredModeCheck() -{ - deleteTmpDir(true); -} - - -void LocalDerivationGoal::cleanupPostOutputsRegisteredModeNonCheck() -{ - /* Delete unused redirected outputs (when doing hash rewriting). */ - for (auto & i : redirectedOutputs) - deletePath(worker.store.Store::toRealPath(i.second)); - - /* Delete the chroot (if we were using one). */ - autoDelChroot.reset(); /* this runs the destructor */ - - cleanupPostOutputsRegisteredModeCheck(); -} - #if __linux__ static void doBind(const Path & source, const Path & target, bool optional = false) { debug("bind mounting '%1%' to '%2%'", source, target); @@ -727,7 +781,7 @@ void LocalDerivationGoal::startBuilder() environment using bind-mounts. We put it in the Nix store so that the build outputs can be moved efficiently from the chroot to their final location. */ - chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; + auto chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; deletePath(chrootParentDir); /* Clean up the chroot directory automatically. */ @@ -972,9 +1026,6 @@ void LocalDerivationGoal::startBuilder() us. */ - if (derivationType->isSandboxed()) - privateNetwork = true; - userNamespaceSync.create(); usingUserNamespace = userNamespacesSupported(); @@ -1002,7 +1053,7 @@ void LocalDerivationGoal::startBuilder() ProcessOptions options; options.cloneFlags = CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWIPC | CLONE_NEWUTS | CLONE_PARENT | SIGCHLD; - if (privateNetwork) + if (derivationType->isSandboxed()) options.cloneFlags |= CLONE_NEWNET; if (usingUserNamespace) options.cloneFlags |= CLONE_NEWUSER; @@ -1819,7 +1870,7 @@ void LocalDerivationGoal::runChild() userNamespaceSync.readSide = -1; - if (privateNetwork) { + if (derivationType->isSandboxed()) { /* Initialise the loopback interface. */ AutoCloseFD fd(socket(PF_INET, SOCK_DGRAM, IPPROTO_IP)); @@ -2279,15 +2330,7 @@ void LocalDerivationGoal::runChild() SingleDrvOutputs LocalDerivationGoal::registerOutputs() { - /* When using a build hook, the build hook can register the output - as valid (by doing `nix-store --import'). If so we don't have - to do anything here. - - We can only early return when the outputs are known a priori. For - floating content-addressing derivations this isn't the case. - */ - if (hook) - return DerivationGoal::registerOutputs(); + assert(!hook); std::map infos; @@ -2829,7 +2872,7 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() if (experimentalFeatureSettings.isEnabled(Xp::CaDerivations) && !drv->type().isImpure()) { - signRealisation(thisRealisation); + worker.store.signRealisation(thisRealisation); worker.store.registerDrvOutput(thisRealisation); } builtOutputs.emplace(outputName, thisRealisation); @@ -2838,11 +2881,6 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs() return builtOutputs; } -void LocalDerivationGoal::signRealisation(Realisation & realisation) -{ - getLocalStore().signRealisation(realisation); -} - void LocalDerivationGoal::checkOutputs(const std::map & outputs) { diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index c7a129f90..ec8cf2c0b 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -71,13 +71,6 @@ struct LocalDerivationGoal : public DerivationGoal */ bool useChroot = false; - /** - * The parent directory of `chrootRootDir`. It has permission 700 - * and is owned by root to ensure other users cannot mess with - * `chrootRootDir`. - */ - Path chrootParentDir; - /** * The root of the chroot environment. */ @@ -88,11 +81,6 @@ struct LocalDerivationGoal : public DerivationGoal */ std::shared_ptr autoDelChroot; - /** - * Whether to run the build in a private network namespace. - */ - bool privateNetwork = false; - /** * Stuff we need to pass to initChild(). */ @@ -242,8 +230,6 @@ struct LocalDerivationGoal : public DerivationGoal */ void chownToBuilder(const Path & path); - int getChildStatus() override; - /** * Run the builder's process. */ @@ -253,9 +239,7 @@ struct LocalDerivationGoal : public DerivationGoal * Check that the derivation outputs all exist and register them * as valid. */ - SingleDrvOutputs registerOutputs() override; - - void signRealisation(Realisation &) override; + SingleDrvOutputs registerOutputs(); /** * Check that an output meets the requirements specified by the @@ -264,21 +248,6 @@ struct LocalDerivationGoal : public DerivationGoal */ void checkOutputs(const std::map & outputs); - /** - * Close the read side of the logger pipe. - */ - void closeReadPipes() override; - - /** - * Cleanup hooks for buildDone() - */ - void cleanupHookFinally() override; - void cleanupPreChildKill() override; - void cleanupPostChildKill() override; - bool cleanupDecideWhetherDiskFull() override; - void cleanupPostOutputsRegisteredModeCheck() override; - void cleanupPostOutputsRegisteredModeNonCheck() override; - bool isReadDesc(int fd) override; /** @@ -299,6 +268,8 @@ struct LocalDerivationGoal : public DerivationGoal */ void killSandbox(bool getStats); + bool cleanupDecideWhetherDiskFull(); + /** * Create alternative path calculated from but distinct from the * input, so we can avoid overwriting outputs (or other store paths)