From 5da05dbbbd9855fec4cbcfaa589a766208475804 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Thu, 2 Nov 2023 11:04:17 -0400
Subject: [PATCH 01/16] Refactored applock to have a more robust API.

---
 convert2rhel/applock.py                 | 55 ++++++++++++++-----------
 convert2rhel/unit_tests/applock_test.py |  5 ++-
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index a337c54263..e7fbc01c23 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -59,15 +59,13 @@ 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
         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"
@@ -103,8 +101,28 @@ def _try_create(self):
 
     @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."""
+        pid = self._read_pidfile()
+        if pid is not None:
+            return pid == self._pid
+        return False
+
+    def _read_pidfile(self):
+        """Read and return the contents of the PID file.
+
+        :returns: the file contents as an integer, or None if it doesn't exist
+        :raises: ApplicationLockedError if the contents are corrupt
+        """
+        if os.path.exists(self._pidfile):
+            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)
+            return pid
+        return None
 
     @staticmethod
     def _pid_exists(pid):
@@ -120,7 +138,7 @@ def _pid_exists(pid):
                 return False
         return True
 
-    def try_to_lock(self, _recursive=False):
+    def try_to_lock(self):
         """Try to get a lock on this application. If successful,
         the application will be locked; the lock should be released
         with unlock().
@@ -129,31 +147,23 @@ def try_to_lock(self, _recursive=False):
         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)
-
+        pid = self._read_pidfile()
+        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)
         os.unlink(self._pidfile)
-        self.try_to_lock(_recursive=True)
+        if not self._try_create():
+            # Race condition: 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.
@@ -162,10 +172,9 @@ def unlock(self):
         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
         loggerinst.debug("%s." % self)
 
     def __enter__(self):
diff --git a/convert2rhel/unit_tests/applock_test.py b/convert2rhel/unit_tests/applock_test.py
index 2898ba6ee3..d6cbf06f94 100644
--- a/convert2rhel/unit_tests/applock_test.py
+++ b/convert2rhel/unit_tests/applock_test.py
@@ -51,8 +51,9 @@ 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)

From 8a1fbc7e4ee917f1a57cee31cf62be71b59b4d0a Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Thu, 2 Nov 2023 11:46:57 -0400
Subject: [PATCH 02/16] Test that locking is idempotent.

---
 convert2rhel/unit_tests/applock_test.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/convert2rhel/unit_tests/applock_test.py b/convert2rhel/unit_tests/applock_test.py
