From 132131b404e06ee1a19b040a1f96cd1118abed0c Mon Sep 17 00:00:00 2001 From: Winson Luk Date: Tue, 2 Mar 2021 15:53:15 -0500 Subject: [PATCH] bpo-42782: Fail fast for permission errors in shutil.move() (GH-24001) * Fail fast in shutil.move() to avoid creating destination directories on failure. Co-authored-by: Zackery Spytz --- Lib/shutil.py | 11 ++++++ Lib/test/test_shutil.py | 37 +++++++++++++++++++ .../2020-12-29-13-46-57.bpo-42782.3r0HFY.rst | 2 + 3 files changed, 50 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst diff --git a/Lib/shutil.py b/Lib/shutil.py index f0e833dc979..89d924dec8a 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -813,6 +813,12 @@ def move(src, dst, copy_function=copy2): if _destinsrc(src, dst): raise Error("Cannot move a directory '%s' into itself" " '%s'." % (src, dst)) + if (_is_immutable(src) + or (not os.access(src, os.W_OK) and os.listdir(src) + and sys.platform == 'darwin')): + raise PermissionError("Cannot move the non-empty directory " + "'%s': Lacking write permission to '%s'." + % (src, src)) copytree(src, real_dst, copy_function=copy_function, symlinks=True) rmtree(src) @@ -830,6 +836,11 @@ def _destinsrc(src, dst): dst += os.path.sep return dst.startswith(src) +def _is_immutable(src): + st = _stat(src) + immutable_states = [stat.UF_IMMUTABLE, stat.SF_IMMUTABLE] + return hasattr(st, 'st_flags') and st.st_flags in immutable_states + def _get_gid(name): """Returns a gid, given a group name.""" if getgrnam is None or name is None: diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index df8dcdcce60..4bcad51509d 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -34,6 +34,8 @@ from test.support import os_helper from test.support.os_helper import TESTFN, FakePath TESTFN2 = TESTFN + "2" +TESTFN_SRC = TESTFN + "_SRC" +TESTFN_DST = TESTFN + "_DST" MACOS = sys.platform.startswith("darwin") AIX = sys.platform[:3] == 'aix' try: @@ -2085,6 +2087,41 @@ class TestMove(BaseTest, unittest.TestCase): os.rmdir(dst_dir) + @unittest.skipUnless(hasattr(os, 'geteuid') and os.geteuid() == 0 + and hasattr(os, 'lchflags') + and hasattr(stat, 'SF_IMMUTABLE') + and hasattr(stat, 'UF_OPAQUE'), + 'root privileges required') + def test_move_dir_permission_denied(self): + # bpo-42782: shutil.move should not create destination directories + # if the source directory cannot be removed. + try: + os.mkdir(TESTFN_SRC) + os.lchflags(TESTFN_SRC, stat.SF_IMMUTABLE) + + # Testing on an empty immutable directory + # TESTFN_DST should not exist if shutil.move failed + self.assertRaises(PermissionError, shutil.move, TESTFN_SRC, TESTFN_DST) + self.assertFalse(TESTFN_DST in os.listdir()) + + # Create a file and keep the directory immutable + os.lchflags(TESTFN_SRC, stat.UF_OPAQUE) + os_helper.create_empty_file(os.path.join(TESTFN_SRC, 'child')) + os.lchflags(TESTFN_SRC, stat.SF_IMMUTABLE) + + # Testing on a non-empty immutable directory + # TESTFN_DST should not exist if shutil.move failed + self.assertRaises(PermissionError, shutil.move, TESTFN_SRC, TESTFN_DST) + self.assertFalse(TESTFN_DST in os.listdir()) + finally: + if os.path.exists(TESTFN_SRC): + os.lchflags(TESTFN_SRC, stat.UF_OPAQUE) + os_helper.rmtree(TESTFN_SRC) + if os.path.exists(TESTFN_DST): + os.lchflags(TESTFN_DST, stat.UF_OPAQUE) + os_helper.rmtree(TESTFN_DST) + + class TestCopyFile(unittest.TestCase): class Faux(object): diff --git a/Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst b/Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst new file mode 100644 index 00000000000..065df9bf0cf --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst @@ -0,0 +1,2 @@ +Fail fast in :func:`shutil.move()` to avoid creating destination directories on +failure.