From 335a1ecea34ea8a1d3674b8882130f048cfab545 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Tue, 11 Jun 2019 10:44:43 +0100 Subject: utils: indent 'with' continuation line --- src/buildstream/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index 0e2cdeb43..f47f31a29 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -1013,7 +1013,7 @@ def _tempnamedfile(suffix="", prefix="tmp", dir=None): # pylint: disable=redefi temp.close() with _signals.terminator(close_tempfile), \ - tempfile.NamedTemporaryFile(suffix=suffix, prefix=prefix, dir=dir) as temp: + tempfile.NamedTemporaryFile(suffix=suffix, prefix=prefix, dir=dir) as temp: yield temp -- cgit v1.2.1 From 712250bbf13e54c081861fe58d6d12151e987c3a Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 27 Jun 2019 13:30:14 +0100 Subject: utils: fix yield type of _tempnamedfile --- src/buildstream/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index f47f31a29..18810b557 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -998,7 +998,7 @@ def _tempdir(suffix="", prefix="tmp", dir=None): # pylint: disable=redefined-bu # prefix (str): A prefix for the temporary file name # # Yields: -# (str): The temporary file handle +# (tempfile.NamedTemporaryFile): The temporary file handle # # Do not use tempfile.NamedTemporaryFile() directly, as this will # leak files on the filesystem when BuildStream exits a process -- cgit v1.2.1 From 1f9a077bb1d6877d1417870b0aad3a6a31012afb Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 6 Jun 2019 16:23:59 +0100 Subject: utils: add _tempnamedfile_name for win32 happiness Avoid a restriction of Windows that prevents us from using both the file descriptor and the file name of tempfile.NamedTemporaryFile. Provide a wrapper that only returns the temporary filename, and makes it easier to be windows-compatible. --- src/buildstream/utils.py | 60 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index 18810b557..03ff67bf2 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -1017,6 +1017,66 @@ def _tempnamedfile(suffix="", prefix="tmp", dir=None): # pylint: disable=redefi yield temp +# _tempnamedfile_name() +# +# A context manager for doing work on a temporary file, via the filename only. +# +# Note that a Windows restriction prevents us from using both the file +# descriptor and the file name of tempfile.NamedTemporaryFile, the same file +# cannot be opened twice at the same time. This wrapper makes it easier to +# operate in a Windows-friendly way when only filenames are needed. +# +# Takes care to delete the file even if interrupted by SIGTERM. Note that there +# is a race-condition, so this is done on a best-effort basis. +# +# Args: +# dir (str): A path to a parent directory for the temporary file, this is +# not optional for security reasons, please see below. +# +# Note that 'dir' should not be a directory that may be shared with potential +# attackers that have rights to affect the directory, e.g. the system temp +# directory. This is because an attacker can replace the temporary file with +# e.g. a symlink to another location, that the BuildStream user has rights to +# but the attacker does not. +# +# See here for more information: +# https://www.owasp.org/index.php/Insecure_Temporary_File +# +# Yields: +# (str): The temporary file name +# +@contextmanager +def _tempnamedfile_name(dir): # pylint: disable=redefined-builtin + filename = None + + def rm_tempfile(): + nonlocal filename + if filename is not None: + # Note that this code is re-entrant - it's possible for us to + # "delete while we're deleting" if we are responding to SIGTERM. + # + # For simplicity, choose to leak the file in this unlikely case, + # rather than introducing a scheme for handling the file sometimes + # being missing. Note that we will still leak tempfiles on forced + # termination anyway. + # + # Note that we must not try to delete the file more than once. If + # we delete the file then another temporary file can be created + # with the same name. + # + filename2 = filename + filename = None + os.remove(filename2) + + with _signals.terminator(rm_tempfile): + fd, filename = tempfile.mkstemp(dir=dir) + os.close(fd) + try: + yield filename + finally: + rm_tempfile() + + # _kill_process_tree() # # Brutally murder a process and all of its children -- cgit v1.2.1 From a46307c561ae84443e240b5540f037f993e2a9ae Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Thu, 27 Jun 2019 10:34:36 +0100 Subject: _artifact.py: use utils._tempnamedfile_name We are only using the filename of the temporary file here, so use the wrapper for this use-case. --- src/buildstream/_artifact.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/buildstream/_artifact.py b/src/buildstream/_artifact.py index 02adb3404..ffe92db16 100644 --- a/src/buildstream/_artifact.py +++ b/src/buildstream/_artifact.py @@ -29,7 +29,6 @@ artifact composite interaction away from Element class """ import os -import tempfile from ._protos.buildstream.v2.artifact_pb2 import Artifact as ArtifactProto from . import _yaml @@ -148,9 +147,9 @@ class Artifact(): size += filesvdir.get_size() # Store public data - with tempfile.NamedTemporaryFile(dir=self._tmpdir) as tmp: - _yaml.dump(publicdata, tmp.name) - public_data_digest = self._cas.add_object(path=tmp.name, link_directly=True) + with utils._tempnamedfile_name(dir=self._tmpdir) as tmpname: + _yaml.dump(publicdata, tmpname) + public_data_digest = self._cas.add_object(path=tmpname, link_directly=True) artifact.public_data.CopyFrom(public_data_digest) size += public_data_digest.size_bytes -- cgit v1.2.1