libstore: never return from lockFile without a lock

signals could cause lockFile to return without having locked the file.
the garbage collector didn't check for this, and then hilarity ensued.

Change-Id: If86d33595e8bf5510d2b032139342261dc6e07c9

lix/legacy/build-remote.cc

@@ -1,4 +1,5 @@
 #include <algorithm>
+#include <chrono>
 #include <set>
 #include <memory>
 #include <tuple>
@@ -23,9 +24,6 @@
 
 namespace nix {
 
-static void handleAlarm(int sig) {
-}
-
 std::string escapeUri(std::string uri)
 {
     std::replace(uri.begin(), uri.end(), '/', '_');
@@ -279,12 +277,8 @@ connected:
         {
             Activity act(*logger, lvlTalkative, actUnknown, fmt("waiting for the upload lock to '%s'", storeUri));
 
-            auto old = signal(SIGALRM, handleAlarm);
-            alarm(15 * 60);
-            if (!lockFile(uploadLock.get(), ltWrite))
+            if (!unsafeLockFileSingleThreaded(uploadLock.get(), ltWrite, std::chrono::minutes(15)))
                 printError("somebody is hogging the upload lock for '%s', continuing...");
-            alarm(0);
-            signal(SIGALRM, old);
         }
 
         auto substitute = settings.buildersUseSubstitutes ? Substitute : NoSubstitute;

lix/libstore/gc.cc

@@ -129,7 +129,7 @@ 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, false, "");
+    FdLock gcLock(_fdGCLock.lock()->get(), ltRead);
 
     if (!gcLock.acquired) {
         /* We couldn't get a shared global GC lock, so the garbage
@@ -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, true, "waiting for the big garbage collector lock...");
+    FdLock gcLock(fdGCLock.get(), ltWrite, "waiting for the big garbage collector lock...");
 
     /* Synchronisation point to test ENOENT handling in
        addTempRoot(), see tests/gc-non-blocking.sh. */

lix/libstore/local-store.cc

@@ -1489,7 +1489,8 @@ std::pair<Path, AutoCloseFD> LocalStore::createTempDirInStore()
         if (tmpDirFd.get() < 0) {
             continue;
         }
-        lockedByUs = lockFile(tmpDirFd.get(), ltWrite);
+        lockFile(tmpDirFd.get(), ltWrite);
+        lockedByUs = true;
     } while (!pathExists(tmpDirFn) || !lockedByUs);
     return {tmpDirFn, std::move(tmpDirFd)};
 }
@@ -1525,7 +1526,7 @@ bool LocalStore::verifyStore(bool checkContents, RepairFlag repair)
     /* Acquire the global GC lock to get a consistent snapshot of
        existing and valid paths. */
     auto fdGCLock = openGCLock();
-    FdLock gcLock(fdGCLock.get(), ltRead, true, "waiting for the big garbage collector lock...");
+    FdLock gcLock(fdGCLock.get(), ltRead, "waiting for the big garbage collector lock...");
 
     StorePathSet validPaths;
 

lix/libstore/pathlocks.cc

@@ -5,6 +5,7 @@
 #include <cerrno>
 
 #include <fcntl.h>
+#include <kj/common.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/file.h>
@@ -42,10 +43,28 @@ static int convertLockType(LockType lockType)
     else abort();
 }
 
-bool lockFile(int fd, LockType lockType)
+void lockFile(int fd, LockType lockType)
 {
     int type = convertLockType(lockType);
 
+    while (flock(fd, type) != 0) {
+        checkInterrupt();
+        if (errno != EINTR)
+            throw SysError("acquiring lock");
+    }
+}
+
+bool unsafeLockFileSingleThreaded(int fd, LockType lockType, std::chrono::seconds timeout)
+{
+    int type = convertLockType(lockType);
+
+    auto old = signal(SIGALRM, [](int) {});
+    alarm(timeout.count());
+    KJ_DEFER({
+        alarm(0);
+        signal(SIGALRM, old);
+    });
+
     while (flock(fd, type) != 0) {
         checkInterrupt();
         if (errno != EINTR)
@@ -189,16 +208,20 @@ void PathLocks::setDeletion(bool deletePaths)
 }
 
 
-FdLock::FdLock(int fd, LockType lockType, bool wait, std::string_view waitMsg)
+FdLock::FdLock(int fd, LockType lockType)
     : fd(fd)
 {
-    if (wait) {
-        if (!tryLockFile(fd, lockType)) {
-            printInfo("%s", waitMsg);
-            acquired = lockFile(fd, lockType);
-        }
-    } else
-        acquired = tryLockFile(fd, lockType);
+    acquired = tryLockFile(fd, lockType);
+}
+
+FdLock::FdLock(int fd, LockType lockType, std::string_view waitMsg)
+    : fd(fd)
+{
+    if (!tryLockFile(fd, lockType)) {
+        printInfo("%s", waitMsg);
+        lockFile(fd, lockType);
+        acquired = true;
+    }
 }
 
 

lix/libstore/pathlocks.hh

@@ -3,6 +3,7 @@
 
 #include "lix/libutil/error.hh"
 #include "lix/libutil/file-descriptor.hh"
+#include <chrono>
 
 namespace nix {
 
@@ -20,7 +21,14 @@ void deleteLockFile(const Path & path, int fd);
 
 enum LockType { ltRead, ltWrite };
 
-bool lockFile(int fd, LockType lockType);
+void lockFile(int fd, LockType lockType);
+/**
+ * Same as `lockFile`, but with a timeout. This timeout uses the POSIX `alarm`
+ * facility and a `SIGALRM` handler. Using this function from multiple threads
+ * in the same process is not safe: all `SIGALRM` handlers set previously will
+ * be overwritten while this function is executing and are restored on return.
+ */
+bool unsafeLockFileSingleThreaded(int fd, LockType lockType, std::chrono::seconds timeout);
 bool tryLockFile(int fd, LockType lockType);
 void unlockFile(int fd);
 
@@ -48,7 +56,8 @@ struct FdLock
     int fd;
     bool acquired = false;
 
-    FdLock(int fd, LockType lockType, bool wait, std::string_view waitMsg);
+    FdLock(int fd, LockType lockType);
+    FdLock(int fd, LockType lockType, std::string_view waitMsg);
 
     ~FdLock()
     {