Bugfix: stage directory permissions and cleaning (#12733)

* This updates stage names to use "spack-stage-" as a prefix.
  This avoids removing non-Spack directories in "spack clean" as
  c141e99 did (in this case so long as they don't contain the
  prefix "spack-stage-"), and also addresses a follow-up issue
  where Spack stage directories were not removed.
* Spack now does more-stringent checking of expected permissions for
  staging directories. For a given stage root that includes a user
  component, all directories before the user component that are
  created by Spack are expected to match the permissions of their
  parent; the user component and all deeper directories are expected
  to be accessible to the user (read/write/execute).

lib/spack/docs/config_yaml.rst

@@ -91,9 +91,10 @@ the selected ``build_stage`` path.
 .. warning:: We highly recommend specifying ``build_stage`` paths that
    distinguish between staging and other activities to ensure 
    ``spack clean`` does not inadvertently remove unrelated files.
-   This can be accomplished by using a combination of ``spack`` and or
+   Spack prepends ``spack-stage-`` to temporary staging directory names to
-   ``stage`` in each path as shown in the default settings and documented
+   reduce this risk.  Using a combination of ``spack`` and or ``stage`` in
-   examples.
+   each specified path, as shown in the default settings and documented
+   examples, will add another layer of protection.
 
 By default, Spack's ``build_stage`` is configured like this:
 

lib/spack/docs/tutorial_configuration.rst

@@ -856,10 +856,11 @@ from this file system with the following ``config.yaml``:
 
    It is important to distinguish the build stage directory from other
    directories in your scratch space to ensure ``spack clean`` does not
-   inadvertently remove unrelated files.  This can be accomplished by
+   inadvertently remove unrelated files.  Spack prepends ``spack-stage-``
-   including a combination of ``spack`` and or ``stage`` in each path
+   to temporary staging directory names to reduce this risk.  Using a
-   as shown in the default settings and documented examples.  See
+   combination of ``spack`` and or ``stage`` in each specified path, as
-   :ref:`config-yaml` for details.
+   shown in the default settings and documented examples, will add
+   another layer of protection.  See :ref:`config-yaml` for details.
 
 
 On systems with compilers that absolutely *require* environment variables

lib/spack/llnl/util/filesystem.py

@@ -49,6 +49,8 @@
     'is_exe',
     'join_path',
     'mkdirp',
+    'partition_path',
+    'prefixes',
     'remove_dead_links',
     'remove_if_dead_link',
     'remove_linked_tree',
@@ -1608,3 +1610,70 @@ def search_paths_for_executables(*path_hints):
             executable_paths.append(bin_dir)
 
     return executable_paths
+
+
+def partition_path(path, entry=None):
+    """
+    Split the prefixes of the path at the first occurrence of entry and
+    return a 3-tuple containing a list of the prefixes before the entry, a
+    string of the prefix ending with the entry, and a list of the prefixes
+    after the entry.
+
+    If the entry is not a node in the path, the result will be the prefix list
+    followed by an empty string and an empty list.
+    """
+    paths = prefixes(path)
+
+    if entry is not None:
+        # Derive the index of entry within paths, which will correspond to
+        # the location of the entry in within the path.
+        try:
+            entries = path.split(os.sep)
+            i = entries.index(entry)
+            if '' in entries:
+                i -= 1
+            return paths[:i], paths[i], paths[i + 1:]
+        except ValueError:
+            pass
+
+    return paths, '', []
+
+
+def prefixes(path):
+    """
+    Returns a list containing the path and its ancestors, top-to-bottom.
+
+    The list for an absolute path will not include an ``os.sep`` entry.
+    For example, assuming ``os.sep`` is ``/``, given path ``/ab/cd/efg``
+    the resulting paths will be, in order: ``/ab``, ``/ab/cd``, and
+    ``/ab/cd/efg``
+
+    The list for a relative path starting ``./`` will not include ``.``.
+    For example, path ``./hi/jkl/mn`` results in a list with the following
+    paths, in order: ``./hi``, ``./hi/jkl``, and ``./hi/jkl/mn``.
+
+    Parameters:
+        path (str): the string used to derive ancestor paths
+
+    Returns:
+        A list containing ancestor paths in order and ending with the path
+    """
+    if not path:
+        return []
+
+    parts = path.strip(os.sep).split(os.sep)
+    if path.startswith(os.sep):
+        parts.insert(0, os.sep)
+    paths = [os.path.join(*parts[:i + 1]) for i in range(len(parts))]
+
+    try:
+        paths.remove(os.sep)
+    except ValueError:
+        pass
+
+    try:
+        paths.remove('.')
+    except ValueError:
+        pass
+
+    return paths

lib/spack/spack/package.py

@@ -56,7 +56,7 @@
 from llnl.util.tty.color import colorize
 from spack.filesystem_view import YamlFilesystemView
 from spack.util.executable import which
-from spack.stage import Stage, ResourceStage, StageComposite
+from spack.stage import stage_prefix, Stage, ResourceStage, StageComposite
 from spack.util.environment import dump_environment
 from spack.util.package_hash import package_hash
 from spack.version import Version
@@ -753,7 +753,8 @@ def _make_root_stage(self, fetcher):
         mp = spack.mirror.mirror_archive_path(self.spec, fetcher)
         # Construct a path where the stage should build..
         s = self.spec
-        stage_name = "%s-%s-%s" % (s.name, s.version, s.dag_hash())
+        stage_name = "{0}{1}-{2}-{3}".format(stage_prefix, s.name, s.version,
+                                             s.dag_hash())
 
         def download_search():
             dynamic_fetcher = fs.from_list_url(self)

lib/spack/spack/stage.py

@@ -4,7 +4,6 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import os
-import re
 import stat
 import sys
 import errno
@@ -17,7 +16,7 @@
 
 import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp, can_access, install, install_tree
-from llnl.util.filesystem import remove_linked_tree
+from llnl.util.filesystem import partition_path, remove_linked_tree
 
 import spack.paths
 import spack.caches
@@ -34,40 +33,63 @@
 _source_path_subdir = 'spack-src'
 
 # The temporary stage name prefix.
-_stage_prefix = 'spack-stage-'
+stage_prefix = 'spack-stage-'
 
 
 def _create_stage_root(path):
     """Create the stage root directory and ensure appropriate access perms."""
     assert path.startswith(os.path.sep) and len(path.strip()) > 1
 
-    curr_dir = os.path.sep
+    err_msg = 'Cannot create stage root {0}: Access to {1} is denied'
-    parts = path.strip(os.path.sep).split(os.path.sep)
-    for i, part in enumerate(parts):
-        curr_dir = os.path.join(curr_dir, part)
-        if not os.path.exists(curr_dir):
-            rest = os.path.join(os.path.sep, *parts[i + 1:])
-            user_parts = rest.partition(os.path.sep + getpass.getuser())
-            if len(user_parts[0]) > 0:
-                # Ensure access controls of subdirs created above `$user`
-                # inherit from the parent and share the group.
-                stats = os.stat(os.path.dirname(curr_dir))
-                curr_dir = ''.join([curr_dir, user_parts[0]])
-                mkdirp(curr_dir, group=stats.st_gid, mode=stats.st_mode)
 
-            user_subdirs = ''.join(user_parts[1:])
+    user_uid = os.getuid()
-            if len(user_subdirs) > 0:
-                # Ensure access controls of subdirs from `$user` on down are
-                # restricted to the user.
-                curr_dir = ''.join([curr_dir, user_subdirs])
-                mkdirp(curr_dir, mode=stat.S_IRWXU)
 
-            assert os.getuid() == os.stat(curr_dir).st_uid
+    # Obtain lists of ancestor and descendant paths of the $user node, if any.
-            break
+    group_paths, user_node, user_paths = partition_path(path,
+                                                        getpass.getuser())
 
-    if not can_access(path):
+    for p in group_paths:
-        raise OSError(errno.EACCES,
+        if not os.path.exists(p):
-                      'Cannot access %s: Permission denied' % curr_dir)
+            # Ensure access controls of subdirs created above `$user` inherit
+            # from the parent and share the group.
+            par_stat = os.stat(os.path.dirname(p))
+            mkdirp(p, group=par_stat.st_gid, mode=par_stat.st_mode)
+
+            p_stat = os.stat(p)
+            if par_stat.st_gid != p_stat.st_gid:
+                tty.warn("Expected {0} to have group {1}, but it is {2}"
+                         .format(p, par_stat.st_gid, p_stat.st_gid))
+
+            if par_stat.st_mode & p_stat.st_mode != par_stat.st_mode:
+                tty.warn("Expected {0} to support mode {1}, but it is {2}"
+                         .format(p, par_stat.st_mode, p_stat.st_mode))
+
+            if not can_access(p):
+                raise OSError(errno.EACCES, err_msg.format(path, p))
+
+    # Add the path ending with the $user node to the user paths to ensure paths
+    # from $user (on down) meet the ownership and permission requirements.
+    if user_node:
+        user_paths.insert(0, user_node)
+
+    for p in user_paths:
+        # Ensure access controls of subdirs from `$user` on down are
+        # restricted to the user.
+        if not os.path.exists(p):
+            mkdirp(p, mode=stat.S_IRWXU)
+
+            p_stat = os.stat(p)
+            if p_stat.st_mode & stat.S_IRWXU != stat.S_IRWXU:
+                tty.error("Expected {0} to support mode {1}, but it is {2}"
+                          .format(p, stat.S_IRWXU, p_stat.st_mode))
+
+                raise OSError(errno.EACCES, err_msg.format(path, p))
+        else:
+            p_stat = os.stat(p)
+
+        if user_uid != p_stat.st_uid:
+            tty.warn("Expected user {0} to own {1}, but it is owned by {2}"
+                     .format(user_uid, p, p_stat.st_uid))
 
 
 def _first_accessible_path(paths):
@@ -106,7 +128,7 @@ def _resolve_paths(candidates):
         # Remove the extra `$user` node from a `$tempdir/$user` entry for
         # hosts that automatically append `$user` to `$tempdir`.
         if path.startswith(os.path.join('$tempdir', '$user')) and tmp_has_usr:
-            path = os.path.join('$tempdir', path[15:])
+            path = path.replace("/$user", "", 1)
 
         # Ensure the path is unique per user.
         can_path = sup.canonicalize_path(path)
@@ -250,7 +272,7 @@ def __init__(
         # TODO: temporary stage area in _stage_root.
         self.name = name
         if name is None:
-            self.name = _stage_prefix + next(tempfile._get_candidate_names())
+            self.name = stage_prefix + next(tempfile._get_candidate_names())
         self.mirror_path = mirror_path
 
         # Use the provided path or construct an optionally named stage path.
@@ -680,11 +702,8 @@ def purge():
     """Remove all build directories in the top-level stage path."""
     root = get_stage_root()
     if os.path.isdir(root):
-        # TODO: Figure out a "standard" way to identify the hash length
-        # TODO: Should this support alternate base represenations?
-        dir_expr = re.compile(r'.*-[0-9a-f]{32}$')
         for stage_dir in os.listdir(root):
-            if re.match(dir_expr, stage_dir) or stage_dir == '.lock':
+            if stage_dir.startswith(stage_prefix) or stage_dir == '.lock':
                 stage_path = os.path.join(root, stage_dir)
                 remove_linked_tree(stage_path)
 

lib/spack/spack/test/cmd/env.py

@@ -17,6 +17,7 @@
 from spack.cmd.env import _env_create
 from spack.spec import Spec
 from spack.main import SpackCommand
+from spack.stage import stage_prefix
 
 from spack.spec_list import SpecListError
 from spack.test.conftest import MockPackage, MockPackageMultiRepo
@@ -576,7 +577,8 @@ def test_stage(mock_stage, mock_fetch, install_mockery):
     def check_stage(spec):
         spec = Spec(spec).concretized()
         for dep in spec.traverse():
-            stage_name = "%s-%s-%s" % (dep.name, dep.version, dep.dag_hash())
+            stage_name = "{0}{1}-{2}-{3}".format(stage_prefix, dep.name,
+                                                 dep.version, dep.dag_hash())
             assert os.path.isdir(os.path.join(root, stage_name))
 
     check_stage('mpileaks')

lib/spack/spack/test/conftest.py

@@ -5,6 +5,7 @@
 
 import collections
 import copy
+import errno
 import inspect
 import itertools
 import os
@@ -41,6 +42,14 @@
 from spack.version import Version
 
 
+@pytest.fixture
+def no_path_access(monkeypatch):
+    def _can_access(path, perms):
+        return False
+
+    monkeypatch.setattr(os, 'access', _can_access)
+
+
 #
 # Disable any activate Spack environment BEFORE all tests
 #
@@ -184,23 +193,29 @@ def working_env():
 
 @pytest.fixture(scope='function', autouse=True)
 def check_for_leftover_stage_files(request, mock_stage, ignore_stage_files):
-    """Ensure that each test leaves a clean stage when done.
+    """
+    Ensure that each (mock_stage) test leaves a clean stage when done.
 
-    This can be disabled for tests that are expected to dirty the stage
+    Tests that are expected to dirty the stage can disable the check by
-    by adding::
+    adding::
 
         @pytest.mark.disable_clean_stage_check
 
-    to tests that need it.
+    and the associated stage files will be removed.
     """
     stage_path = mock_stage
 
     yield
 
     files_in_stage = set()
-    if os.path.exists(stage_path):
+    try:
         stage_files = os.listdir(stage_path)
         files_in_stage = set(stage_files) - ignore_stage_files
+    except OSError as err:
+        if err.errno == errno.ENOENT:
+            pass
+        else:
+            raise
 
     if 'disable_clean_stage_check' in request.keywords:
         # clean up after tests that are expected to be dirty

lib/spack/spack/test/llnl/util/filesystem.py

@@ -312,6 +312,30 @@ def test_headers_directory_setter():
     assert hl.directories == []
 
 
+@pytest.mark.parametrize('path,entry,expected', [
+    ('/tmp/user/root', None,
+     (['/tmp', '/tmp/user', '/tmp/user/root'], '', [])),
+    ('/tmp/user/root', 'tmp', ([], '/tmp', ['/tmp/user', '/tmp/user/root'])),
+    ('/tmp/user/root', 'user', (['/tmp'], '/tmp/user', ['/tmp/user/root'])),
+    ('/tmp/user/root', 'root', (['/tmp', '/tmp/user'], '/tmp/user/root', [])),
+    ('relative/path', None, (['relative', 'relative/path'], '', [])),
+    ('relative/path', 'relative', ([], 'relative', ['relative/path'])),
+    ('relative/path', 'path', (['relative'], 'relative/path', []))
+])
+def test_partition_path(path, entry, expected):
+    assert fs.partition_path(path, entry) == expected
+
+
+@pytest.mark.parametrize('path,expected', [
+    ('', []),
+    ('/tmp/user/dir', ['/tmp', '/tmp/user', '/tmp/user/dir']),
+    ('./some/sub/dir', ['./some', './some/sub', './some/sub/dir']),
+    ('another/sub/dir', ['another', 'another/sub', 'another/sub/dir'])
+])
+def test_prefixes(path, expected):
+    assert fs.prefixes(path) == expected
+
+
 @pytest.mark.regression('7358')
 @pytest.mark.parametrize('regex,replacement,filename,keyword_args', [
     (r"\<malloc\.h\>", "<stdlib.h>", 'x86_cpuid_info.c', {}),

lib/spack/spack/test/stage.py

@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 """Test that the Stage class works correctly."""
+import errno
 import os
 import collections
 import shutil
@@ -13,7 +14,7 @@
 
 import pytest
 
-from llnl.util.filesystem import mkdirp, touch, working_dir
+from llnl.util.filesystem import mkdirp, partition_path, touch, working_dir
 
 import spack.paths
 import spack.stage
@@ -354,23 +355,34 @@ def check_stage_dir_perms(prefix, path):
     # Ensure the path's subdirectories -- to `$user` -- have their parent's
     # perms while those from `$user` on are owned and restricted to the
     # user.
-    status = os.stat(prefix)
+    assert path.startswith(prefix)
-    gid = status.st_gid
+
-    uid = os.getuid()
     user = getpass.getuser()
-    parts = path[len(prefix) + 1:].split(os.path.sep)
+    prefix_status = os.stat(prefix)
-    have_user = False
+    uid = os.getuid()
-    for part in parts:
+
-        if part == user:
+    # Obtain lists of ancestor and descendant paths of the $user node, if any.
-            have_user = True
+    #
-        prefix = os.path.join(prefix, part)
+    # Skip processing prefix ancestors since no guarantee they will be in the
-        prefix_status = os.stat(prefix)
+    # required group (e.g. $TEMPDIR on HPC machines).
-        if not have_user:
+    skip = prefix if prefix.endswith(os.sep) else prefix + os.sep
-            assert gid == prefix_status.st_gid
+    group_paths, user_node, user_paths = partition_path(path.replace(skip, ""),
-            assert status.st_mode == prefix_status.st_mode
+                                                        user)
-        else:
+
-            assert uid == status.st_uid
+    for p in group_paths:
-            assert status.st_mode & stat.S_IRWXU == stat.S_IRWXU
+        p_status = os.stat(os.path.join(prefix, p))
+        assert p_status.st_gid == prefix_status.st_gid
+        assert p_status.st_mode == prefix_status.st_mode
+
+    # Add the path ending with the $user node to the user paths to ensure paths
+    # from $user (on down) meet the ownership and permission requirements.
+    if user_node:
+        user_paths.insert(0, user_node)
+
+    for p in user_paths:
+        p_status = os.stat(os.path.join(prefix, p))
+        assert uid == p_status.st_uid
+        assert p_status.st_mode & stat.S_IRWXU == stat.S_IRWXU
 
 
 @pytest.mark.usefixtures('mock_packages')
@@ -643,7 +655,7 @@ def test_source_path_available(self, mock_stage_archive):
 
     def test_first_accessible_path(self, tmpdir):
         """Test _first_accessible_path names."""
-        spack_dir = tmpdir.join('spack-test-fap')
+        spack_dir = tmpdir.join('paths')
         name = str(spack_dir)
         files = [os.path.join(os.path.sep, 'no', 'such', 'path'), name]
 
@@ -671,9 +683,75 @@ def test_first_accessible_path(self, tmpdir):
         # Cleanup
         shutil.rmtree(str(name))
 
+    def test_create_stage_root(self, tmpdir, no_path_access):
+        """Test _create_stage_root permissions."""
+        test_dir = tmpdir.join('path')
+        test_path = str(test_dir)
+
+        try:
+            if getpass.getuser() in str(test_path).split(os.sep):
+                # Simply ensure directory created if tmpdir includes user
+                spack.stage._create_stage_root(test_path)
+                assert os.path.exists(test_path)
+
+                p_stat = os.stat(test_path)
+                assert p_stat.st_mode & stat.S_IRWXU == stat.S_IRWXU
+            else:
+                # Ensure an OS Error is raised on created, non-user directory
+                with pytest.raises(OSError) as exc_info:
+                    spack.stage._create_stage_root(test_path)
+
+                assert exc_info.value.errno == errno.EACCES
+        finally:
+            try:
+                shutil.rmtree(test_path)
+            except OSError:
+                pass
+
+    @pytest.mark.nomockstage
+    def test_create_stage_root_bad_uid(self, tmpdir, monkeypatch):
+        """
+        Test the code path that uses an existing user path -- whether `$user`
+        in `$tempdir` or not -- and triggers the generation of the UID
+        mismatch warning.
+
+        This situation can happen with some `config:build_stage` settings
+        for teams using a common service account for installing software.
+        """
+        orig_stat = os.stat
+
+        class MinStat:
+            st_mode = -1
+            st_uid = -1
+
+        def _stat(path):
+            p_stat = orig_stat(path)
+
+            fake_stat = MinStat()
+            fake_stat.st_mode = p_stat.st_mode
+            return fake_stat
+
+        user_dir = tmpdir.join(getpass.getuser())
+        user_dir.ensure(dir=True)
+        user_path = str(user_dir)
+
+        # TODO: If we could guarantee access to the monkeypatch context
+        # function (i.e., 3.6.0 on), the call and assertion could be moved
+        # to a with block, such as:
+        #
+        #  with monkeypatch.context() as m:
+        #      m.setattr(os, 'stat', _stat)
+        #      spack.stage._create_stage_root(user_path)
+        #      assert os.stat(user_path).st_uid != os.getuid()
+        monkeypatch.setattr(os, 'stat', _stat)
+        spack.stage._create_stage_root(user_path)
+
+        # The following check depends on the patched os.stat as a poor
+        # substitute for confirming the generated warnings.
+        assert os.stat(user_path).st_uid != os.getuid()
+
     def test_resolve_paths(self):
         """Test _resolve_paths."""
-
         assert spack.stage._resolve_paths([]) == []
 
         # resolved path without user appends user
@@ -686,19 +764,24 @@ def test_resolve_paths(self):
         paths = [os.path.join(os.path.sep, 'spack-{0}'.format(user), 'stage')]
         assert spack.stage._resolve_paths(paths) == paths
 
-        # resolve paths where user
+        tempdir = '$tempdir'
-        tmp = '$tempdir'
+        can_tempdir = canonicalize_path(tempdir)
-        can_tmpdir = canonicalize_path(tmp)
+        user = getpass.getuser()
-        temp_has_user = user in can_tmpdir.split(os.sep)
+        temp_has_user = user in can_tempdir.split(os.sep)
-        paths = [os.path.join(tmp, 'stage'), os.path.join(tmp, '$user')]
+        paths = [os.path.join(tempdir, 'stage'),
-        can_paths = [canonicalize_path(p) for p in paths]
+                 os.path.join(tempdir, '$user'),
+                 os.path.join(tempdir, '$user', '$user'),
+                 os.path.join(tempdir, '$user', 'stage', '$user')]
 
+        res_paths = [canonicalize_path(p) for p in paths]
         if temp_has_user:
-            can_paths[1] = can_tmpdir
+            res_paths[1] = can_tempdir
+            res_paths[2] = os.path.join(can_tempdir, user)
+            res_paths[3] = os.path.join(can_tempdir, 'stage', user)
         else:
-            can_paths[0] = os.path.join(can_paths[0], user)
+            res_paths[0] = os.path.join(res_paths[0], user)
 
-        assert spack.stage._resolve_paths(paths) == can_paths
+        assert spack.stage._resolve_paths(paths) == res_paths
 
     def test_get_stage_root_bad_path(self, clear_stage_root):
         """Ensure an invalid stage path root raises a StageError."""
@@ -711,9 +794,9 @@ def test_get_stage_root_bad_path(self, clear_stage_root):
         assert spack.stage._stage_root is None
 
     @pytest.mark.parametrize(
-        'path,purged', [('stage-1234567890abcdef1234567890abcdef', True),
+        'path,purged', [('spack-stage-1234567890abcdef1234567890abcdef', True),
-                        ('stage-abcdef12345678900987654321fedcba', True),
+                        ('spack-stage-anything-goes-here', True),
-                        ('stage-a1b2c3', False)])
+                        ('stage-spack', False)])
     def test_stage_purge(self, tmpdir, clear_stage_root, path, purged):
         """Test purging of stage directories."""
         stage_dir = tmpdir.join('stage')
@@ -737,7 +820,6 @@ def test_stage_purge(self, tmpdir, clear_stage_root, path, purged):
 
     def test_get_stage_root_in_spack(self, clear_stage_root):
         """Ensure an instance path is an accessible build stage path."""
-
         base = canonicalize_path(os.path.join('$spack', '.spack-test-stage'))
         mkdirp(base)
         test_path = tempfile.mkdtemp(dir=base)