From 21948deed99a3295e4d5666e027a6ca42dc00b40 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Thu, 19 Jan 2017 16:58:39 +0100 Subject: [PATCH] Kill builds when we get EOF on the log FD This closes a long-time bug that allowed builds to hang Nix indefinitely (regardless of timeouts) simply by doing exec > /dev/null 2>&1; while true; do true; done Now, on EOF, we just send SIGKILL to the child to make sure it's really gone. --- src/libmain/shared.cc | 2 +- src/libstore/build.cc | 21 ++++++++++----------- src/libutil/util.cc | 33 ++++++++++----------------------- src/libutil/util.hh | 11 ++++++----- tests/timeout.nix | 8 ++++++++ tests/timeout.sh | 5 +++++ 6 files changed, 40 insertions(+), 40 deletions(-) diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 44579a236..12f083c7f 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -333,7 +333,7 @@ RunPager::~RunPager() if (pid != -1) { std::cout.flush(); close(STDOUT_FILENO); - pid.wait(true); + pid.wait(); } } catch (...) { ignoreException(); diff --git a/src/libstore/build.cc b/src/libstore/build.cc index e8fb43896..7fc6ff0df 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -646,7 +646,7 @@ HookInstance::~HookInstance() { try { toHook.writeSide = -1; - pid.kill(true); + if (pid != -1) pid.kill(true); } catch (...) { ignoreException(); } @@ -960,7 +960,7 @@ void DerivationGoal::killChild() child. */ ::kill(-pid, SIGKILL); /* ignore the result */ buildUser.kill(); - pid.wait(true); + pid.wait(); } else pid.kill(); @@ -1416,11 +1416,9 @@ void DerivationGoal::buildDone() /* 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 --- just don't do that - :-) */ - /* !!! this could block! security problem! solution: kill the - child */ - int status = hook ? hook->pid.wait(true) : pid.wait(true); + simply have closed its end of the pipe, so just to be sure, + kill it. */ + int status = hook ? hook->pid.kill(true) : pid.kill(true); debug(format("builder process for ‘%1%’ finished") % drvPath); @@ -2112,6 +2110,8 @@ void DerivationGoal::startBuilder() result.startTime = time(0); /* Fork a child to build the package. */ + ProcessOptions options; + #if __linux__ if (useChroot) { /* Set up private namespaces for the build: @@ -2153,7 +2153,6 @@ void DerivationGoal::startBuilder() userNamespaceSync.create(); - ProcessOptions options; options.allowVfork = false; Pid helper = startProcess([&]() { @@ -2189,7 +2188,7 @@ void DerivationGoal::startBuilder() _exit(0); }, options); - if (helper.wait(true) != 0) + if (helper.wait() != 0) throw Error("unable to start build process"); userNamespaceSync.readSide = -1; @@ -2220,7 +2219,6 @@ void DerivationGoal::startBuilder() } else #endif { - ProcessOptions options; options.allowVfork = !buildUser.enabled() && !drv->isBuiltin(); pid = startProcess([&]() { runChild(); @@ -3702,7 +3700,8 @@ void Worker::waitForInput() auto after = steady_time_point::clock::now(); - /* Process all available file descriptors. */ + /* Process all available file descriptors. FIXME: this is + O(children * fds). */ decltype(children)::iterator i; for (auto j = children.begin(); j != children.end(); j = i) { i = std::next(j); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index d79cb5c13..e94575828 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -648,26 +648,25 @@ void Pipe::create() Pid::Pid() - : pid(-1), separatePG(false), killSignal(SIGKILL) { } Pid::Pid(pid_t pid) - : pid(pid), separatePG(false), killSignal(SIGKILL) + : pid(pid) { } Pid::~Pid() { - kill(); + if (pid != -1) kill(); } void Pid::operator =(pid_t pid) { - if (this->pid != pid) kill(); + if (this->pid != -1 && this->pid != pid) kill(); this->pid = pid; killSignal = SIGKILL; // reset signal to default } @@ -679,9 +678,9 @@ Pid::operator pid_t() } -void Pid::kill(bool quiet) +int Pid::kill(bool quiet) { - if (pid == -1 || pid == 0) return; + assert(pid != -1); if (!quiet) printError(format("killing process %1%") % pid); @@ -692,32 +691,20 @@ void Pid::kill(bool quiet) if (::kill(separatePG ? -pid : pid, killSignal) != 0) printError((SysError(format("killing process %1%") % pid).msg())); - /* Wait until the child dies, disregarding the exit status. */ - int status; - while (waitpid(pid, &status, 0) == -1) { - checkInterrupt(); - if (errno != EINTR) { - printError( - (SysError(format("waiting for process %1%") % pid).msg())); - break; - } - } - - pid = -1; + return wait(); } -int Pid::wait(bool block) +int Pid::wait() { assert(pid != -1); while (1) { int status; - int res = waitpid(pid, &status, block ? 0 : WNOHANG); + int res = waitpid(pid, &status, 0); if (res == pid) { pid = -1; return status; } - if (res == 0 && !block) return -1; if (errno != EINTR) throw SysError("cannot get child exit status"); checkInterrupt(); @@ -782,7 +769,7 @@ void killUser(uid_t uid) _exit(0); }, options); - int status = pid.wait(true); + int status = pid.wait(); if (status != 0) throw Error(format("cannot kill processes for uid ‘%1%’: %2%") % uid % statusToString(status)); @@ -893,7 +880,7 @@ string runProgram(Path program, bool searchPath, const Strings & args, string result = drainFD(out.readSide.get()); /* Wait for the child to finish. */ - int status = pid.wait(true); + int status = pid.wait(); if (!statusOk(status)) throw ExecError(status, format("program ‘%1%’ %2%") % program % statusToString(status)); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 67d289168..d00f9c645 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -192,17 +192,18 @@ typedef std::unique_ptr AutoCloseDir; class Pid { - pid_t pid; - bool separatePG; - int killSignal; + pid_t pid = -1; + bool separatePG = false; + int killSignal = SIGKILL; public: Pid(); Pid(pid_t pid); ~Pid(); void operator =(pid_t pid); operator pid_t(); - void kill(bool quiet = false); - int wait(bool block); + int kill(bool quiet = false); + int wait(); + void setSeparatePG(bool separatePG); void setKillSignal(int signal); pid_t release(); diff --git a/tests/timeout.nix b/tests/timeout.nix index 1f3f23f16..540fba934 100644 --- a/tests/timeout.nix +++ b/tests/timeout.nix @@ -17,4 +17,12 @@ with import ./config.nix; ''; }; + closeLog = mkDerivation { + name = "silent"; + buildCommand = '' + exec > /dev/null 2>&1 + sleep 1000000000 + ''; + }; + } diff --git a/tests/timeout.sh b/tests/timeout.sh index ba9a6c3df..ce1ae7d67 100644 --- a/tests/timeout.sh +++ b/tests/timeout.sh @@ -24,3 +24,8 @@ if nix-build timeout.nix -A silent --max-silent-time 2; then echo "build should have failed" exit 1 fi + +if nix-build timeout.nix -A closeLog; then + echo "build should have failed" + exit 1 +fi