From 8f5f33c9d80885d596dfef48a0d7b1e9120ef9ea Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Wed, 5 Feb 2025 02:12:56 +0100
Subject: [PATCH] libstore: bind FdLock to AutoCloseFD and make resource-safe

Change-Id: I2451c15bd0e7d3a5a537bbb65502fd4a18ae0c22
---
 lix/libstore/gc.cc          |  6 +++---
 lix/libstore/local-store.cc |  2 +-
 lix/libstore/pathlocks.cc   | 17 +++++++++--------
 lix/libstore/pathlocks.hh   | 34 ++++++++++++++++++++--------------
 4 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/lix/libstore/gc.cc b/lix/libstore/gc.cc
index 9a8fc53c6..e5636a994 100644
--- a/lix/libstore/gc.cc
+++ b/lix/libstore/gc.cc
@@ -129,9 +129,9 @@ void LocalStore::addTempRoot(const StorePath & path)
     /* Try to acquire a shared global GC lock (non-blocking). This
        only succeeds if the garbage collector is not currently
        running. */
-    FdLock gcLock(_fdGCLock.lock()->get(), ltRead);
+    FdLock gcLock(*_fdGCLock.lock(), ltRead, FdLock::dont_wait);
 
-    if (!gcLock.acquired) {
+    if (!gcLock.valid()) {
         /* We couldn't get a shared global GC lock, so the garbage
            collector is running. So we have to connect to the garbage
            collector and inform it about our root. */
@@ -583,7 +583,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results)
        here because then in auto-gc mode, another thread could
        downgrade our exclusive lock. */
     auto fdGCLock = openGCLock();
-    FdLock gcLock(fdGCLock.get(), ltWrite, "waiting for the big garbage collector lock...");
+    FdLock gcLock(fdGCLock, ltWrite, "waiting for the big garbage collector lock...");
 
     /* Synchronisation point to test ENOENT handling in
        addTempRoot(), see tests/gc-non-blocking.sh. */
diff --git a/lix/libstore/local-store.cc b/lix/libstore/local-store.cc
index 4a47c2ce0..e6bd5ab08 100644
--- a/lix/libstore/local-store.cc
+++ b/lix/libstore/local-store.cc
@@ -1527,7 +1527,7 @@ try {
     /* Acquire the global GC lock to get a consistent snapshot of
        existing and valid paths. */
     auto fdGCLock = openGCLock();
-    FdLock gcLock(fdGCLock.get(), ltRead, "waiting for the big garbage collector lock...");
+    FdLock gcLock(fdGCLock, ltRead, "waiting for the big garbage collector lock...");
 
     StorePathSet validPaths;
 
diff --git a/lix/libstore/pathlocks.cc b/lix/libstore/pathlocks.cc
index ba80ed51c..dba1e0d60 100644
--- a/lix/libstore/pathlocks.cc
+++ b/lix/libstore/pathlocks.cc
@@ -1,4 +1,5 @@
 #include "lix/libstore/pathlocks.hh"
+#include "lix/libutil/file-descriptor.hh"
 #include "lix/libutil/logging.hh"
 #include "lix/libutil/signals.hh"
 
@@ -208,19 +209,19 @@ void PathLocks::setDeletion(bool deletePaths)
 }
 
 
-FdLock::FdLock(int fd, LockType lockType)
-    : fd(fd)
+FdLock::FdLock(AutoCloseFD & fd, LockType lockType, DontWait)
 {
-    acquired = tryLockFile(fd, lockType);
+    if (tryLockFile(fd.get(), lockType)) {
+        this->fd.reset(&fd);
+    }
 }
 
-FdLock::FdLock(int fd, LockType lockType, std::string_view waitMsg)
-    : fd(fd)
+FdLock::FdLock(AutoCloseFD & fd, LockType lockType, std::string_view waitMsg)
 {
-    if (!tryLockFile(fd, lockType)) {
+    if (!tryLockFile(fd.get(), lockType)) {
         printInfo("%s", waitMsg);
-        lockFile(fd, lockType);
-        acquired = true;
+        lockFile(fd.get(), lockType);
+        this->fd.reset(&fd);
     }
 }
 
diff --git a/lix/libstore/pathlocks.hh b/lix/libstore/pathlocks.hh
index 2bfb5b5aa..bd44549cf 100644
--- a/lix/libstore/pathlocks.hh
+++ b/lix/libstore/pathlocks.hh
@@ -57,23 +57,29 @@ public:
     void setDeletion(bool deletePaths);
 };
 
-struct FdLock
+class FdLock
 {
-    int fd;
-    bool acquired = false;
-
-    FdLock(int fd, LockType lockType);
-    FdLock(int fd, LockType lockType, std::string_view waitMsg);
-
-    ~FdLock()
+    struct Unlocker
     {
-        try {
-            if (acquired)
-                unlockFile(fd);
-        } catch (SysError &) {
-            ignoreExceptionInDestructor();
+        void operator()(AutoCloseFD * fd)
+        {
+            try {
+                unlockFile(fd->get());
+            } catch (SysError &) {
+                ignoreExceptionInDestructor();
+            }
         }
-    }
+    };
+
+    std::unique_ptr<AutoCloseFD, Unlocker> fd;
+
+public:
+    static constexpr struct DontWait { explicit DontWait() = default; } dont_wait;
+
+    FdLock(AutoCloseFD & fd, LockType lockType, DontWait);
+    FdLock(AutoCloseFD & fd, LockType lockType, std::string_view waitMsg);
+
+    bool valid() const { return bool(fd); }
 };
 
 }