diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 6f69321b7..c5931f00e 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -811,45 +811,6 @@ int DerivationGoal::getChildStatus() } -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, @@ -937,11 +898,11 @@ void appendLogTailErrorMsg( Goal::Co DerivationGoal::hookDone() { - trace("build done"); +#ifndef _WIN32 + assert(hook); +#endif - Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); - - cleanupPreChildKill(); + 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 @@ -958,13 +919,14 @@ Goal::Co DerivationGoal::hookDone() 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(); - if (buildResult.cpuUser && buildResult.cpuSystem) { debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", worker.store.printStorePath(drvPath), @@ -973,24 +935,16 @@ Goal::Co DerivationGoal::hookDone() ((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); } @@ -1008,8 +962,6 @@ Goal::Co DerivationGoal::hookDone() 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 @@ -1024,23 +976,20 @@ Goal::Co DerivationGoal::hookDone() BuildResult::Status st = BuildResult::MiscFailure; -#ifndef _WIN32 // TODO abstract over proc exit status - if (hook && WIFEXITED(status) && WEXITSTATUS(status) == 101) +#ifndef _WIN32 + if (WIFEXITED(status) && WEXITSTATUS(status) == 101) st = BuildResult::TimedOut; - else if (hook && (!WIFEXITED(status) || WEXITSTATUS(status) != 100)) { - } - - else -#endif + else if (WIFEXITED(status) && WEXITSTATUS(status) == 100) { assert(derivationType); st = dynamic_cast(&e) ? BuildResult::NotDeterministic : statusOk(status) ? BuildResult::OutputRejected : - !derivationType->isSandboxed() || diskFull ? BuildResult::TransientFailure : + !derivationType->isSandboxed() ? BuildResult::TransientFailure : BuildResult::PermanentFailure; } +#endif co_return done(st, {}, std::move(e)); } diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index 52830bbfc..0fd9afa71 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -285,21 +285,6 @@ struct DerivationGoal : public Goal */ 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/unix/build/local-derivation-goal.cc b/src/libstore/unix/build/local-derivation-goal.cc index 80e339a53..18f6518e3 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()); @@ -266,9 +268,15 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() trace("build done"); - Finally releaseBuildUser([&](){ this->cleanupHookFinally(); }); + 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(); + }); - cleanupPreChildKill(); + 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 @@ -285,12 +293,20 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() worker.childTerminated(this); /* Close the read side of the logger pipe. */ - closeReadPipes(); + builderOut.close(); /* Close the log file. */ closeLogFile(); - 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(); if (buildResult.cpuUser && buildResult.cpuSystem) { debug("builder for '%s' terminated with status %d, user CPU %.3fs, system CPU %.3fs", @@ -335,7 +351,14 @@ Goal::Co LocalDerivationGoal::tryLocalBuild() outputPaths ); - 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 */ + + 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 @@ -407,44 +430,6 @@ 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() { @@ -486,24 +471,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); diff --git a/src/libstore/unix/build/local-derivation-goal.hh b/src/libstore/unix/build/local-derivation-goal.hh index c7a129f90..59b33d72a 100644 --- a/src/libstore/unix/build/local-derivation-goal.hh +++ b/src/libstore/unix/build/local-derivation-goal.hh @@ -264,21 +264,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 +284,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)