libstore/platform/linux: destroy cgroup before we release user locks

User locks are taken to avoid another build grabbing the same UID.

Under build user contention, it is possible to recycle the same UID from
another build which did not run the Goal destructor yet.

Prior to this change, cgroups were destroyed at Goal destruction time,
but user locks were released at `buildDone()` time.

Therefore, it was possible to have 2 builds fights for the same cgroup
and mess with it, resulting in confusion.

To avoid this, we override `cleanupHookFinally` in charge to release the
user locks and we destroy the cgroup before releasing the locks.

Statistics are kept in the `cgroup` object a bit longer and can be
obtained at `killSandbox(true)` time.

`AutoDestroyCgroup::kill` now ignore if the cgroup path has already been
destroyed, as kill is idempotent.

Reported-by: Ramses <@rvdp:infosec.exchange>
Reported-by: Frederico Schonborn <@fredericoschonborn:matrix.org>
Change-Id: Idfbf9aaf010c5f718f2c1c38548383d912d8ee95
Signed-off-by: Raito Bezarius <raito@lix.systems>

lix/libstore/platform/linux.cc

@@ -1007,9 +1007,33 @@ Pid LinuxLocalDerivationGoal::startChild(std::function<void()> openSlave)
     return pid;
 }
 
+void LinuxLocalDerivationGoal::cleanupHookFinally()
+{
+    /* This hook is used to release the build users
+     * and release the lock on this UID.
+     *
+     * So we need to ensure that our cgroup business
+     * is already done before releasing it,
+     * otherwise, another build may grab the UID
+     * and start a cgroup with it, resulting
+     * in a confusing set of errors.
+     *
+     * Statistics are stored inside the cgroup
+     * object so that `killSandbox` can retrieve
+     * them later.
+     */
+    if (context.cgroup) {
+        context.cgroup->destroy();
+    }
+
+    LocalDerivationGoal::cleanupHookFinally();
+}
+
 void LinuxLocalDerivationGoal::killSandbox(bool getStats)
 {
     if (context.cgroup) {
+        /* This might have already been killed
+         * by the clean-up hook above. */
         context.cgroup->kill();
         if (getStats) {
             auto stats = context.cgroup->getStatistics();

lix/libstore/platform/linux.hh

@@ -34,6 +34,13 @@ public:
     using LocalDerivationGoal::LocalDerivationGoal;
 
 private:
+    /*
+     * Destroy the cgroup otherwise another build
+     * may grab the current UID which is used in the cgroup name
+     * and then mess with a cgroup we might be reading statistics from.
+     */
+    void cleanupHookFinally() override;
+
     /**
      * Create and populate chroot
      */

lix/libutil/cgroup.cc

@@ -291,25 +291,32 @@ AutoDestroyCgroup::AutoDestroyCgroup(
     delegation_ = {.uid = uid, .gid = gid};
 }
 
+void AutoDestroyCgroup::destroy()
+{
+    std::visit(
+        overloaded{
+            [&, this](const Path & aliveCgroup) {
+                auto maybeStats = destroyCgroup(name_, aliveCgroup);
+                if (!maybeStats) {
+                    warn(
+                        "cgroup '%s' was destroyed unexpectedly (something else removed the "
+                        "cgroup).",
+                        aliveCgroup
+                    );
+                } else {
+                    cgroup_ = *maybeStats;
+                }
+            },
+            [&](const CgroupStats & stats) {}
+        },
+        cgroup_
+    );
+}
+
 AutoDestroyCgroup::~AutoDestroyCgroup()
 {
     try {
-        std::visit(
-            overloaded{
-                [&, this](const Path & aliveCgroup) {
-                    auto maybeStats = destroyCgroup(name_, aliveCgroup);
-                    if (!maybeStats) {
-                        warn(
-                            "cgroup '%s' was destroyed unexpectedly (something else removed the "
-                            "cgroup).",
-                            aliveCgroup
-                        );
-                    }
-                },
-                [&](const CgroupStats & stats) {}
-            },
-            cgroup_
-        );
+        destroy();
     } catch (...) {
         ignoreExceptionInDestructor();
     }
@@ -347,7 +354,10 @@ void AutoDestroyCgroup::kill()
 {
     auto path = std::get_if<std::filesystem::path>(&cgroup_);
     if (!path) {
-        throw SysError("killing cgroup '%s' but it went away", name_);
+        /* If the cgroup already disappeared,
+         * processes already got killed.
+         */
+        return;
     }
 
     killCgroup(name_, *path);

lix/libutil/cgroup.hh

@@ -141,9 +141,6 @@ private:
      */
     AutoDelete stateRecord;
 
-    /* Kill all processes under its hierarchy and tear down the cgroup */
-    void destroy();
-
     /*
      * Cleanse all previous instances of this cgroup where the deletion process
      * might have been interrupted and record ourself in stead.
@@ -167,6 +164,9 @@ public:
     );
     ~AutoDestroyCgroup();
 
+    /* Kill all processes under its hierarchy and tear down the cgroup */
+    void destroy();
+
     std::optional<Path> path() const
     {
         return std::visit(