Revise the repo locker.

Now if the process holding the lock has exited without cleanup, it won't
block future access.

Author: netheril96

sources/full_format.cpp

@@ -8,6 +8,7 @@
 
 #include <absl/container/inlined_vector.h>
 #include <absl/strings/numbers.h>
+#include <absl/strings/str_cat.h>
 #include <absl/strings/str_split.h>
 
 #include <cerrno>
@@ -674,4 +675,42 @@ void FuseHighLevelOps::postprocess_stat(fuse_stat* st)
     }
 }
 
+void RepoLocker::check_lock_file()
+{
+    lock_stream_ = open_lock_stream_checked();
+    auto pid = absl::StrCat(OSService::get_current_process_id());
+    lock_stream_->resize(0);
+    lock_stream_->write(pid.data(), 0, pid.size());
+}
+
+std::shared_ptr<FileStream> RepoLocker::open_lock_stream_checked()
+{
+    try
+    {
+        return root_.open_file_stream(kLockFileName, O_RDWR | O_CREAT | O_EXCL, 0644);
+    }
+    catch (const ExceptionBase& e)
+    {
+        if (e.error_number() != EEXIST)
+        {
+            throw;
+        }
+
+        auto result = root_.open_file_stream(kLockFileName, O_RDWR, 0644);
+        pid_t pid{};
+        if (absl::SimpleAtoi(result->as_string(), &pid) && OSService::is_process_running(pid))
+        {
+            ERROR_LOG("Failed to acquire lock file %s. Process %d is holding "
+                      "the lock.",
+                      root_.norm_path_narrowed(kLockFileName),
+                      pid);
+            throw;
+        }
+        else
+        {
+            return result;
+        }
+    }
+}
+
 }    // namespace securefs::full_format

sources/full_format.h

@@ -27,17 +27,7 @@ class RepoLocker
         {
             return;
         }
-        try
-        {
-            lock_stream_ = root_.open_file_stream(kLockFileName, O_RDONLY | O_CREAT | O_EXCL, 0644);
-        }
-        catch (const std::exception& e)
-        {
-            ERROR_LOG("Failed to acquire lock file %s. Perhaps another securefs process is holding "
-                      "the lock.",
-                      root_.norm_path_narrowed(kLockFileName));
-            throw;
-        }
+        check_lock_file();
     }
 
     ~RepoLocker()
@@ -53,6 +43,9 @@ class RepoLocker
 private:
     DISABLE_COPY_MOVE(RepoLocker)
 
+    void check_lock_file();
+    std::shared_ptr<FileStream> open_lock_stream_checked();
+
     OSService& root_;
     std::shared_ptr<FileStream> lock_stream_;
 };

sources/platform.h

@@ -253,6 +253,7 @@ class OSService
     static std::string stringify_system_error(int errcode);
     static void enter_background();
     static bool is_process_running(pid_t pid);
+    static pid_t get_current_process_id();
 };
 
 struct Colour

sources/unix.cpp

@@ -666,6 +666,7 @@ void OSService::read_password_with_confirmation(const char* prompt,
 }
 
 void OSService::enter_background() { daemon(true, false); }
+pid_t OSService::get_current_process_id() { return getpid(); }
 
 // These two overloads are used to distinguish the GNU and XSI version of strerror_r
 

sources/win.cpp

@@ -1347,6 +1347,8 @@ bool OSService::is_process_running(pid_t pid)
     // we'll assume if it's not ERROR_INVALID_PARAMETER, the PID likely exists.
     return true;
 }
+pid_t OSService::get_current_process_id() { return GetCurrentProcessId(); }
+
 }    // namespace securefs
 
 #endif

test/simple_test.py

@@ -112,9 +112,15 @@ def securefs_mount(
     )
     try:
         for _ in range(600):
-            time.sleep(0.1)
+            try:
+                p.wait(timeout=0.1)
+            except subprocess.TimeoutExpired:
+                pass
+            if p.returncode:
+                raise subprocess.CalledProcessError(p.returncode, p.args)
             if is_mount_then_statvfs(mount_point):
                 return p
+
         raise TimeoutError(f"Failed to mount {repr(mount_point)} after many attempts")
     except:
         securefs_unmount(p=p, mount_point=mount_point)
@@ -882,6 +888,28 @@ def test_regression(self):
             securefs_unmount(p, mount_point)
 
 
+class RepoLockerTestCase(unittest.TestCase):
+    def test_locked(self):
+        data_dir = get_data_dir(fmt=RepoFormat.FULL)
+        securefs_create(data_dir=data_dir, fmt=RepoFormat.FULL, password="123")
+        mount_point = get_mount_point()
+        with open(os.path.join(data_dir, ".securefs.lock"), "w") as f:
+            f.write(str(os.getpid()))
+        with self.assertRaises(subprocess.CalledProcessError):
+            p = securefs_mount(
+                data_dir=data_dir, mount_point=mount_point, password="123"
+            )
+
+    def test_locker_died(self):
+        data_dir: str = get_data_dir(fmt=RepoFormat.FULL)
+        securefs_create(data_dir=data_dir, fmt=RepoFormat.FULL, password="123")
+        mount_point = get_mount_point()
+        with open(os.path.join(data_dir, ".securefs.lock"), "w") as f:
+            f.write(str(2**31 - 1))
+        p = securefs_mount(data_dir=data_dir, mount_point=mount_point, password="123")
+        securefs_unmount(p, mount_point)
+
+
 def list_dir_recursive(dirname: str, relpath=False) -> Set[str]:
     # Note: os.walk does not work on Windows when crossing filesystem boundary.
     # So we use this crude version instead.