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.
This commit is contained in:
Eelco Dolstra 2017-01-19 16:58:39 +01:00
parent 63e10b4d28
commit 21948deed9
No known key found for this signature in database
GPG key ID: 8170B4726D7198DE
6 changed files with 40 additions and 40 deletions

View file

@ -333,7 +333,7 @@ RunPager::~RunPager()
if (pid != -1) { if (pid != -1) {
std::cout.flush(); std::cout.flush();
close(STDOUT_FILENO); close(STDOUT_FILENO);
pid.wait(true); pid.wait();
} }
} catch (...) { } catch (...) {
ignoreException(); ignoreException();

View file

@ -646,7 +646,7 @@ HookInstance::~HookInstance()
{ {
try { try {
toHook.writeSide = -1; toHook.writeSide = -1;
pid.kill(true); if (pid != -1) pid.kill(true);
} catch (...) { } catch (...) {
ignoreException(); ignoreException();
} }
@ -960,7 +960,7 @@ void DerivationGoal::killChild()
child. */ child. */
::kill(-pid, SIGKILL); /* ignore the result */ ::kill(-pid, SIGKILL); /* ignore the result */
buildUser.kill(); buildUser.kill();
pid.wait(true); pid.wait();
} else } else
pid.kill(); pid.kill();
@ -1416,11 +1416,9 @@ void DerivationGoal::buildDone()
/* Since we got an EOF on the logger pipe, the builder is presumed /* Since we got an EOF on the logger pipe, the builder is presumed
to have terminated. In fact, the builder could also have to have terminated. In fact, the builder could also have
simply have closed its end of the pipe --- just don't do that simply have closed its end of the pipe, so just to be sure,
:-) */ kill it. */
/* !!! this could block! security problem! solution: kill the int status = hook ? hook->pid.kill(true) : pid.kill(true);
child */
int status = hook ? hook->pid.wait(true) : pid.wait(true);
debug(format("builder process for %1% finished") % drvPath); debug(format("builder process for %1% finished") % drvPath);
@ -2112,6 +2110,8 @@ void DerivationGoal::startBuilder()
result.startTime = time(0); result.startTime = time(0);
/* Fork a child to build the package. */ /* Fork a child to build the package. */
ProcessOptions options;
#if __linux__ #if __linux__
if (useChroot) { if (useChroot) {
/* Set up private namespaces for the build: /* Set up private namespaces for the build:
@ -2153,7 +2153,6 @@ void DerivationGoal::startBuilder()
userNamespaceSync.create(); userNamespaceSync.create();
ProcessOptions options;
options.allowVfork = false; options.allowVfork = false;
Pid helper = startProcess([&]() { Pid helper = startProcess([&]() {
@ -2189,7 +2188,7 @@ void DerivationGoal::startBuilder()
_exit(0); _exit(0);
}, options); }, options);
if (helper.wait(true) != 0) if (helper.wait() != 0)
throw Error("unable to start build process"); throw Error("unable to start build process");
userNamespaceSync.readSide = -1; userNamespaceSync.readSide = -1;
@ -2220,7 +2219,6 @@ void DerivationGoal::startBuilder()
} else } else
#endif #endif
{ {
ProcessOptions options;
options.allowVfork = !buildUser.enabled() && !drv->isBuiltin(); options.allowVfork = !buildUser.enabled() && !drv->isBuiltin();
pid = startProcess([&]() { pid = startProcess([&]() {
runChild(); runChild();
@ -3702,7 +3700,8 @@ void Worker::waitForInput()
auto after = steady_time_point::clock::now(); 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; decltype(children)::iterator i;
for (auto j = children.begin(); j != children.end(); j = i) { for (auto j = children.begin(); j != children.end(); j = i) {
i = std::next(j); i = std::next(j);

View file

@ -648,26 +648,25 @@ void Pipe::create()
Pid::Pid() Pid::Pid()
: pid(-1), separatePG(false), killSignal(SIGKILL)
{ {
} }
Pid::Pid(pid_t pid) Pid::Pid(pid_t pid)
: pid(pid), separatePG(false), killSignal(SIGKILL) : pid(pid)
{ {
} }
Pid::~Pid() Pid::~Pid()
{ {
kill(); if (pid != -1) kill();
} }
void Pid::operator =(pid_t pid) void Pid::operator =(pid_t pid)
{ {
if (this->pid != pid) kill(); if (this->pid != -1 && this->pid != pid) kill();
this->pid = pid; this->pid = pid;
killSignal = SIGKILL; // reset signal to default 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) if (!quiet)
printError(format("killing process %1%") % pid); printError(format("killing process %1%") % pid);
@ -692,32 +691,20 @@ void Pid::kill(bool quiet)
if (::kill(separatePG ? -pid : pid, killSignal) != 0) if (::kill(separatePG ? -pid : pid, killSignal) != 0)
printError((SysError(format("killing process %1%") % pid).msg())); printError((SysError(format("killing process %1%") % pid).msg()));
/* Wait until the child dies, disregarding the exit status. */ return wait();
int status;
while (waitpid(pid, &status, 0) == -1) {
checkInterrupt();
if (errno != EINTR) {
printError(
(SysError(format("waiting for process %1%") % pid).msg()));
break;
}
}
pid = -1;
} }
int Pid::wait(bool block) int Pid::wait()
{ {
assert(pid != -1); assert(pid != -1);
while (1) { while (1) {
int status; int status;
int res = waitpid(pid, &status, block ? 0 : WNOHANG); int res = waitpid(pid, &status, 0);
if (res == pid) { if (res == pid) {
pid = -1; pid = -1;
return status; return status;
} }
if (res == 0 && !block) return -1;
if (errno != EINTR) if (errno != EINTR)
throw SysError("cannot get child exit status"); throw SysError("cannot get child exit status");
checkInterrupt(); checkInterrupt();
@ -782,7 +769,7 @@ void killUser(uid_t uid)
_exit(0); _exit(0);
}, options); }, options);
int status = pid.wait(true); int status = pid.wait();
if (status != 0) if (status != 0)
throw Error(format("cannot kill processes for uid %1%: %2%") % uid % statusToString(status)); 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()); string result = drainFD(out.readSide.get());
/* Wait for the child to finish. */ /* Wait for the child to finish. */
int status = pid.wait(true); int status = pid.wait();
if (!statusOk(status)) if (!statusOk(status))
throw ExecError(status, format("program %1% %2%") throw ExecError(status, format("program %1% %2%")
% program % statusToString(status)); % program % statusToString(status));

