Merge branch 'juerg/umask' into 'master'

Respect umask for created file and directories

See merge request BuildStream/buildstream!1724

4 files changed, 53 insertions, 18 deletions

diff --git a/src/buildstream/element.py b/src/buildstream/element.py
index a8c6bfa8f..e40dc3c8c 100644
--- a/src/buildstream/element.py
+++ b/src/buildstream/element.py
@@ -81,7 +81,6 @@ import contextlib
 from contextlib import contextmanager
 from functools import partial
 from itertools import chain
-import tempfile
 import string
 from typing import cast, TYPE_CHECKING, Any, Dict, Iterator, List, Optional
 
@@ -94,7 +93,6 @@ from ._exceptions import BstError, LoadError, LoadErrorReason, ImplError, ErrorD
 from .utils import FileListResult
 from . import utils
 from . import _cachekey
-from . import _signals
 from . import _site
 from ._platform import Platform
 from .node import Node
@@ -1635,13 +1633,10 @@ class Element(Plugin):
 
             # Explicitly clean it up, keep the build dir around if exceptions are raised
             os.makedirs(context.builddir, exist_ok=True)
-            rootdir = tempfile.mkdtemp(prefix="{}-".format(self.normal_name), dir=context.builddir)
 
-            # Cleanup the build directory on explicit SIGTERM
-            def cleanup_rootdir():
-                utils._force_rmtree(rootdir)
-
-            with _signals.terminator(cleanup_rootdir), self.__sandbox(
+            with utils._tempdir(
+                prefix="{}-".format(self.normal_name), dir=context.builddir
+            ) as rootdir, self.__sandbox(
                 rootdir, output_file, output_file, self.__sandbox_config
             ) as sandbox:  # noqa
 
@@ -1696,8 +1691,6 @@ class Element(Plugin):
                     raise
                 else:
                     return self._cache_artifact(rootdir, sandbox, collect)
-                finally:
-                    cleanup_rootdir()
 
     def _cache_artifact(self, rootdir, sandbox, collect):
 
@@ -2587,17 +2580,15 @@ class Element(Plugin):
 
         else:
             os.makedirs(context.builddir, exist_ok=True)
-            rootdir = tempfile.mkdtemp(prefix="{}-".format(self.normal_name), dir=context.builddir)
 
             # Recursive contextmanager...
-            with self.__sandbox(
+            with utils._tempdir(
+                prefix="{}-".format(self.normal_name), dir=context.builddir
+            ) as rootdir, self.__sandbox(
                 rootdir, stdout=stdout, stderr=stderr, config=config, allow_remote=allow_remote, bare_directory=False
             ) as sandbox:
                 yield sandbox
 
-            # Cleanup the build dir
-            utils._force_rmtree(rootdir)
-
     @classmethod
     def __compose_default_splits(cls, project, defaults, is_junction):
 
diff --git a/src/buildstream/plugins/sources/tar.py b/src/buildstream/plugins/sources/tar.py
index 658cc2735..e35b52f85 100644
--- a/src/buildstream/plugins/sources/tar.py
+++ b/src/buildstream/plugins/sources/tar.py
@@ -76,8 +76,13 @@ class ReadableTarInfo(tarfile.TarInfo):
 
     @property
     def mode(self):
-        # ensure file is readable by owner
-        return self.__permission | 0o400
+        # Respect umask instead of the file mode stored in the archive.
+        # The only bit used from the embedded mode is the executable bit for files.
+        umask = utils.get_umask()
+        if self.isdir() or bool(self.__permission | 0o100):
+            return 0o777 & ~umask
+        else:
+            return 0o666 & ~umask
 
     @mode.setter
     def mode(self, permission):
diff --git a/src/buildstream/testing/integration.py b/src/buildstream/testing/integration.py
index 584d7da1b..9a1a48816 100644
--- a/src/buildstream/testing/integration.py
+++ b/src/buildstream/testing/integration.py
@@ -28,6 +28,8 @@ import tempfile
 
 import pytest
 
+from buildstream import utils
+
 
 # Return a list of files relative to the given directory
 def walk_dir(root):
@@ -66,6 +68,8 @@ class IntegrationCache:
         # the artifacts directory
         try:
             self.cachedir = tempfile.mkdtemp(dir=self.root, prefix="cache-")
+            # Apply mode allowed by umask
+            os.chmod(self.cachedir, 0o777 & ~utils.get_umask())
         except OSError as e:
             raise AssertionError("Unable to create test directory !") from e
 
diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py
index 181ea1df9..7f7bf67b2 100644
--- a/src/buildstream/utils.py
+++ b/src/buildstream/utils.py
@@ -65,6 +65,11 @@ _INITIAL_NUM_THREADS_IN_MAIN_PROCESS = 1
 # Number of seconds to wait for background threads to exit.
 _AWAIT_THREADS_TIMEOUT_SECONDS = 5
 
+# The process's file mode creation mask.
+# Impossible to retrieve without temporarily changing it on POSIX.
+_UMASK = os.umask(0o777)
+os.umask(_UMASK)
+
 
 class UtilError(BstError):
     """Raised by utility functions when system calls fail.
@@ -602,6 +607,8 @@ def save_file_atomic(
     if tempdir is None:
         tempdir = os.path.dirname(filename)
     fd, tempname = tempfile.mkstemp(dir=tempdir)
+    # Apply mode allowed by umask
+    os.fchmod(fd, 0o666 & ~_UMASK)
     os.close(fd)
 
     f = open(
@@ -638,6 +645,17 @@ def save_file_atomic(
         raise
 
 
+# get_umask():
+#
+# Get the process's file mode creation mask without changing it.
+#
+# Returns:
+#     (int) The process's file mode creation mask.
+#
+def get_umask():
+    return _UMASK
+
+
 # _get_dir_size():
 #
 # Get the disk usage of a given directory in bytes.
@@ -1002,6 +1020,13 @@ def _set_deterministic_mtime(directory):
 #
 # A context manager for doing work in a temporary directory.
 #
+# NOTE: Unlike mkdtemp(), this method may not restrict access to other
+#       users. The process umask is the only access restriction, similar
+#       to mkdir().
+#       This is potentially insecure. Do not create directories in /tmp
+#       with this method. *Only* use this in directories whose parents are
+#       more tightly controlled (i.e., non-public directories).
+#
 # Args:
 #    dir (str): A path to a parent directory for the temporary directory
 #    suffix (str): A suffix for the temproary directory name
@@ -1015,7 +1040,14 @@ def _set_deterministic_mtime(directory):
 # supports cleaning up the temp directory on SIGTERM.
 #
 @contextmanager
-def _tempdir(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-builtin
+def _tempdir(*, suffix="", prefix="tmp", dir):  # pylint: disable=redefined-builtin
+    # Do not allow fallback to a global temp directory. Due to the chmod
+    # below, this method is not safe to be used in global temp
+    # directories such as /tmp.
+    assert (
+        dir
+    ), "Creating directories in the public fallback `/tmp` is dangerous. Please use a directory with tight access controls."
+
     tempdir = tempfile.mkdtemp(suffix=suffix, prefix=prefix, dir=dir)
 
     def cleanup_tempdir():
@@ -1024,6 +1056,9 @@ def _tempdir(suffix="", prefix="tmp", dir=None):  # pylint: disable=redefined-bu
 
     try:
         with _signals.terminator(cleanup_tempdir):
+            # Apply mode allowed by umask
+            os.chmod(tempdir, 0o777 & ~_UMASK)
+
             yield tempdir
     finally:
         cleanup_tempdir()