index d6cbf06f94..03a9bb8baf 100644
--- a/convert2rhel/unit_tests/applock_test.py
+++ b/convert2rhel/unit_tests/applock_test.py
@@ -59,6 +59,14 @@ def test_applock_basic_islocked(tmp_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."""

From d01a15b0c5bc8e0fb9bfbc3560a2c7777ed7054d Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Wed, 10 Jan 2024 09:51:47 -0500
Subject: [PATCH 03/16] Address potential race conditions.

---
 convert2rhel/applock.py | 43 ++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index e7fbc01c23..a1bafd430c 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -115,10 +115,18 @@ def _read_pidfile(self):
         :raises: ApplicationLockedError if the contents are corrupt
         """
         if os.path.exists(self._pidfile):
-            with open(self._pidfile, "r") as f:
-                file_contents = f.read()
             try:
+                with open(self._pidfile, "r") as f:
+                    file_contents = f.read()
                 pid = int(file_contents.rstrip())
+            except OSError as exc:
+                # This addresses a race condition in which another process
+                # deletes the lock file between our check that it exists
+                # and our attempt to read the contents.
+                # In Python 3 this could be changed to FileNotFoundError.
+                if exc.errno == errno.ENOENT:
+                    return None
+                raise exc
             except ValueError:
                 raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
             return pid
@@ -138,10 +146,23 @@ def _pid_exists(pid):
                 return False
         return True
 
+    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 exc
+
     def try_to_lock(self):
-        """Try to get a lock on this application. If successful,
-        the application will be locked; the lock should be released
-        with unlock().
+        """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
@@ -159,22 +180,22 @@ def try_to_lock(self):
         # 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._safe_unlink()
         if not self._try_create():
-            # Race condition: between the unlink and our attempt to
-            # create the lock file, another process got there first.
+            # 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.is_locked:
             return
-        os.unlink(self._pidfile)
+        self._safe_unlink()
         loggerinst.debug("%s." % self)
 
     def __enter__(self):

From 431844e3d0ca42a42ec1580511b81adc4bcc6530 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Wed, 10 Jan 2024 10:43:48 -0500
Subject: [PATCH 04/16] Address Toshio's commments.

---
 convert2rhel/applock.py | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index a1bafd430c..acb5aae1ca 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -59,7 +59,8 @@ class ApplicationLock:
     def __init__(self, name):
         # Our application name
         self._name = name
-        # 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")
@@ -95,7 +96,7 @@ def _try_create(self):
             except OSError as exc:
                 if exc.errno == errno.EEXIST:
                     return False
-                raise exc
+                raise
         loggerinst.debug("%s." % self)
         return True
 
@@ -114,23 +115,19 @@ def _read_pidfile(self):
         :returns: the file contents as an integer, or None if it doesn't exist
         :raises: ApplicationLockedError if the contents are corrupt
         """
-        if os.path.exists(self._pidfile):
-            try:
-                with open(self._pidfile, "r") as f:
-                    file_contents = f.read()
-                pid = int(file_contents.rstrip())
-            except OSError as exc:
-                # This addresses a race condition in which another process
-                # deletes the lock file between our check that it exists
-                # and our attempt to read the contents.
-                # In Python 3 this could be changed to FileNotFoundError.
-                if exc.errno == errno.ENOENT:
-                    return None
-                raise exc
-            except ValueError:
-                raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
+        try:
+            with open(self._pidfile, "r") as f:
+                file_contents = f.read()
+            pid = int(file_contents.rstrip())
             return pid
-        return None
+        except IOError as exc:
+            # The open() failed because the file exists.
+            # In Python 3 this could be changed to FileNotFoundError.
+            if exc.errno == errno.ENOENT:
+                return None
+            raise
+        except ValueError:
+            raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
 
     @staticmethod
     def _pid_exists(pid):
@@ -157,7 +154,7 @@ def _safe_unlink(self):
             # In Python 3 this could be changed to FileNotFoundError.
             if exc.errno == errno.ENOENT:
                 return
-            raise exc
+            raise
 
     def try_to_lock(self):
         """Try to get a lock on this application. If this method does

From 04c7e3efa0d2bc3d3dc53444fb47b3d74432a56b Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Thu, 7 Mar 2024 10:38:42 -0500
Subject: [PATCH 05/16] Make PID file close explicit (needed for file locking).

---
 convert2rhel/applock.py | 44 ++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index acb5aae1ca..2f82b550dc 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -20,6 +20,7 @@
 import errno
 import logging
 import os
+import stat
 import tempfile
 
 
@@ -64,6 +65,8 @@ def __init__(self, name):
         self._pid = os.getpid()
         # Path to the file that contains the process id
         self._pidfile = os.path.join(_DEFAULT_LOCK_DIR, self._name + ".pid")
+        # File object for _pidfile when the file is locked
+        self._pidfile_fp = None
 
     def __str__(self):
         if self.is_locked:
@@ -88,6 +91,7 @@ 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:
+            os.chmod(f.name, stat.S_IRUSR | stat.S_IWUSR)
             f.write(str(self._pid) + "\n")
             f.flush()
             try:
@@ -105,19 +109,33 @@ def is_locked(self):
         """Test whether this object was locked by this instance of
         the application."""
         pid = self._read_pidfile()
+        self._release_pidfile_flock()
         if pid is not None:
             return pid == self._pid
         return False
 
+    def _release_pidfile_flock(self):
+        """Release the advisory file lock we hold on the PID file
+        and close the open file descriptor."""
+        if self._pidfile_fp is not None:
+            if not self._pidfile_fp.closed:
+                self._pidfile_fp.close()
+            self._pidfile_fp = None
+
     def _read_pidfile(self):
         """Read and return the contents of the PID file.
 
+        If this method does not raise an exception, the
+        _release_pidfile_flock() method must be called to release the
+        advisory lock and close the file.
+
         :returns: the file contents as an integer, or None if it doesn't exist
         :raises: ApplicationLockedError if the contents are corrupt
         """
+        # pylint: disable=consider-using-with
         try:
-            with open(self._pidfile, "r") as f:
-                file_contents = f.read()
+            self._pidfile_fp = open(self._pidfile, "r")
+            file_contents = self._pidfile_fp.read()
             pid = int(file_contents.rstrip())
             return pid
         except IOError as exc:
@@ -127,6 +145,7 @@ def _read_pidfile(self):
                 return None
             raise
         except ValueError:
+            self._release_pidfile_flock()
             raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
 
     @staticmethod
@@ -169,15 +188,18 @@ def try_to_lock(self):
         """
         if self._try_create():
             return
-        pid = self._read_pidfile()
-        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()
+        try:
+            pid = self._read_pidfile()
+            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()
+        finally:
+            self._release_pidfile_flock()
         if not self._try_create():
             # Between the unlink and our attempt to create the lock
             # file, another process got there first.

From 5f3aa7ef4b75e82b5f346069b6382c03bd90eb07 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Mon, 11 Mar 2024 09:39:08 -0400
Subject: [PATCH 06/16] Add flock() calls.

---
 convert2rhel/applock.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index 2f82b550dc..917efbbf12 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -18,6 +18,7 @@
 __metaclass__ = type
 
 import errno
+import fcntl
 import logging
 import os
 import stat
@@ -52,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.
@@ -91,6 +97,8 @@ 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:
+            # Using flock() 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()
@@ -118,6 +126,7 @@ def _release_pidfile_flock(self):
         """Release the advisory file lock we hold on the PID file
         and close the open file descriptor."""
         if self._pidfile_fp is not None:
+            fcntl.flock(self._pidfile_fp, fcntl.LOCK_UN)
             if not self._pidfile_fp.closed:
                 self._pidfile_fp.close()
             self._pidfile_fp = None
@@ -135,6 +144,7 @@ def _read_pidfile(self):
         # pylint: disable=consider-using-with
         try:
             self._pidfile_fp = open(self._pidfile, "r")
+            fcntl.flock(self._pidfile_fp, fcntl.LOCK_EX)
             file_contents = self._pidfile_fp.read()
             pid = int(file_contents.rstrip())
             return pid

From c154b1b76592851b67fb9ec63340c5624311536d Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Tue, 7 May 2024 09:48:11 -0400
Subject: [PATCH 07/16] Don't use 'is not None' idiom.

---
 convert2rhel/applock.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index 917efbbf12..ddb862833a 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -118,14 +118,14 @@ def is_locked(self):
         the application."""
         pid = self._read_pidfile()
         self._release_pidfile_flock()
-        if pid is not None:
+        if pid:
             return pid == self._pid
         return False
 
     def _release_pidfile_flock(self):
         """Release the advisory file lock we hold on the PID file
         and close the open file descriptor."""
-        if self._pidfile_fp is not None:
+        if self._pidfile_fp:
             fcntl.flock(self._pidfile_fp, fcntl.LOCK_UN)
             if not self._pidfile_fp.closed:
                 self._pidfile_fp.close()

From 383e33c791bd5d44ae0c2b609004723bec6a9c71 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Tue, 4 Jun 2024 09:47:19 -0400
Subject: [PATCH 08/16] Rework control flow to be able to use context manager
 with lock file.

---
 convert2rhel/applock.py | 96 ++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 58 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index ddb862833a..cc6d27606e 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -71,8 +71,6 @@ def __init__(self, name):
         self._pid = os.getpid()
         # Path to the file that contains the process id
         self._pidfile = os.path.join(_DEFAULT_LOCK_DIR, self._name + ".pid")
-        # File object for _pidfile when the file is locked
-        self._pidfile_fp = None
 
     def __str__(self):
         if self.is_locked:
@@ -82,9 +80,10 @@ def __str__(self):
         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.
         """
@@ -97,7 +96,8 @@ 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:
-            # Using flock() on a group- or world-readable file poses an
+            # 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")
@@ -116,47 +116,20 @@ def _try_create(self):
     def is_locked(self):
         """Test whether this object was locked by this instance of
         the application."""
-        pid = self._read_pidfile()
-        self._release_pidfile_flock()
-        if pid:
-            return pid == self._pid
-        return False
-
-    def _release_pidfile_flock(self):
-        """Release the advisory file lock we hold on the PID file
-        and close the open file descriptor."""
-        if self._pidfile_fp:
-            fcntl.flock(self._pidfile_fp, fcntl.LOCK_UN)
-            if not self._pidfile_fp.closed:
-                self._pidfile_fp.close()
-            self._pidfile_fp = None
-
-    def _read_pidfile(self):
-        """Read and return the contents of the PID file.
-
-        If this method does not raise an exception, the
-        _release_pidfile_flock() method must be called to release the
-        advisory lock and close the file.
-
-        :returns: the file contents as an integer, or None if it doesn't exist
-        :raises: ApplicationLockedError if the contents are corrupt
-        """
-        # pylint: disable=consider-using-with
         try:
-            self._pidfile_fp = open(self._pidfile, "r")
-            fcntl.flock(self._pidfile_fp, fcntl.LOCK_EX)
-            file_contents = self._pidfile_fp.read()
-            pid = int(file_contents.rstrip())
-            return pid
-        except IOError as exc:
-            # The open() failed because the file exists.
-            # In Python 3 this could be changed to FileNotFoundError.
+            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:
-                return None
-            raise
-        except ValueError:
-            self._release_pidfile_flock()
-            raise ApplicationLockedError("Lock file %s is corrupt" % self._pidfile)
+                pass
+        return False
 
     @staticmethod
     def _pid_exists(pid):
@@ -167,7 +140,8 @@ 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
@@ -198,18 +172,24 @@ def try_to_lock(self):
         """
         if self._try_create():
             return
-        try:
-            pid = self._read_pidfile()
-            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()
-        finally:
-            self._release_pidfile_flock()
+        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.

From 43431db7b3defaac5daca71a046a85bbec8da29e Mon Sep 17 00:00:00 2001
From: Joseph Chapman <33102029+jochapma@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:34:12 -0400
Subject: [PATCH 09/16] Improve commenting.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>
---
 convert2rhel/applock.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index cc6d27606e..9cd4aae7b3 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -99,6 +99,8 @@ def _try_create(self):
             # 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.
+            # stat.S_IRUSR = Owner has read permission
+            # stat.S_IWUSR = Owner has write permission
             os.chmod(f.name, stat.S_IRUSR | stat.S_IWUSR)
             f.write(str(self._pid) + "\n")
             f.flush()

From 3abb270bf50317a1e25f10f5a4e24448117679ec Mon Sep 17 00:00:00 2001
From: Joseph Chapman <33102029+jochapma@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:34:58 -0400
Subject: [PATCH 10/16] Document fcntl flags.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>
---
 convert2rhel/applock.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index 9cd4aae7b3..b0337ae06f 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -120,6 +120,7 @@ def is_locked(self):
         the application."""
         try:
             with open(self._pidfile, "r") as filep:
+                # fcntl.LOCK_EX = Acquire an exclusive lock
                 fcntl.flock(filep, fcntl.LOCK_EX)
                 try:
                     file_contents = filep.read()

From e45531d47d867f5e64e96f7c1c15576eb3b8e54b Mon Sep 17 00:00:00 2001
From: Joseph Chapman <33102029+jochapma@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:35:23 -0400
Subject: [PATCH 11/16] Document fcntl flags.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>
---
 convert2rhel/applock.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index b0337ae06f..66042ea030 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -176,6 +176,7 @@ def try_to_lock(self):
         if self._try_create():
             return
         with open(self._pidfile, "r") as filep:
+            # fcntl.LOCK_EX = Acquire an exclusive lock
             fcntl.flock(filep, fcntl.LOCK_EX)
             try:
                 file_contents = filep.read()

From 2012108e0a08bc47ab361334c7399607d44a2788 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <33102029+jochapma@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:35:50 -0400
Subject: [PATCH 12/16] Document fcntl flags.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>
---
 convert2rhel/applock.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index 66042ea030..6aba2f6d11 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -128,6 +128,7 @@ def is_locked(self):
                     if pid:
                         return pid == self._pid
                 finally:
+                    # fcntl.LOCK_UN = Release an existing lock
                     fcntl.flock(filep, fcntl.LOCK_UN)
         except (IOError, OSError) as exc:
             if exc.errno == errno.ENOENT:

From 86347be1e8955e8193901ce7148da6c5096fb0e2 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <33102029+jochapma@users.noreply.github.com>
Date: Wed, 28 Aug 2024 09:36:16 -0400
Subject: [PATCH 13/16] Document fcntl flags.

Co-authored-by: Freya Gustavsson <freya@venefilyn.se>
---
 convert2rhel/applock.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index 6aba2f6d11..d6d4a250fe 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -193,6 +193,7 @@ def try_to_lock(self):
             except ValueError:
                 raise ApplicationLockedError("%s has invalid contents" % (self._pidfile))
             finally:
+                # fcntl.LOCK_UN = Release an existing lock
                 fcntl.flock(filep, fcntl.LOCK_UN)
 
         if not self._try_create():

From 311888bd326a6b8aa126222fc2edee6f6265e37b Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Wed, 28 Aug 2024 11:27:50 -0400
Subject: [PATCH 14/16] Removed auto-cleanup code.

---
 convert2rhel/applock.py                 | 102 ++++++++----------------
 convert2rhel/unit_tests/applock_test.py |  10 ---
 2 files changed, 35 insertions(+), 77 deletions(-)

diff --git a/convert2rhel/applock.py b/convert2rhel/applock.py
index d6d4a250fe..66b9e0fba4 100644
--- a/convert2rhel/applock.py
+++ b/convert2rhel/applock.py
@@ -18,7 +18,6 @@
 __metaclass__ = type
 
 import errno
-import fcntl
 import logging
 import os
 import stat
@@ -47,16 +46,14 @@ class ApplicationLock:
 
     The implementation uses a standard Linux PID file. When a program
     that uses ApplicationLock starts, it writes its process ID to a
-    file in /var/run/lock. If the file already exists, it reads the
-    PID and checks to see if the process is still running. If the
-    process is still running, it raises ApplicationLockedError. If the
-    process is not running, it removes the file and tries to lock it
-    again.
+    file in /var/run/lock. If the file already exists, it reads and
+    reports the PID that it finds.
 
-    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.
+    While it would be nice to try to clean up a stale process ID file
+    (i.e., a file created by a process that has exited) that opens up
+    a race condition that would require us to leave the process ID
+    file pointer open across the invocation of several methods, which
+    would complicate the code more than the feature is worth.
 
     For safety, unexpected conditions, like garbage in the file or
     bad permissions, are treated as if the application is locked,
@@ -96,9 +93,6 @@ 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.
             # stat.S_IRUSR = Owner has read permission
             # stat.S_IWUSR = Owner has write permission
             os.chmod(f.name, stat.S_IRUSR | stat.S_IWUSR)
@@ -114,41 +108,35 @@ def _try_create(self):
         loggerinst.debug("%s." % self)
         return True
 
-    @property
-    def is_locked(self):
-        """Test whether this object was locked by this instance of
-        the application."""
+    def _get_pid_from_file(self):
+        """If the lock file exists, return the process ID stored in
+        the file. No check is made whether the process exists. Because
+        of the way we create the lock file, the case where it has bad
+        data or is empty is a pathological case.
+
+        :returns: The process ID, or None if the file does not exist.
+        :raises ApplicationLockedError: Bad data in the file.
+        """
         try:
             with open(self._pidfile, "r") as filep:
-                # fcntl.LOCK_EX = Acquire an exclusive lock
-                fcntl.flock(filep, fcntl.LOCK_EX)
-                try:
-                    file_contents = filep.read()
-                    pid = int(file_contents.rstrip())
-                    if pid:
-                        return pid == self._pid
-                finally:
-                    # fcntl.LOCK_UN = Release an existing lock
-                    fcntl.flock(filep, fcntl.LOCK_UN)
+                file_contents = filep.read()
+                pid = int(file_contents.rstrip())
+                if pid:
+                    return pid
+        except ValueError:
+            raise ApplicationLockedError("%s has invalid contents" % (self._pidfile))
         except (IOError, OSError) as exc:
             if exc.errno == errno.ENOENT:
-                pass
-        return False
+                return None
+            raise
+        return None
 
-    @staticmethod
-    def _pid_exists(pid):
-        """Test whether a particular process exists."""
-        try:
-            # Bulletproofing: avoid killing init or all processes.
-            if pid > 1:
-                os.kill(pid, 0)
-        except OSError as exc:
-            # The only other (theoretical) possibility is EPERM, which
-            # would mean the process exists and therefore we should return
-            # True.
-            if exc.errno == errno.ESRCH:
-                return False
-        return True
+    @property
+    def is_locked(self):
+        """Test whether this object was locked by this instance of
+        the application."""
+        pid = self._get_pid_from_file()
+        return pid == self._pid
 
     def _safe_unlink(self):
         """Unlink the lock file. If the unlink fails because the file
@@ -176,30 +164,10 @@ def try_to_lock(self):
         """
         if self._try_create():
             return
-        with open(self._pidfile, "r") as filep:
-            # fcntl.LOCK_EX = Acquire an exclusive lock
-            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.LOCK_UN = Release an existing lock
-                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)
+        pid = self._get_pid_from_file()
+        if pid == self._pid:
+            return
+        raise ApplicationLockedError("%s locked by process %d" % (self._pidfile, pid))
 
     def unlock(self):
         """Release the lock on this application.
diff --git a/convert2rhel/unit_tests/applock_test.py b/convert2rhel/unit_tests/applock_test.py
index 03a9bb8baf..32be5b96d8 100644
--- a/convert2rhel/unit_tests/applock_test.py
+++ b/convert2rhel/unit_tests/applock_test.py
@@ -67,16 +67,6 @@ def test_applock_basic_lock_idempotent(tmp_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."""
-    old_pid = subprocess.check_output("/bin/echo $$", shell=True, universal_newlines=True)
-    with open(tmp_lock._pidfile, "w") as f:
-        f.write(old_pid)
-    tmp_lock.try_to_lock()
-    os.unlink(tmp_lock._pidfile)
-
-
 def test_applock_bogus_lock(tmp_lock):
     """Test the case where the lock file exists, but has bogus data."""
     with open(tmp_lock._pidfile, "w") as f:

From 577ec86ec4c960f17a03c257117881c502c99472 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Thu, 29 Aug 2024 08:43:29 -0400
Subject: [PATCH 15/16] Improve error message when application is locked.

---
 convert2rhel/main.py                 | 7 +++++--
 convert2rhel/unit_tests/main_test.py | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/convert2rhel/main.py b/convert2rhel/main.py
index 8d9fcb5225..390bd59a95 100644
--- a/convert2rhel/main.py
+++ b/convert2rhel/main.py
@@ -117,8 +117,11 @@ def main():
     try:
         with applock.ApplicationLock("convert2rhel"):
             return main_locked()
-    except applock.ApplicationLockedError:
-        loggerinst.warning("Another copy of convert2rhel is running.\n")
+    except applock.ApplicationLockedError as exc:
+        loggerinst.warning(str(exc))
+        loggerinst.warning("Another copy of convert2rhel may be running.\n")
+        loggerinst.warning("If you are certain that no other convert2rhel process\n")
+        loggerinst.warning("is running, remove the lock file and try again\n")
         loggerinst.warning("\nNo changes were made to the system.\n")
         return ConversionExitCodes.FAILURE
 
diff --git a/convert2rhel/unit_tests/main_test.py b/convert2rhel/unit_tests/main_test.py
index 0817742126..d6fb536990 100644
--- a/convert2rhel/unit_tests/main_test.py
+++ b/convert2rhel/unit_tests/main_test.py
@@ -707,5 +707,5 @@ def test_main_already_running_conversion(monkeypatch, caplog, tmpdir):
     monkeypatch.setattr(main, "main_locked", mock.Mock(side_effect=applock.ApplicationLockedError("failed")))
 
     assert main.main() == 1
-    assert "Another copy of convert2rhel is running.\n" in caplog.records[-2].message
+    assert "Another copy of convert2rhel may be running.\n" in caplog.records[-4].message
     assert "\nNo changes were made to the system.\n" in caplog.records[-1].message

From cbedf556d8e3e1ec7298ecfa7cae64b915186c37 Mon Sep 17 00:00:00 2001
From: Joseph Chapman <jochapma@redhat.com>
Date: Thu, 29 Aug 2024 10:46:50 -0400
Subject: [PATCH 16/16] Prettified a warning message.

---
 convert2rhel/main.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/convert2rhel/main.py b/convert2rhel/main.py
index 390bd59a95..37febfcfd8 100644
--- a/convert2rhel/main.py
+++ b/convert2rhel/main.py
@@ -119,9 +119,9 @@ def main():
             return main_locked()
     except applock.ApplicationLockedError as exc:
         loggerinst.warning(str(exc))
-        loggerinst.warning("Another copy of convert2rhel may be running.\n")
-        loggerinst.warning("If you are certain that no other convert2rhel process\n")
-        loggerinst.warning("is running, remove the lock file and try again\n")
+        loggerinst.warning("Another copy of convert2rhel may be running.")
+        loggerinst.warning("If you are certain that no other convert2rhel process")
+        loggerinst.warning("is running, remove the lock file and try again.\n")
         loggerinst.warning("\nNo changes were made to the system.\n")
         return ConversionExitCodes.FAILURE