From e81b4eae56d7f212024c4369b0c23c2c4129b7d1 Mon Sep 17 00:00:00 2001 From: Daniel Fleischer Date: Tue, 3 Nov 2020 12:16:06 +0200 Subject: [PATCH 1/5] utils/fs: checking files ownership in 'move' (#4348) --- dvc/utils/fs.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index d80bce6ede..975ab9c1f5 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -95,15 +95,19 @@ def move(src, dst, mode=None): dst = os.path.abspath(dst) tmp = f"{dst}.{uuid()}" + try: + if mode is not None: + os.chmod(src, mode) + except OSError: + # File not owned by us, raise exception + raise DvcException(f"File not owned by user: {src}") + if os.path.islink(src): shutil.copy(src, tmp) os.unlink(src) else: shutil.move(src, tmp) - if mode is not None: - os.chmod(tmp, mode) - shutil.move(tmp, dst) From e82d13281071f0ae92c69f7ac1a91ea3da67215d Mon Sep 17 00:00:00 2001 From: danielfleischer Date: Thu, 5 Nov 2020 10:52:31 +0200 Subject: [PATCH 2/5] Update fs.py Exception is verbose enough. --- dvc/utils/fs.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index 975ab9c1f5..de77da6bc6 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -99,7 +99,6 @@ def move(src, dst, mode=None): if mode is not None: os.chmod(src, mode) except OSError: - # File not owned by us, raise exception raise DvcException(f"File not owned by user: {src}") if os.path.islink(src): From 77974fa334e292f87675ec828eb36d8c1c91562f Mon Sep 17 00:00:00 2001 From: Daniel Fleischer Date: Thu, 5 Nov 2020 20:14:47 +0200 Subject: [PATCH 3/5] Using 'reraise' with new dedicated exception --- dvc/exceptions.py | 5 +++++ dvc/utils/fs.py | 7 +++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 17b4a87eda..b793dad2a3 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -218,6 +218,11 @@ def __init__(self, path, hint=None): ) +class FileOwnershipError(DvcException): + def __init__(self, path): + super().__init__(f"file '{path}' not owned by user! ") + + class DvcIgnoreInCollectedDirError(DvcException): def __init__(self, ignore_dirname): super().__init__( diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index de77da6bc6..54c5010975 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -6,9 +6,10 @@ import sys import nanotime +from funcy import reraise from shortuuid import uuid -from dvc.exceptions import DvcException +from dvc.exceptions import DvcException, FileOwnershipError from dvc.system import System from dvc.utils import dict_md5 @@ -95,11 +96,9 @@ def move(src, dst, mode=None): dst = os.path.abspath(dst) tmp = f"{dst}.{uuid()}" - try: + with reraise(OSError, FileOwnershipError(src)): if mode is not None: os.chmod(src, mode) - except OSError: - raise DvcException(f"File not owned by user: {src}") if os.path.islink(src): shutil.copy(src, tmp) From 557976784db272427fa08bea5c0ccdea9b25c22c Mon Sep 17 00:00:00 2001 From: Daniel Fleischer Date: Fri, 6 Nov 2020 20:07:39 +0200 Subject: [PATCH 4/5] Specific error handling If error related to access/permission, throw the newly created FileOwnershipError exception, otherwise throw the OSError as is. --- dvc/utils/fs.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index 54c5010975..51a33077e6 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -6,7 +6,6 @@ import sys import nanotime -from funcy import reraise from shortuuid import uuid from dvc.exceptions import DvcException, FileOwnershipError @@ -96,9 +95,14 @@ def move(src, dst, mode=None): dst = os.path.abspath(dst) tmp = f"{dst}.{uuid()}" - with reraise(OSError, FileOwnershipError(src)): + try: if mode is not None: os.chmod(src, mode) + except OSError as e: + if e.errno not in [errno.EACCES, errno.EPERM]: + raise + else: + raise FileOwnershipError(src) if os.path.islink(src): shutil.copy(src, tmp) From 314f49eb49b0a6ec2da313e540f7e0c3469117f0 Mon Sep 17 00:00:00 2001 From: Daniel Fleischer Date: Mon, 9 Nov 2020 22:16:33 +0200 Subject: [PATCH 5/5] Using safer 'unlink' Using the function _unlink which includes error handling. --- dvc/utils/fs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index 51a33077e6..a5cdf1ca88 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -106,7 +106,7 @@ def move(src, dst, mode=None): if os.path.islink(src): shutil.copy(src, tmp) - os.unlink(src) + _unlink(src, _chmod) else: shutil.move(src, tmp)