[RHELC-1124] Refactor applock to have a more robust API.

convert2rhel/applock.py

@@ -18,8 +18,10 @@
 __metaclass__ = type
 
 import errno
+import fcntl
 import logging
 import os
+import stat
 import tempfile
 
 
@@ -51,6 +53,11 @@ class ApplicationLock:
     process is not running, it removes the file and tries to lock it
     again.
 
+    File locking: to avoid a race condition in which another process
+    overwrites the PID file between the time we check for the process's
+    existence and we unlink the stale lockfile, we lock the file using
+    BSD file locking.
+
     For safety, unexpected conditions, like garbage in the file or
     bad permissions, are treated as if the application is locked,
     because something is obviously wrong.
@@ -59,24 +66,24 @@ class ApplicationLock:
     def __init__(self, name):
         # Our application name
         self._name = name
-        # Do we think we locked the pid file?
-        self._locked = False
-        # Our process ID
+        # Our process ID. We save this when the lock is created so it will be
+        # consistent even if we check from inside a fork.
         self._pid = os.getpid()
         # Path to the file that contains the process id
         self._pidfile = os.path.join(_DEFAULT_LOCK_DIR, self._name + ".pid")
 
     def __str__(self):
-        if self._locked:
+        if self.is_locked:
             status = "locked"
         else:
             status = "unlocked"
         return "%s PID %d %s" % (self._pidfile, self._pid, status)
 
     def _try_create(self):
-        """Try to create the lock file. If this succeeds, the lock file
-        exists and we created it. If an exception other than the one
-        we expect is raised, re-raises it.
+        """Try to create the lock file. If this succeeds, the lock
+        file exists and we created it, so we hold the lock. If an
+        exception other than the one we expect is raised, re-raises
+        it.
 
         :returns: True if we created the lock, False if we didn't.
         """
@@ -89,6 +96,10 @@ def _try_create(self):
         # Note that NamedTemporaryFile will clean up the file it created,
         # but the lockfile we created by doing the link will stay around.
         with tempfile.NamedTemporaryFile(mode="w", suffix=".pid", prefix=self._name, dir=_DEFAULT_LOCK_DIR) as f:
+            # Elsewhere in the code, we use flock() on this file;
+            # using that call on a group- or world-readable file poses an
+            # extreme security risk.
+            os.chmod(f.name, stat.S_IRUSR | stat.S_IWUSR)
             f.write(str(self._pid) + "\n")
             f.flush()
             try:
@@ -97,14 +108,28 @@ def _try_create(self):
             except OSError as exc:
                 if exc.errno == errno.EEXIST:
                     return False
-                raise exc
+                raise
         loggerinst.debug("%s." % self)
         return True
 
     @property
     def is_locked(self):
-        """Test whether this object is locked."""
-        return self._locked
+        """Test whether this object was locked by this instance of
+        the application."""
+        try:
+            with open(self._pidfile, "r") as filep:
+                fcntl.flock(filep, fcntl.LOCK_EX)
+                try:
+                    file_contents = filep.read()
+                    pid = int(file_contents.rstrip())
+                    if pid:
+                        return pid == self._pid
+                finally:
+                    fcntl.flock(filep, fcntl.LOCK_UN)
+        except (IOError, OSError) as exc:
+            if exc.errno == errno.ENOENT:
+                pass
+        return False
 
     @staticmethod
     def _pid_exists(pid):
@@ -115,57 +140,71 @@ def _pid_exists(pid):
                 os.kill(pid, 0)
         except OSError as exc:
             # The only other (theoretical) possibility is EPERM, which
-            # would mean the process exists.
+            # would mean the process exists and therefore we should return
+            # True.
             if exc.errno == errno.ESRCH:
                 return False
         return True
 
-    def try_to_lock(self, _recursive=False):
-        """Try to get a lock on this application. If successful,
-        the application will be locked; the lock should be released
-        with unlock().
+    def _safe_unlink(self):
+        """Unlink the lock file. If the unlink fails because the file
+        doesn't exist, swallow the exception; this avoids spurious
+        errors due to race conditions.
+        """
+        try:
+            os.unlink(self._pidfile)
+        except OSError as exc:
+            # In Python 3 this could be changed to FileNotFoundError.
+            if exc.errno == errno.ENOENT:
+                return
+            raise
+
+    def try_to_lock(self):
+        """Try to get a lock on this application. If this method does
+        not raise an Exception, the application will be locked and we
+        hold the lock; the lock should be released with unlock().
 
         If the file has unexpected contents, for safety we treat the
         application as locked, since it is probably the result of
         manual meddling, intentional or otherwise.
 
-        :keyword _recursive: True if we are being called recursively
-                             and should not try to clean up the lockfile
-                             again.
         :raises ApplicationLockedError: the application is locked
         """
         if self._try_create():
