From 7bd82718e243389797383cedf9fac950eda9a83f Mon Sep 17 00:00:00 2001
From: Raito Bezarius <raito@lix.systems>
Date: Wed, 11 Jun 2025 13:47:38 +0200
Subject: [PATCH] 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 | 24 +++++++++++++++++++
 lix/libstore/platform/linux.hh |  7 ++++++
 lix/libutil/cgroup.cc          | 44 +++++++++++++++++++++-------------
 lix/libutil/cgroup.hh          |  6 ++---
 4 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/lix/libstore/platform/linux.cc b/lix/libstore/platform/linux.cc
index 229136a93..f8b721475 100644
--- a/lix/libstore/platform/linux.cc
+++ b/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();
diff --git a/lix/libstore/platform/linux.hh b/lix/libstore/platform/linux.hh
index 38c8b6d3a..9dba7f1de 100644
--- a/lix/libstore/platform/linux.hh
+++ b/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
      */
diff --git a/lix/libutil/cgroup.cc b/lix/libutil/cgroup.cc
index f05dd6a1c..5ecc96092 100644
--- a/lix/libutil/cgroup.cc
+++ b/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);
diff --git a/lix/libutil/cgroup.hh b/lix/libutil/cgroup.hh
index 89d6bc916..4833017a9 100644
--- a/lix/libutil/cgroup.hh
+++ b/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(