libutil: give Pid proper resource semantics

copy-constructing or assigning from pid_t can easily lead to duplicate
Pid instances for the same process if a pid_t was used carelessly, and
Pid itself was copy-constructible. both could cause surprising results
such as killing processes twice (which could become very problemantic,
but luckily modern systems don't reuse PIDs all that quickly), or more
than one piece of the code believing it owns a process when neither do

Change-Id: Ifea7445f84200b34c1a1d0acc2cdffe0f01e20c6

8 files changed, 35 insertions, 38 deletions

diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc
index 29538a9ca..5dabd0c8e 100644
--- a/src/libmain/shared.cc
+++ b/src/libmain/shared.cc
@@ -354,7 +354,7 @@ RunPager::RunPager()
     Pipe toPager;
     toPager.create();
 
-    pid = startProcess([&]() {
+    pid = Pid{startProcess([&]() {
         if (dup2(toPager.readSide.get(), STDIN_FILENO) == -1)
             throw SysError("dupping stdin");
         if (!getenv("LESS"))
@@ -366,7 +366,7 @@ RunPager::RunPager()
         execlp("less", "less", nullptr);
         execlp("more", "more", nullptr);
         throw SysError("executing '%1%'", pager);
-    });
+    })};
 
     pid.setKillSignal(SIGINT);
     std_out = fcntl(STDOUT_FILENO, F_DUPFD_CLOEXEC, 0);
diff --git a/src/libstore/build/hook-instance.cc b/src/libstore/build/hook-instance.cc
index 9a93646f4..b8d8fe7a4 100644
--- a/src/libstore/build/hook-instance.cc
+++ b/src/libstore/build/hook-instance.cc
@@ -35,7 +35,7 @@ HookInstance::HookInstance()
     builderOut.create();
 
     /* Fork the hook. */
-    pid = startProcess([&]() {
+    pid = Pid{startProcess([&]() {
 
         if (dup2(fromHook.writeSide.get(), STDERR_FILENO) == -1)
             throw SysError("cannot pipe standard error into log file");
@@ -60,7 +60,7 @@ HookInstance::HookInstance()
         execv(buildHook.c_str(), stringsToCharPtrs(args).data());
 
         throw SysError("executing '%s'", buildHook);
-    });
+    })};
 
     pid.setSeparatePG(true);
     fromHook.writeSide.reset();
diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index eb6229c56..a2ff495bf 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -906,7 +906,7 @@ void LocalDerivationGoal::startBuilder()
         Pipe sendPid;
         sendPid.create();
 
-        Pid helper = startProcess([&]() {
+        Pid helper{startProcess([&]() {
             sendPid.readSide.close();
 
             /* We need to open the slave early, before
@@ -934,7 +934,7 @@ void LocalDerivationGoal::startBuilder()
 
             writeFull(sendPid.writeSide.get(), fmt("%d\n", child));
             _exit(0);
-        });
+        })};
 
         sendPid.writeSide.close();
 
@@ -951,7 +951,7 @@ void LocalDerivationGoal::startBuilder()
 
         auto ss = tokenizeString<std::vector<std::string>>(readLine(sendPid.readSide.get()));
         assert(ss.size() == 1);
-        pid = string2Int<pid_t>(ss[0]).value();
+        pid = Pid{string2Int<pid_t>(ss[0]).value()};
 
         if (usingUserNamespace) {
             /* Set the UID/GID mapping of the builder's user namespace
@@ -1010,10 +1010,10 @@ void LocalDerivationGoal::startBuilder()
     } else
 #endif
     {
-        pid = startProcess([&]() {
+        pid = Pid{startProcess([&]() {
             openSlave();
             runChild();
-        });
+        })};
     }
 
     /* parent */
diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc
index 0d7bfa01d..c40eba894 100644
--- a/src/libstore/ssh.cc
+++ b/src/libstore/ssh.cc
@@ -70,7 +70,7 @@ std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(const std::string
     }
     Finally cleanup = [&]() { logger->resume(); };
 
-    conn->sshPid = startProcess([&]() {
+    conn->sshPid = Pid{startProcess([&]() {
         restoreProcessContext();
 
         close(in.writeSide.get());
@@ -99,7 +99,7 @@ std::unique_ptr<SSHMaster::Connection> SSHMaster::startCommand(const std::string
 
         // could not exec ssh/bash
         throw SysError("unable to execute '%s'", args.front());
-    }, options);
+    }, options)};
 
 
     in.readSide.reset();
@@ -147,7 +147,7 @@ Path SSHMaster::startMaster()
     if (isMasterRunning())
         return state->socketPath;
 
-    state->sshMaster = startProcess([&]() {
+    state->sshMaster = Pid{startProcess([&]() {
         restoreProcessContext();
 
         close(out.readSide.get());
@@ -160,7 +160,7 @@ Path SSHMaster::startMaster()
         execvp(args.begin()->c_str(), stringsToCharPtrs(args).data());
 
         throw SysError("unable to execute '%s'", args.front());
-    }, options);
+    }, options)};
 
     out.writeSide.reset();
 
diff --git a/src/libutil/namespaces.cc b/src/libutil/namespaces.cc
index 98d3cd306..4b7a1b577 100644
--- a/src/libutil/namespaces.cc
+++ b/src/libutil/namespaces.cc
@@ -94,12 +94,7 @@ bool userNamespacesSupported()
     static auto res = [&]() -> bool
     {
         try {
-            Pid pid = startProcess([&]()
-            {
-                _exit(0);
-            }, {
-                .cloneFlags = CLONE_NEWUSER
-            });
+            Pid pid{startProcess([&]() { _exit(0); }, {.cloneFlags = CLONE_NEWUSER})};
 
             auto r = pid.wait();
             assert(!r);
@@ -120,8 +115,7 @@ bool mountAndPidNamespacesSupported()
     {
         try {
 
-            Pid pid = startProcess([&]()
-            {
+            Pid pid{startProcess([&]() {
                 /* Make sure we don't remount the parent's /proc. */
                 if (mount(0, "/", 0, MS_PRIVATE | MS_REC, 0) == -1)
                     _exit(1);
@@ -136,7 +130,7 @@ bool mountAndPidNamespacesSupported()
                 _exit(0);
             }, {
                 .cloneFlags = CLONE_NEWNS | CLONE_NEWPID | (userNamespacesSupported() ? CLONE_NEWUSER : 0)
-            });
+            })};
 
             if (pid.wait()) {
                 debug("PID namespaces do not work on this system: cannot remount /proc");
diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc
index 3bb419e45..9c99f047c 100644
--- a/src/libutil/processes.cc
+++ b/src/libutil/processes.cc
@@ -35,23 +35,25 @@ Pid::Pid()
 }
 
 
-Pid::Pid(pid_t pid)
-    : pid(pid)
+Pid::Pid(Pid && other) : pid(other.pid), separatePG(other.separatePG), killSignal(other.killSignal)
 {
+    other.pid = -1;
 }
 
 
-Pid::~Pid() noexcept(false)
+Pid & Pid::operator=(Pid && other)
 {
-    if (pid != -1) kill();
+    Pid tmp(std::move(other));
+    std::swap(pid, tmp.pid);
+    std::swap(separatePG, tmp.separatePG);
+    std::swap(killSignal, tmp.killSignal);
+    return *this;
 }
 
 
-void Pid::operator =(pid_t pid)
+Pid::~Pid() noexcept(false)
 {
-    if (this->pid != -1 && this->pid != pid) kill();
-    this->pid = pid;
-    killSignal = SIGKILL; // reset signal to default
+    if (pid != -1) kill();
 }
 
 
@@ -125,7 +127,7 @@ void killUser(uid_t uid)
        users to which the current process can send signals.  So we
        fork a process, switch to uid, and send a mass kill. */
 
-    Pid pid = startProcess([&]() {
+    Pid pid{startProcess([&]() {
 
         if (setuid(uid) == -1)
             throw SysError("setting uid");
@@ -147,7 +149,7 @@ void killUser(uid_t uid)
         }
 
         _exit(0);
-    });
+    })};
 
     int status = pid.wait();
     if (status != 0)
@@ -288,7 +290,7 @@ void runProgram2(const RunOptions & options)
     }
 
     /* Fork. */
-    Pid pid = startProcess([&]() {
+    Pid pid{startProcess([&]() {
         if (options.environment)
             replaceEnv(*options.environment);
         if (options.standardOut && dup2(out.writeSide.get(), STDOUT_FILENO) == -1)
@@ -322,7 +324,7 @@ void runProgram2(const RunOptions & options)
             execv(options.program.c_str(), stringsToCharPtrs(args_).data());
 
         throw SysError("executing '%1%'", options.program);
-    }, processOptions);
+    }, processOptions)};
 
     out.writeSide.close();
 
diff --git a/src/libutil/processes.hh b/src/libutil/processes.hh
index 2079e1b36..8fc241ab1 100644
--- a/src/libutil/processes.hh
+++ b/src/libutil/processes.hh
@@ -26,9 +26,10 @@ class Pid
     int killSignal = SIGKILL;
 public:
     Pid();
-    Pid(pid_t pid);
+    explicit Pid(pid_t pid): pid(pid) {}
+    Pid(Pid && other);
+    Pid & operator=(Pid && other);
     ~Pid() noexcept(false);
-    void operator =(pid_t pid);
     explicit operator bool() const { return pid != -1; }
     int kill();
     int wait();
diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc
index a6e46ca50..d4fc37fab 100644
--- a/src/libutil/unix-domain-socket.cc
+++ b/src/libutil/unix-domain-socket.cc
@@ -65,7 +65,7 @@ static void bindConnectProcHelper(
     if (path.size() + 1 >= sizeof(addr.sun_path)) {
         Pipe pipe;
         pipe.create();
-        Pid pid = startProcess([&] {
+        Pid pid{startProcess([&] {
             try {
                 pipe.readSide.close();
                 Path dir = dirOf(path);
@@ -83,7 +83,7 @@ static void bindConnectProcHelper(
             } catch (...) {
                 writeFull(pipe.writeSide.get(), "-1\n");
             }
-        });
+        })};
         pipe.writeSide.close();
         auto errNo = string2Int<int>(chomp(drainFD(pipe.readSide.get())));
         if (!errNo || *errNo == -1)