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.

Author: edolstra

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();

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);

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));

src/libutil/util.hh

@@ -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();

tests/timeout.nix

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

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