From cc093b0ea933fa8f19a953934c9981ad1ff71b63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Billeter?= Date: Wed, 10 Jun 2020 08:04:52 +0200 Subject: utils.py: Fix error handling in safe_copy() Catch errors raised by `shutil.copyfile()`. --- src/buildstream/utils.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index 99c22d169..9591951e8 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -385,7 +385,11 @@ def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) - if e.errno != errno.ENOENT: raise UtilError("Failed to remove destination file '{}': {}".format(dest, e)) from e - shutil.copyfile(src, dest) + try: + shutil.copyfile(src, dest) + except (OSError, shutil.Error) as e: + raise UtilError("Failed to copy '{} -> {}': {}".format(src, dest, e)) from e + try: shutil.copystat(src, dest) except PermissionError: @@ -398,9 +402,6 @@ def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) - if result: result.failed_attributes.append(dest) - except shutil.Error as e: - raise UtilError("Failed to copy '{} -> {}': {}".format(src, dest, e)) from e - def safe_link(src: str, dest: str, *, result: Optional[FileListResult] = None, _unlink=False) -> None: """Try to create a hardlink, but resort to copying in the case of cross device links. -- cgit v1.2.1 From 81c0fb1a468c2bef9fe7128a6d44ce38315d6d14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Billeter?= Date: Wed, 10 Jun 2020 08:20:10 +0200 Subject: utils.py: Make copystat optional in safe_copy() --- src/buildstream/utils.py | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/buildstream/utils.py b/src/buildstream/utils.py index 9591951e8..9c6761ccc 100644 --- a/src/buildstream/utils.py +++ b/src/buildstream/utils.py @@ -363,19 +363,20 @@ def sha256sum(filename: str) -> str: return h.hexdigest() -def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) -> None: - """Copy a file while preserving attributes +def safe_copy(src: str, dest: str, *, copystat: bool = True, result: Optional[FileListResult] = None) -> None: + """Copy a file while optionally preserving attributes Args: src: The source filename dest: The destination filename + copystat: Whether to preserve attributes result: An optional collective result Raises: UtilError: In the case of unexpected system call failures - This is almost the same as shutil.copy2(), except that - we unlink *dest* before overwriting it if it exists, just + This is almost the same as shutil.copy2() when copystat is True, + except that we unlink *dest* before overwriting it if it exists, just incase *dest* is a hardlink to a different file. """ # First unlink the target if it exists @@ -390,17 +391,18 @@ def safe_copy(src: str, dest: str, *, result: Optional[FileListResult] = None) - except (OSError, shutil.Error) as e: raise UtilError("Failed to copy '{} -> {}': {}".format(src, dest, e)) from e - try: - shutil.copystat(src, dest) - except PermissionError: - # If we failed to copy over some file stats, dont treat - # it as an unrecoverable error, but provide some feedback - # we can use for a warning. - # - # This has a tendency of happening when attempting to copy - # over extended file attributes. - if result: - result.failed_attributes.append(dest) + if copystat: + try: + shutil.copystat(src, dest) + except PermissionError: + # If we failed to copy over some file stats, dont treat + # it as an unrecoverable error, but provide some feedback + # we can use for a warning. + # + # This has a tendency of happening when attempting to copy + # over extended file attributes. + if result: + result.failed_attributes.append(dest) def safe_link(src: str, dest: str, *, result: Optional[FileListResult] = None, _unlink=False) -> None: -- cgit v1.2.1 From 725435f79b4a0284693415c5367cb9013fcd8cfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=BCrg=20Billeter?= Date: Wed, 10 Jun 2020 08:26:54 +0200 Subject: cascache.py: Fix file modes in checkout() Do not copy file mode from casd object file. Do not change non-executable mode bits if file is executable. --- src/buildstream/_cas/cascache.py | 21 ++++++++++----------- tests/frontend/pull.py | 14 ++++++++------ tests/sources/remote.py | 9 +++++++-- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/src/buildstream/_cas/cascache.py b/src/buildstream/_cas/cascache.py index 9394ac5b2..6b2f0d1a4 100644 --- a/src/buildstream/_cas/cascache.py +++ b/src/buildstream/_cas/cascache.py @@ -241,21 +241,20 @@ class CASCache: if can_link and mtime is None: utils.safe_link(self.objpath(filenode.digest), fullpath) else: - utils.safe_copy(self.objpath(filenode.digest), fullpath) + utils.safe_copy(self.objpath(filenode.digest), fullpath, copystat=False) if mtime is not None: utils._set_file_mtime(fullpath, mtime) if filenode.is_executable: - os.chmod( - fullpath, - stat.S_IRUSR - | stat.S_IWUSR - | stat.S_IXUSR - | stat.S_IRGRP - | stat.S_IXGRP - | stat.S_IROTH - | stat.S_IXOTH, - ) + st = os.stat(fullpath) + mode = st.st_mode + if mode & stat.S_IRUSR: + mode |= stat.S_IXUSR + if mode & stat.S_IRGRP: + mode |= stat.S_IXGRP + if mode & stat.S_IROTH: + mode |= stat.S_IXOTH + os.chmod(fullpath, mode) for dirnode in directory.directories: fullpath = os.path.join(dest, dirnode.name) diff --git a/tests/frontend/pull.py b/tests/frontend/pull.py index 6a5a41977..c1fa76ab0 100644 --- a/tests/frontend/pull.py +++ b/tests/frontend/pull.py @@ -442,6 +442,8 @@ def test_pull_access_rights(cli, tmpdir, datafiles): project = str(datafiles) checkout = os.path.join(str(tmpdir), "checkout") + umask = utils.get_umask() + # Work-around datafiles not preserving mode os.chmod(os.path.join(project, "files/bin-files/usr/bin/hello"), 0o0755) @@ -467,15 +469,15 @@ def test_pull_access_rights(cli, tmpdir, datafiles): st = os.lstat(os.path.join(checkout, "usr/include/pony.h")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0644 + assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask st = os.lstat(os.path.join(checkout, "usr/bin/hello")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0755 + assert stat.S_IMODE(st.st_mode) == 0o0777 & ~umask st = os.lstat(os.path.join(checkout, "usr/share/big-file")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0644 + assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask shutil.rmtree(checkout) @@ -493,15 +495,15 @@ def test_pull_access_rights(cli, tmpdir, datafiles): st = os.lstat(os.path.join(checkout, "usr/include/pony.h")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0644 + assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask st = os.lstat(os.path.join(checkout, "usr/bin/hello")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0755 + assert stat.S_IMODE(st.st_mode) == 0o0777 & ~umask st = os.lstat(os.path.join(checkout, "usr/share/big-file")) assert stat.S_ISREG(st.st_mode) - assert stat.S_IMODE(st.st_mode) == 0o0644 + assert stat.S_IMODE(st.st_mode) == 0o0666 & ~umask # Tests `bst artifact pull $artifact_ref` diff --git a/tests/sources/remote.py b/tests/sources/remote.py index d546c6131..416d2d53c 100644 --- a/tests/sources/remote.py +++ b/tests/sources/remote.py @@ -5,6 +5,7 @@ import os import stat import pytest +from buildstream import utils from buildstream.testing import ErrorDomain from buildstream.testing import generate_project from buildstream.testing import cli # pylint: disable=unused-import @@ -71,8 +72,12 @@ def test_simple_file_build(cli, tmpdir, datafiles): mode = os.stat(checkout_file).st_mode # Assert not executable by anyone assert not mode & (stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) - # Assert not writeable by anyone other than me - assert not mode & (stat.S_IWGRP | stat.S_IWOTH) + + # Assert not writeable by anyone other than me (unless umask allows it) + if utils.get_umask() & stat.S_IWGRP: + assert not mode & stat.S_IWGRP + if utils.get_umask() & stat.S_IWOTH: + assert not mode & stat.S_IWOTH @pytest.mark.datafiles(os.path.join(DATA_DIR, "single-file-custom-name")) -- cgit v1.2.1