Permalink
Browse files

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.
  • Loading branch information...
edolstra committed Jan 19, 2017
1 parent 63e10b4 commit 21948deed99a3295e4d5666e027a6ca42dc00b40
Showing with 40 additions and 40 deletions.
  1. +1 −1 src/libmain/shared.cc
  2. +10 −11 src/libstore/build.cc
  3. +10 −23 src/libutil/util.cc
  4. +6 −5 src/libutil/util.hh
  5. +8 −0 tests/timeout.nix
  6. +5 −0 tests/timeout.sh
@@ -333,7 +333,7 @@ RunPager::~RunPager()
if (pid != -1) {
std::cout.flush();
close(STDOUT_FILENO);
pid.wait(true);
pid.wait();
}
} catch (...) {
ignoreException();
@@ -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);
@@ -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));
@@ -192,17 +192,18 @@ typedef std::unique_ptr<DIR, DIRDeleter> 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();
@@ -17,4 +17,12 @@ with import ./config.nix;
'';
};
closeLog = mkDerivation {
name = "silent";
buildCommand = ''
exec > /dev/null 2>&1
sleep 1000000000
'';
};
}
@@ -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

0 comments on commit 21948de

Please sign in to comment.