Ensure that artifact file permissions are set in the right order81-non-empty-read-only-directories-not-handled-during-bst-build-and-others

4 files changed, 60 insertions, 13 deletions

diff --git a/buildstream/_ostree.py b/buildstream/_ostree.py
index 40bfda644..89d59b342 100644
--- a/buildstream/_ostree.py
+++ b/buildstream/_ostree.py
@@ -26,7 +26,7 @@ import os
 import subprocess
 from . import _site
 from . import utils
-from .exceptions import _BstError
+from .exceptions import _BstError, _ArtifactError, _ArtifactErrorReason
 
 import gi
 gi.require_version('OSTree', '1.0')
@@ -153,6 +153,13 @@ def commit(repo, dir, ref, branch=None):
 
         # complete repo transaction
         repo.commit_transaction(None)
+    except GLib.IOError as e:
+        repo.abort_transaction()
+
+        if e.code == Gio.IOErrorEnum.PERMISSION_DENIED:
+            raise _ArtifactError(e.message, _ArtifactErrorReason.PERMISSION_DENIED) from e
+        else:
+            raise
     except:
         repo.abort_transaction()
         raise
diff --git a/buildstream/element.py b/buildstream/element.py
index e3e9a4b34..94ba11f35 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -38,7 +38,7 @@ import shutil
 from . import _yaml
 from ._yaml import CompositePolicy
 from ._variables import Variables
-from .exceptions import _BstError
+from .exceptions import _BstError, _ArtifactError, _ArtifactErrorReason
 from . import LoadError, LoadErrorReason, ElementError, ImplError
 from . import Plugin, Consistency
 from .project import BST_ARTIFACT_VERSION as BST_CORE_ARTIFACT_VERSION
@@ -1040,7 +1040,7 @@ class Element(Plugin):
 
             # Cleanup the build directory on explicit SIGTERM
             def cleanup_rootdir():
-                shutil.rmtree(rootdir)
+                utils._force_rmtree(rootdir)
 
             with _signals.terminator(cleanup_rootdir), \
                 self.__sandbox(rootdir, output_file, output_file) as sandbox:  # nopep8
@@ -1112,11 +1112,22 @@ class Element(Plugin):
                     }
                     _yaml.dump(_yaml.node_sanitize(meta), os.path.join(metadir, 'artifact.yaml'))
 
-                    with self.timed_activity("Caching Artifact"):
-                        self.__artifacts.commit(self, assembledir)
+                    try:
+                        with self.timed_activity("Caching Artifact"):
+                            self.__artifacts.commit(self, assembledir)
+                    except _ArtifactError as e:
+                        if e.reason == _ArtifactErrorReason.PERMISSION_DENIED:
+                            raise ElementError("Permission denied while trying "
+                                               "to commit artifact: {}".format(e)) from e
+                        else:
+                            raise
+
+                    finally:
+                        # Ensure that the assembledir can be removed
+                        utils._force_rmtree(assembledir)
 
             # Finally cleanup the build dir
-            shutil.rmtree(rootdir)
+            cleanup_rootdir()
 
     # _pull():
     #
@@ -1415,7 +1426,7 @@ class Element(Plugin):
                 yield sandbox
 
             # Cleanup the build dir
-            shutil.rmtree(rootdir)
+            utils._force_rmtree(rootdir)
 
     def __compose_default_splits(self, defaults):
         project = self.get_project()
diff --git a/buildstream/exceptions.py b/buildstream/exceptions.py
index c33e7d7bf..e79176e3d 100644
--- a/buildstream/exceptions.py
+++ b/buildstream/exceptions.py
@@ -157,5 +157,13 @@ class PlatformError(_BstError):
     pass
 
 
+class _ArtifactErrorReason(Enum):
+    MISC = 0
+    PERMISSION_DENIED = 1
+
+
 class _ArtifactError(_BstError):
-    pass
+    def __init__(self, message, reason=_ArtifactErrorReason.MISC):
+        super(_ArtifactError, self).__init__(message)
+
+        self.reason = reason
diff --git a/buildstream/utils.py b/buildstream/utils.py
index 273d5dd0b..2a873bf57 100644
--- a/buildstream/utils.py
+++ b/buildstream/utils.py
@@ -465,14 +465,26 @@ def get_bst_version():
     return (int(versions[0]), int(versions[1]))
 
 
-# Recursively make directories in target area and copy permissions
+# Recursively remove directories, ignoring file permissions as much as
+# possible.
+def _force_rmtree(path, **kwargs):
+    for root, dirs, _ in os.walk(path):
+        for d in dirs:
+            path = os.path.join(root, d.lstrip('/'))
+            if os.path.exists(path) and not os.path.islink(path):
+                os.chmod(path, 0o755)
+
+    shutil.rmtree(path, **kwargs)
+
+
+# Recursively make directories in target area
 def _copy_directories(srcdir, destdir, target):
     this_dir = os.path.dirname(target)
     new_dir = os.path.join(destdir, this_dir)
 
     if not os.path.lexists(new_dir):
         if this_dir:
-            _copy_directories(srcdir, destdir, this_dir)
+            yield from _copy_directories(srcdir, destdir, this_dir)
 
         old_dir = os.path.join(srcdir, this_dir)
         if os.path.lexists(old_dir):
@@ -481,7 +493,7 @@ def _copy_directories(srcdir, destdir, target):
 
             if stat.S_ISDIR(mode) or stat.S_ISLNK(mode):
                 os.makedirs(new_dir)
-                shutil.copystat(old_dir, new_dir)
+                yield (new_dir, mode)
             else:
                 raise OSError('Source directory tree has file where '
                               'directory expected: %s' % dir)
@@ -533,6 +545,10 @@ def _ensure_real_directory(root, destpath):
 #
 def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=False):
 
+    # Keep track of directory permissions, since these need to be set
+    # *after* files have been written.
+    permissions = []
+
     # Note we consume the filelist (which is a generator and not a list)
     # by sorting it, this is necessary to ensure that we processes symbolic
     # links which lead to directories before processing files inside those
@@ -547,7 +563,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             result.overwritten.append(path)
 
         # The destination directory may not have been created separately
-        _copy_directories(srcdir, destdir, path)
+        permissions.extend(_copy_directories(srcdir, destdir, path))
 
         # Ensure that broken symlinks to directories have their targets
         # created before attempting to stage files across broken
@@ -557,6 +573,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
         try:
             file_stat = os.lstat(srcpath)
             mode = file_stat.st_mode
+
         except FileNotFoundError:
             # Skip this missing file
             if ignore_missing:
@@ -573,7 +590,7 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             if not stat.S_ISDIR(dest_stat.st_mode):
                 raise OSError('Destination not a directory. source has %s'
                               ' destination has %s' % (srcpath, destpath))
-            shutil.copystat(srcpath, destpath)
+            permissions.append((destpath, os.stat(srcpath).st_mode))
 
         elif stat.S_ISLNK(mode):
             if not safe_remove(destpath):
@@ -607,6 +624,10 @@ def _process_list(srcdir, destdir, filelist, actionfunc, result, ignore_missing=
             # Unsupported type.
             raise OSError('Cannot extract %s into staging-area. Unsupported type.' % srcpath)
 
+    # Write directory permissions now that all files have been written
+    for d, perms in permissions:
+        os.chmod(d, perms)
+
 
 # _relative_symlink_target()
 #