-            self._locked = True
             return
-        if _recursive:
-            raise ApplicationLockedError("Cannot lock %s" % self._name)
-
-        with open(self._pidfile, "r") as f:
-            file_contents = f.read()
-        try:
-            pid = int(file_contents.rstrip())
-        except ValueError:
-            raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
-
-        if self._pid_exists(pid):
-            raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
-        # The lock file was created by a process that has exited;
-        # remove it and try again.
-        loggerinst.info("Cleaning up lock held by exited process %d." % pid)
-        os.unlink(self._pidfile)
-        self.try_to_lock(_recursive=True)
+        with open(self._pidfile, "r") as filep:
+            fcntl.flock(filep, fcntl.LOCK_EX)
+            try:
+                file_contents = filep.read()
+                pid = int(file_contents.rstrip())
+                if pid == self._pid:
+                    return
+                if self._pid_exists(pid):
+                    raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
+                # The lock file was created by a process that has exited;
+                # remove it and try again.
+                loggerinst.info("Cleaning up lock held by exited process %d." % pid)
+                self._safe_unlink()
+            except ValueError:
+                raise ApplicationLockedError("%s has invalid contents" % (self._pidfile))
+            finally:
+                fcntl.flock(filep, fcntl.LOCK_UN)
+
+        if not self._try_create():
+            # Between the unlink and our attempt to create the lock
+            # file, another process got there first.
+            raise ApplicationLockedError("%s is locked" % self._pidfile)
 
     def unlock(self):
         """Release the lock on this application.
 
-        Note that if the unlink fails (a pathological failure) the
-        object will stay locked and the OSError or other
+        Note that if the safe unlink fails (a pathological failure)
+        the object will stay locked and the OSError or other
         system-generated exception will be raised.
         """
-        if not self._locked:
+        if not self.is_locked:
             return
-        os.unlink(self._pidfile)
-        self._locked = False
+        self._safe_unlink()
         loggerinst.debug("%s." % self)
 
     def __enter__(self):

convert2rhel/unit_tests/applock_test.py

@@ -51,13 +51,22 @@ def test_applock_basic(tmp_lock):
 
 def test_applock_basic_islocked(tmp_lock):
     with open(tmp_lock._pidfile, "w") as f:
-        pid = os.getpid()
-        f.write(str(pid) + "\n")
+        # Our parent process will be running and have a different pid
+        ppid = os.getppid()
+        f.write(str(ppid) + "\n")
     with pytest.raises(applock.ApplicationLockedError):
         tmp_lock.try_to_lock()
     os.unlink(tmp_lock._pidfile)
 
 
+def test_applock_basic_lock_idempotent(tmp_lock):
+    with open(tmp_lock._pidfile, "w") as f:
+        pid = os.getpid()
+        f.write(str(pid) + "\n")
+    tmp_lock.try_to_lock()
+    os.unlink(tmp_lock._pidfile)
+
+
 def test_applock_basic_reap(tmp_lock):
     """Test the case where the lockfile was held by a process
     that has exited."""