From 21948deed99a3295e4d5666e027a6ca42dc00b40 Mon Sep 17 00:00:00 2001
From: Eelco Dolstra <edolstra@gmail.com>
Date: Thu, 19 Jan 2017 16:58:39 +0100
Subject: 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/libstore/build.cc | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

(limited to 'src/libstore/build.cc')

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index e8fb43896b77..7fc6ff0df0f8 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);
-- 
cgit 1.4.1