From b908a65649cbdd0ed5d09d11fb635c36b5f35bc1 Mon Sep 17 00:00:00 2001
From: Michael Barton <mike@weirdlooking.com>
Date: Mon, 21 Jul 2014 07:45:56 +0000
Subject: [PATCH] lock_file race fixes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I attempted to use this function and found a few problems.

We shouldn’t unlink the file after closing it, because someone else could lock
it in between.  Switch to unlink before close.

If someone else locked the file between our open and flock, they are likely to
unlink it out from underneath us.  Then we have a lock on a file that no longer
exists.  So stat the filename after locking to make sure the inode hasn't
changed or gone away.

We probably shouldn’t unlink the file if we time out waiting for a lock.  So
move that to before the finally block.

Change-Id: Id1858c97805d3ab81c584eaee8ce0d43d34a8089
---
 swift/common/utils.py          | 42 +++++++++++---------
 test/unit/common/test_utils.py | 71 ++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 18 deletions(-)

diff --git a/swift/common/utils.py b/swift/common/utils.py
index 218ccc2b15..1281dc2888 100644
--- a/swift/common/utils.py
+++ b/swift/common/utils.py
@@ -1629,26 +1629,32 @@ def lock_file(filename, timeout=10, append=False, unlink=True):
         mode = 'a+'
     else:
         mode = 'r+'
-    fd = os.open(filename, flags)
-    file_obj = os.fdopen(fd, mode)
-    try:
-        with swift.common.exceptions.LockTimeout(timeout, filename):
-            while True:
-                try:
-                    fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-                    break
-                except IOError as err:
-                    if err.errno != errno.EAGAIN:
-                        raise
-                sleep(0.01)
-        yield file_obj
-    finally:
+    while True:
+        fd = os.open(filename, flags)
+        file_obj = os.fdopen(fd, mode)
         try:
+            with swift.common.exceptions.LockTimeout(timeout, filename):
+                while True:
+                    try:
+                        fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                        break
+                    except IOError as err:
+                        if err.errno != errno.EAGAIN:
+                            raise
+                    sleep(0.01)
+            try:
+                if os.stat(filename).st_ino != os.fstat(fd).st_ino:
+                    continue
+            except OSError as err:
+                if err.errno == errno.ENOENT:
+                    continue
+                raise
+            yield file_obj
+            if unlink:
+                os.unlink(filename)
+            break
+        finally:
             file_obj.close()
-        except UnboundLocalError:
-            pass  # may have not actually opened the file
-        if unlink:
-            os.unlink(filename)
 
 
 def lock_parent_directory(filename, timeout=10):
diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py
index 899fcc0169..a7733f8c25 100644
--- a/test/unit/common/test_utils.py
+++ b/test/unit/common/test_utils.py
@@ -2351,6 +2351,77 @@ cluster_dfw1 = http://dfw1.host/v1/
 
             self.assertRaises(OSError, os.remove, nt.name)
 
+    def test_lock_file_unlinked_after_open(self):
+        os_open = os.open
+        first_pass = [True]
+
+        def deleting_open(filename, flags):
+            # unlink the file after it's opened.  once.
+            fd = os_open(filename, flags)
+            if first_pass[0]:
+                os.unlink(filename)
+                first_pass[0] = False
+            return fd
+
+        with NamedTemporaryFile(delete=False) as nt:
+            with mock.patch('os.open', deleting_open):
+                with utils.lock_file(nt.name, unlink=True) as f:
+                    self.assertNotEqual(os.fstat(nt.fileno()).st_ino,
+                                        os.fstat(f.fileno()).st_ino)
+        first_pass = [True]
+
+        def recreating_open(filename, flags):
+            # unlink and recreate the file after it's opened
+            fd = os_open(filename, flags)
+            if first_pass[0]:
+                os.unlink(filename)
+                os.close(os_open(filename, os.O_CREAT | os.O_RDWR))
+                first_pass[0] = False
+            return fd
+
+        with NamedTemporaryFile(delete=False) as nt:
+            with mock.patch('os.open', recreating_open):
+                with utils.lock_file(nt.name, unlink=True) as f:
+                    self.assertNotEqual(os.fstat(nt.fileno()).st_ino,
+                                        os.fstat(f.fileno()).st_ino)
+
+    def test_lock_file_held_on_unlink(self):
+        os_unlink = os.unlink
+
+        def flocking_unlink(filename):
+            # make sure the lock is held when we unlink
+            fd = os.open(filename, os.O_RDWR)
+            self.assertRaises(
+                IOError, fcntl.flock, fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+            os.close(fd)
+            os_unlink(filename)
+
+        with NamedTemporaryFile(delete=False) as nt:
+            with mock.patch('os.unlink', flocking_unlink):
+                with utils.lock_file(nt.name, unlink=True):
+                    pass
+
+    def test_lock_file_no_unlink_if_fail(self):
+        os_open = os.open
+        with NamedTemporaryFile(delete=True) as nt:
+
+            def lock_on_open(filename, flags):
+                # lock the file on another fd after it's opened.
+                fd = os_open(filename, flags)
+                fd2 = os_open(filename, flags)
+                fcntl.flock(fd2, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                return fd
+
+            try:
+                timedout = False
+                with mock.patch('os.open', lock_on_open):
+                    with utils.lock_file(nt.name, unlink=False, timeout=0.01):
+                        pass
+            except LockTimeout:
+                timedout = True
+            self.assert_(timedout)
+            self.assert_(os.path.exists(nt.name))
+
     def test_ismount_path_does_not_exist(self):
         tmpdir = mkdtemp()
         try: