locks: improve errors and permission checking

- Clean up error messages for when a lock can't be created, or when an
  exclusive (write) lock can't be taken on a file.

- Add a number of subclasses of LockError to distinguish timeouts from
  permission issues.

- Add an explicit check to prevent the user from taking a write lock on a
  read-only file.
  - We had a check for this for when we try to *upgrade* a lock on an RO
    file, but not for an initial write lock attempt.

- Add more tests for different lock permission scenarios.

2 files changed, 145 insertions, 52 deletions

diff --git a/lib/spack/llnl/util/lock.py b/lib/spack/llnl/util/lock.py
index 794b201668..10e552bdb5 100644
--- a/lib/spack/llnl/util/lock.py
+++ b/lib/spack/llnl/util/lock.py
@@ -32,7 +32,8 @@ import llnl.util.tty as tty
 
 
 __all__ = ['Lock', 'LockTransaction', 'WriteTransaction', 'ReadTransaction',
-           'LockError']
+           'LockError', 'LockTimeoutError',
+           'LockPermissionError', 'LockROFileError', 'CantCreateLockError']
 
 
 # Default timeout in seconds, after which locks will raise exceptions.
@@ -80,7 +81,7 @@ class Lock(object):
         self.host = self.old_host = None
 
     def _lock(self, op, timeout=_default_timeout):
-        """This takes a lock using POSIX locks (``fnctl.lockf``).
+        """This takes a lock using POSIX locks (``fcntl.lockf``).
 
         The lock is implemented as a spin lock using a nonblocking call
         to ``lockf()``.
@@ -91,32 +92,36 @@ class Lock(object):
 
         If the lock times out, it raises a ``LockError``.
         """
+        assert op in (fcntl.LOCK_SH, fcntl.LOCK_EX)
+
         start_time = time.time()
         while (time.time() - start_time) < timeout:
-            try:
-                # If we could write the file, we'd have opened it 'r+'.
-                # Raise an error when we attempt to upgrade to a write lock.
-                if op == fcntl.LOCK_EX:
-                    if self._file and self._file.mode == 'r':
-                        raise LockError(
-                            "Can't take exclusive lock on read-only file: %s"
-                            % self.path)
-
-                # Create file and parent directories if they don't exist.
-                if self._file is None:
-                    self._ensure_parent_directory()
-
-                    # Prefer to open 'r+' to allow upgrading to write
-                    # lock later if possible.  Open read-only if we can't
-                    # write the lock file at all.
-                    os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), 'r+'
-                    if os.path.exists(self.path) and not os.access(
-                            self.path, os.W_OK):
-                        os_mode, fd_mode = os.O_RDONLY, 'r'
-
-                    fd = os.open(self.path, os_mode)
-                    self._file = os.fdopen(fd, fd_mode)
+            # Create file and parent directories if they don't exist.
+            if self._file is None:
+                parent = self._ensure_parent_directory()
+
+                # Open writable files as 'r+' so we can upgrade to write later
+                os_mode, fd_mode = (os.O_RDWR | os.O_CREAT), 'r+'
+                if os.path.exists(self.path):
+                    if not os.access(self.path, os.W_OK):
+                        if op == fcntl.LOCK_SH:
+                            # can still lock read-only files if we open 'r'
+                            os_mode, fd_mode = os.O_RDONLY, 'r'
+                        else:
+                            raise LockROFileError(self.path)
+
+                elif not os.access(parent, os.W_OK):
+                    raise CantCreateLockError(self.path)
+
+                fd = os.open(self.path, os_mode)
+                self._file = os.fdopen(fd, fd_mode)
+
+            elif op == fcntl.LOCK_EX and self._file.mode == 'r':
+                # Attempt to upgrade to write lock w/a read-only file.
+                # If the file were writable, we'd have opened it 'r+'
+                raise LockROFileError(self.path)
 
+            try:
                 # Try to get the lock (will raise if not available.)
                 fcntl.lockf(self._file, op | fcntl.LOCK_NB,
                             self._length, self._start, os.SEEK_SET)
@@ -138,20 +143,21 @@ class Lock(object):
                     pass
                 else:
                     raise
+
             time.sleep(_sleep_time)
 
-        raise LockError("Timed out waiting for lock.")
+        raise LockTimeoutError("Timed out waiting for lock.")
 
     def _ensure_parent_directory(self):
         parent = os.path.dirname(self.path)
         try:
             os.makedirs(parent)
-            return True
         except OSError as e:
             # makedirs can fail when diretory already exists.
             if not (e.errno == errno.EEXIST and os.path.isdir(parent) or
                     e.errno == errno.EISDIR):
                 raise
+        return parent
 
     def _read_debug_data(self):
         """Read PID and host data out of the file if it is there."""
@@ -340,7 +346,7 @@ class LockTransaction(object):
 
 
 class ReadTransaction(LockTransaction):
-
+    """LockTransaction context manager that does a read and releases it."""
     def _enter(self):
         return self._lock.acquire_read(self._timeout)
 
@@ -349,7 +355,7 @@ class ReadTransaction(LockTransaction):
 
 
 class WriteTransaction(LockTransaction):
-
+    """LockTransaction context manager that does a write and releases it."""
     def _enter(self):
         return self._lock.acquire_write(self._timeout)
 
@@ -358,4 +364,27 @@ class WriteTransaction(LockTransaction):
 
 
 class LockError(Exception):
+    """Raised for any errors related to locks."""
+
+
+class LockTimeoutError(LockError):
     """Raised when an attempt to acquire a lock times out."""
+
+
+class LockPermissionError(LockError):
+    """Raised when there are permission issues with a lock."""
+
+
+class LockROFileError(LockPermissionError):
+    """Tried to take an exclusive lock on a read-only file."""
+    def __init__(self, path):
+        msg = "Can't take write lock on read-only file: %s" % path
+        super(LockROFileError, self).__init__(msg)
+
+
+class CantCreateLockError(LockPermissionError):
+    """Attempt to create a lock in an unwritable location."""
+    def __init__(self, path):
+        msg = "cannot create lock '%s': " % path
+        msg += "file does not exist and location is not writable"
+        super(LockError, self).__init__(msg)
diff --git a/lib/spack/spack/test/llnl/util/lock.py b/lib/spack/spack/test/llnl/util/lock.py
index 5127462c58..7401ecdc2d 100644
--- a/lib/spack/spack/test/llnl/util/lock.py
+++ b/lib/spack/spack/test/llnl/util/lock.py
@@ -128,12 +128,27 @@ This may need to be higher for some filesystems."""
 lock_fail_timeout = 0.1
 
 
+def make_readable(*paths):
+    for path in paths:
+        mode = 0o555 if os.path.isdir(path) else 0o444
+        os.chmod(path, mode)
+
+
+def make_writable(*paths):
+    for path in paths:
+        mode = 0o755 if os.path.isdir(path) else 0o744
+        os.chmod(path, mode)
+
+
 @contextmanager
-def read_only(path):
-    orginal_mode = os.stat(path).st_mode
-    os.chmod(path, 0o444)
+def read_only(*paths):
+    modes = [os.stat(p).st_mode for p in paths]
+    make_readable(*paths)
+
     yield
-    os.chmod(path, orginal_mode)
+
+    for path, mode in zip(paths, modes):
+        os.chmod(path, mode)
 
 
 @pytest.fixture(scope='session', params=locations)
@@ -172,6 +187,7 @@ def lock_dir(lock_test_directory):
         comm.barrier()
 
     if not mpi or comm.rank == 0:
+        make_writable(tempdir)
         shutil.rmtree(tempdir)
 
 
@@ -188,6 +204,7 @@ def private_lock_path(lock_dir):
     yield lock_file
 
     if os.path.exists(lock_file):
+        make_writable(lock_dir, lock_file)
         os.unlink(lock_file)
 
 
@@ -199,6 +216,7 @@ def lock_path(lock_dir):
     yield lock_file
 
     if os.path.exists(lock_file):
+        make_writable(lock_dir, lock_file)
         os.unlink(lock_file)
 
 
@@ -290,7 +308,7 @@ def timeout_write(lock_path, start=0, length=0):
     def fn(barrier):
         lock = lk.Lock(lock_path, start, length)
         barrier.wait()  # wait for lock acquire in first process
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
         barrier.wait()
     return fn
@@ -300,7 +318,7 @@ def timeout_read(lock_path, start=0, length=0):
     def fn(barrier):
         lock = lk.Lock(lock_path, start, length)
         barrier.wait()  # wait for lock acquire in first process
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()
     return fn
