Merge branch 'jmac/tempfile-extraction-bug' into 'master'575-os-sched_getaffinity-not-supported-on-macosx-blocks-411

Correct crash after staging tars with read-only directories

See merge request BuildStream/buildstream!713

4 files changed, 69 insertions, 1 deletions

diff --git a/buildstream/element.py b/buildstream/element.py
index a34b1ca36..d9c096992 100644
--- a/buildstream/element.py
+++ b/buildstream/element.py
@@ -1361,8 +1361,12 @@ class Element(Plugin):
             if not vdirectory.is_empty():
                 raise ElementError("Staging directory '{}' is not empty".format(vdirectory))
 
-            with tempfile.TemporaryDirectory() as temp_staging_directory:
+            # While mkdtemp is advertised as using the TMP environment variable, it
+            # doesn't, so this explicit extraction is necesasry.
+            tmp_prefix = os.environ.get("TMP", None)
+            temp_staging_directory = tempfile.mkdtemp(prefix=tmp_prefix)
 
+            try:
                 workspace = self._get_workspace()
                 if workspace:
                     # If mount_workspaces is set and we're doing incremental builds,
@@ -1377,6 +1381,19 @@ class Element(Plugin):
                         source._stage(temp_staging_directory)
 
                 vdirectory.import_files(temp_staging_directory)
+
+            finally:
+                # Staging may produce directories with less than 'rwx' permissions
+                # for the owner, which will break tempfile, so we need to use chmod
+                # occasionally.
+                def make_dir_writable(fn, path, excinfo):
+                    os.chmod(os.path.dirname(path), 0o777)
+                    if os.path.isdir(path):
+                        os.rmdir(path)
+                    else:
+                        os.remove(path)
+                shutil.rmtree(temp_staging_directory, onerror=make_dir_writable)
+
         # Ensure deterministic mtime of sources at build time
         vdirectory.set_deterministic_mtime()
         # Ensure deterministic owners of sources at build time
diff --git a/tests/sources/tar.py b/tests/sources/tar.py
index fb02de306..a13fe2647 100644
--- a/tests/sources/tar.py
+++ b/tests/sources/tar.py
@@ -3,6 +3,7 @@ import pytest
 import tarfile
 import tempfile
 import subprocess
+from shutil import copyfile, rmtree
 
 from buildstream._exceptions import ErrorDomain
 from buildstream import _yaml
@@ -257,3 +258,47 @@ def test_stage_default_basedir_lzip(cli, tmpdir, datafiles, srcdir):
     original_contents = list_dir_contents(original_dir)
     checkout_contents = list_dir_contents(checkoutdir)
     assert(checkout_contents == original_contents)
+
+
+# Test that a tarball that contains a read only dir works
+@pytest.mark.datafiles(os.path.join(DATA_DIR, 'read-only'))
+def test_read_only_dir(cli, tmpdir, datafiles):
+    try:
+        project = os.path.join(datafiles.dirname, datafiles.basename)
+        generate_project(project, tmpdir)
+
+        # Get the tarball in tests/sources/tar/read-only/content
+        #
+        # NOTE that we need to do this because tarfile.open and tar.add()
+        # are packing the tar up with writeable files and dirs
+        tarball = os.path.join(str(datafiles), 'content', 'a.tar.gz')
+        if not os.path.exists(tarball):
+            raise FileNotFoundError('{} does not exist'.format(tarball))
+        copyfile(tarball, os.path.join(str(tmpdir), 'a.tar.gz'))
+
+        # Because this test can potentially leave directories behind
+        # which are difficult to remove, ask buildstream to use
+        # our temp directory, so we can clean up.
+        tmpdir_str = str(tmpdir)
+        if not tmpdir_str.endswith(os.path.sep):
+            tmpdir_str += os.path.sep
+        env = {"TMP": tmpdir_str}
+
+        # Track, fetch, build, checkout
+        result = cli.run(project=project, args=['track', 'target.bst'], env=env)
+        result.assert_success()
+        result = cli.run(project=project, args=['fetch', 'target.bst'], env=env)
+        result.assert_success()
+        result = cli.run(project=project, args=['build', 'target.bst'], env=env)
+        result.assert_success()
+
+    finally:
+
+        # Make tmpdir deletable no matter what happens
+        def make_dir_writable(fn, path, excinfo):
+            os.chmod(os.path.dirname(path), 0o777)
+            if os.path.isdir(path):
+                os.rmdir(path)
+            else:
+                os.remove(path)
+        rmtree(str(tmpdir), onerror=make_dir_writable)
diff --git a/tests/sources/tar/read-only/content/a.tar.gz b/tests/sources/tar/read-only/content/a.tar.gz
new file mode 100644
index 000000000..70922627b
--- /dev/null
+++ b/tests/sources/tar/read-only/content/a.tar.gz
diff --git a/tests/sources/tar/read-only/target.bst b/tests/sources/tar/read-only/target.bst
new file mode 100644
index 000000000..ad413a2f4
--- /dev/null
+++ b/tests/sources/tar/read-only/target.bst
@@ -0,0 +1,6 @@
+kind: import
+description: The kind of this element is irrelevant.
+sources:
+- kind: tar
+  url: tmpdir:/a.tar.gz
+  ref: foo