1 files changed, 16 insertions, 22 deletions

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index c46b7cd641c4..7fc6ff0df0f8 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -257,7 +257,7 @@ public:
 
     LocalStore & store;
 
-    std::shared_ptr<HookInstance> hook;
+    std::unique_ptr<HookInstance> hook;
 
     Worker(LocalStore & store);
     ~Worker();
@@ -646,7 +646,7 @@ HookInstance::~HookInstance()
 {
     try {
         toHook.writeSide = -1;
-        pid.kill(true);
+        if (pid != -1) pid.kill(true);
     } catch (...) {
         ignoreException();
     }
@@ -751,7 +751,7 @@ private:
     Pipe userNamespaceSync;
 
     /* The build hook. */
-    std::shared_ptr<HookInstance> hook;
+    std::unique_ptr<HookInstance> hook;
 
     /* Whether we're currently doing a chroot build. */
     bool useChroot = false;
@@ -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);
 
@@ -1566,7 +1564,7 @@ HookReply DerivationGoal::tryBuildHook()
     if (!settings.useBuildHook || getEnv("NIX_BUILD_HOOK") == "" || !useDerivation) return rpDecline;
 
     if (!worker.hook)
-        worker.hook = std::make_shared<HookInstance>();
+        worker.hook = std::make_unique<HookInstance>();
 
     /* Tell the hook about system features (beyond the system type)
        required from the build machine.  (The hook could parse the
@@ -1601,8 +1599,7 @@ HookReply DerivationGoal::tryBuildHook()
 
     printMsg(lvlTalkative, format("using hook to build path(s) %1%") % showPaths(missingPaths));
 
-    hook = worker.hook;
-    worker.hook.reset();
+    hook = std::move(worker.hook);
 
     /* Tell the hook all the inputs that have to be copied to the
        remote system.  This unfortunately has to contain the entire
@@ -2113,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:
@@ -2154,7 +2153,6 @@ void DerivationGoal::startBuilder()
 
         userNamespaceSync.create();
 
-        ProcessOptions options;
         options.allowVfork = false;
 
         Pid helper = startProcess([&]() {
@@ -2190,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;
@@ -2221,7 +2219,6 @@ void DerivationGoal::startBuilder()
     } else
 #endif
     {
-        ProcessOptions options;
         options.allowVfork = !buildUser.enabled() && !drv->isBuiltin();
         pid = startProcess([&]() {
             runChild();
@@ -2295,12 +2292,8 @@ void DerivationGoal::runChild()
                outside of the namespace.  Making a subtree private is
                local to the namespace, though, so setting MS_PRIVATE
                does not affect the outside world. */
-            Strings mounts = tokenizeString<Strings>(readFile("/proc/self/mountinfo", true), "\n");
-            for (auto & i : mounts) {
-                vector<string> fields = tokenizeString<vector<string> >(i, " ");
-                string fs = decodeOctalEscaped(fields.at(4));
-                if (mount(0, fs.c_str(), 0, MS_PRIVATE, 0) == -1)
-                    throw SysError(format("unable to make filesystem ‘%1%’ private") % fs);
+            if (mount(0, "/", 0, MS_REC|MS_PRIVATE, 0) == -1) {
+                throw SysError("unable to make ‘/’ private mount");
             }
 
             /* Bind-mount chroot directory to itself, to treat it as a
@@ -3707,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);