@@ -540,9 +558,47 @@ def test_write_lock_timeout_with_multiple_readers_3_2_ranges(lock_path):
         timeout_write(lock_path, 5, 1))
 
 
-#
-# Test that read can be upgraded to write.
-#
+def test_read_lock_on_read_only_lockfile(lock_dir, lock_path):
+    """read-only directory, read-only lockfile."""
+    touch(lock_path)
+    with read_only(lock_path, lock_dir):
+        lock = lk.Lock(lock_path)
+
+        with lk.ReadTransaction(lock):
+            pass
+
+        with pytest.raises(lk.LockROFileError):
+            with lk.WriteTransaction(lock):
+                pass
+
+
+def test_read_lock_read_only_dir_writable_lockfile(lock_dir, lock_path):
+    """read-only directory, writable lockfile."""
+    touch(lock_path)
+    with read_only(lock_dir):
+        lock = lk.Lock(lock_path)
+
+        with lk.ReadTransaction(lock):
+            pass
+
+        with lk.WriteTransaction(lock):
+            pass
+
+
+def test_read_lock_no_lockfile(lock_dir, lock_path):
+    """read-only directory, no lockfile (so can't create)."""
+    with read_only(lock_dir):
+        lock = lk.Lock(lock_path)
+
+        with pytest.raises(lk.CantCreateLockError):
+            with lk.ReadTransaction(lock):
+                pass
+
+        with pytest.raises(lk.CantCreateLockError):
+            with lk.WriteTransaction(lock):
+                pass
+
+
 def test_upgrade_read_to_write(private_lock_path):
     """Test that a read lock can be upgraded to a write lock.
 
@@ -597,7 +653,7 @@ def test_upgrade_read_to_write_fails_with_readonly_file(private_lock_path):
         assert lock._file.mode == 'r'
 
         # upgrade to writ here
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockROFileError):
             lock.acquire_write()
 
 
@@ -615,7 +671,7 @@ def test_complex_acquire_and_release_chain(lock_path):
         barrier.wait()  # ---------------------------------------- 2
         lock.release_write()   # release and others acquire read
         barrier.wait()  # ---------------------------------------- 3
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
         lock.acquire_read()
         barrier.wait()  # ---------------------------------------- 4
@@ -624,9 +680,9 @@ def test_complex_acquire_and_release_chain(lock_path):
 
         # p2 upgrades read to write
         barrier.wait()  # ---------------------------------------- 6
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 7
         # p2 releases write and read
@@ -636,9 +692,9 @@ def test_complex_acquire_and_release_chain(lock_path):
         barrier.wait()  # ---------------------------------------- 9
         # p3 upgrades read to write
         barrier.wait()  # ---------------------------------------- 10
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 11
         # p3 releases locks
@@ -652,9 +708,9 @@ def test_complex_acquire_and_release_chain(lock_path):
 
         # p1 acquires write
         barrier.wait()  # ---------------------------------------- 1
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 2
         lock.acquire_read()
@@ -676,9 +732,9 @@ def test_complex_acquire_and_release_chain(lock_path):
         barrier.wait()  # ---------------------------------------- 9
         # p3 upgrades read to write
         barrier.wait()  # ---------------------------------------- 10
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 11
         # p3 releases locks
@@ -692,9 +748,9 @@ def test_complex_acquire_and_release_chain(lock_path):
 
         # p1 acquires write
         barrier.wait()  # ---------------------------------------- 1
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 2
         lock.acquire_read()
@@ -706,9 +762,9 @@ def test_complex_acquire_and_release_chain(lock_path):
 
         # p2 upgrades read to write
         barrier.wait()  # ---------------------------------------- 6
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_write(lock_fail_timeout)
-        with pytest.raises(lk.LockError):
+        with pytest.raises(lk.LockTimeoutError):
             lock.acquire_read(lock_fail_timeout)
         barrier.wait()  # ---------------------------------------- 7
         # p2 releases write & read
@@ -983,3 +1039,11 @@ def test_lock_debug_output(lock_path):
 
     q1, q2 = Queue(), Queue()
     local_multiproc_test(p2, p1, extra_args=(q1, q2))
+
+
+def test_lock_with_no_parent_directory(tmpdir):
+    """Make sure locks work even when their parent directory does not exist."""
+    with tmpdir.as_cwd():
+        lock = lk.Lock('foo/bar/baz/lockfile')
+        with lk.WriteTransaction(lock):
+            pass