View file

@ -192,17 +192,18 @@ typedef std::unique_ptr<DIR, DIRDeleter> AutoCloseDir;
class Pid class Pid
{ {
pid_t pid; pid_t pid = -1;
bool separatePG; bool separatePG = false;
int killSignal; int killSignal = SIGKILL;
public: public:
Pid(); Pid();
Pid(pid_t pid); Pid(pid_t pid);
~Pid(); ~Pid();
void operator =(pid_t pid); void operator =(pid_t pid);
operator pid_t(); operator pid_t();
void kill(bool quiet = false); int kill(bool quiet = false);
int wait(bool block); int wait();
void setSeparatePG(bool separatePG); void setSeparatePG(bool separatePG);
void setKillSignal(int signal); void setKillSignal(int signal);
pid_t release(); pid_t release();

View file

@ -17,4 +17,12 @@ with import ./config.nix;
''; '';
}; };
closeLog = mkDerivation {
name = "silent";
buildCommand = ''
exec > /dev/null 2>&1
sleep 1000000000
'';
};
} }

View file

@ -24,3 +24,8 @@ if nix-build timeout.nix -A silent --max-silent-time 2; then
echo "build should have failed" echo "build should have failed"
exit 1 exit 1
fi fi
if nix-build timeout.nix -A closeLog; then
echo "build should have failed"
exit 1
fi