From fe8f24ac0d1d2e103ded8dec21ab9ed06d0e4856 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Tue, 29 Dec 2020 13:32:03 -0500 Subject: [PATCH 1/8] Fail fast in shutil.move() to avoid creating destination directories on failure --- Lib/shutil.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Lib/shutil.py b/Lib/shutil.py index f0e833dc979b79..8d7e0423e8f47d 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -813,6 +813,10 @@ def move(src, dst, copy_function=copy2): if _destinsrc(src, dst): raise Error("Cannot move a directory '%s' into itself" " '%s'." % (src, dst)) + if not os.access(src, os.W_OK) and os.listdir(src): + raise Error("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) From 8826d00d05157cbb84975940bac55238a2184fd3 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Tue, 29 Dec 2020 13:47:20 -0500 Subject: [PATCH 2/8] Add NEWS entry --- .../next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst 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 00000000000000..89471c45b19a35 --- /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 shutil.move() to avoid creating destination directories on +failure. From a231ca6428611f667911c2d31a4d6371a50b230c Mon Sep 17 00:00:00 2001 From: Winson Luk Date: Tue, 12 Jan 2021 08:48:07 -0500 Subject: [PATCH 3/8] Update Misc/NEWS.d/next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst Co-authored-by: Zackery Spytz --- .../next/Library/2020-12-29-13-46-57.bpo-42782.3r0HFY.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 89471c45b19a35..065df9bf0cf427 100644 --- 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 @@ -1,2 +1,2 @@ -Fail fast in shutil.move() to avoid creating destination directories on +Fail fast in :func:`shutil.move()` to avoid creating destination directories on failure. From ee8073d353b5b383458ecca1735ed9aa717fbe89 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Sat, 30 Jan 2021 15:46:58 -0500 Subject: [PATCH 4/8] check if pytest is run as root --- Lib/test/test_shutil.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index df8dcdcce60919..976ece861c25f2 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -2084,6 +2084,22 @@ def test_move_dir_caseinsensitive(self): finally: os.rmdir(dst_dir) + def test_move_dir_permission_denied(self): + # Move a dir to another location on the same filesystem. + os.setuid(0) + ''' + dst_dir = tempfile.mktemp(dir=self.mkdtemp()) + try: + shutil.chown(self.src_dir, user=0) + # os.setuid(1000) + os.setuid(0) + with self.assertRaises(PermissionError) as pe: + self._check_move_dir(self.src_dir, dst_dir, dst_dir) + # self.assertEqual(cm.exception.filename, filename) + finally: + os_helper.rmtree(dst_dir) + ''' + class TestCopyFile(unittest.TestCase): From 1082d79c6955ec69ebfc5e17acdf4f8b1a60cf93 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Sat, 30 Jan 2021 22:03:44 -0500 Subject: [PATCH 5/8] add unit test --- Lib/shutil.py | 6 +++--- Lib/test/test_shutil.py | 26 +++++++++++++------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 8d7e0423e8f47d..c347a715249cc4 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -814,9 +814,9 @@ def move(src, dst, copy_function=copy2): raise Error("Cannot move a directory '%s' into itself" " '%s'." % (src, dst)) if not os.access(src, os.W_OK) and os.listdir(src): - raise Error("Cannot move the non-empty directory '%s': " - "Lacking write permission to '%s'." - % (src, src)) + 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) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 976ece861c25f2..94bc12e4f3044a 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -2084,21 +2084,21 @@ def test_move_dir_caseinsensitive(self): finally: os.rmdir(dst_dir) + + @unittest.skipIf(hasattr(os, 'geteuid') and os.geteuid() == 0, + 'non-root user required') def test_move_dir_permission_denied(self): - # Move a dir to another location on the same filesystem. - os.setuid(0) - ''' - dst_dir = tempfile.mktemp(dir=self.mkdtemp()) + # bpo-42782: shutil.move should not create destination directories + # if the source directory cannot be removed. + parent_dir = self.mkdtemp() + dst_dir = os.path.join(parent_dir, 'dst_dir') try: - shutil.chown(self.src_dir, user=0) - # os.setuid(1000) - os.setuid(0) - with self.assertRaises(PermissionError) as pe: - self._check_move_dir(self.src_dir, dst_dir, dst_dir) - # self.assertEqual(cm.exception.filename, filename) - finally: - os_helper.rmtree(dst_dir) - ''' + self.assertRaises(PermissionError, shutil.move, '/opt', dst_dir) + except OSError: + self.skipTest("cannot copy test source directory") + self.assertFalse(os.listdir(parent_dir)) # dst_dir should not exist + os_helper.rmtree(dst_dir) + os_helper.rmtree(parent_dir) class TestCopyFile(unittest.TestCase): From 6326e7c6059bdd5abc55787f0143ab75f8ccd4ea Mon Sep 17 00:00:00 2001 From: winsonluk Date: Sun, 31 Jan 2021 00:19:21 -0500 Subject: [PATCH 6/8] update test --- Lib/test/test_shutil.py | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 94bc12e4f3044a..c6fe10fa297638 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -34,6 +34,8 @@ 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,20 +2087,28 @@ def test_move_dir_caseinsensitive(self): os.rmdir(dst_dir) - @unittest.skipIf(hasattr(os, 'geteuid') and os.geteuid() == 0, - 'non-root user required') + @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. - parent_dir = self.mkdtemp() - dst_dir = os.path.join(parent_dir, 'dst_dir') try: - self.assertRaises(PermissionError, shutil.move, '/opt', dst_dir) - except OSError: - self.skipTest("cannot copy test source directory") - self.assertFalse(os.listdir(parent_dir)) # dst_dir should not exist - os_helper.rmtree(dst_dir) - os_helper.rmtree(parent_dir) + os.mkdir(TESTFN_SRC) + os_helper.create_empty_file(os.path.join(TESTFN_SRC, 'child')) + os.lchflags(TESTFN_SRC, stat.SF_IMMUTABLE) + self.assertRaises(PermissionError, shutil.move, TESTFN_SRC, TESTFN_DST) + # TESTFN_DST should not exist if shutil.move failed + 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): From 79d68017d22a8fd9fbd7b8ca227f176449f0cbfa Mon Sep 17 00:00:00 2001 From: winsonluk Date: Sun, 31 Jan 2021 01:46:59 -0500 Subject: [PATCH 7/8] test empty immutable directories --- Lib/shutil.py | 8 +++++++- Lib/test/test_shutil.py | 13 ++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index c347a715249cc4..233fb96f1024d5 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -813,7 +813,8 @@ def move(src, dst, copy_function=copy2): if _destinsrc(src, dst): raise Error("Cannot move a directory '%s' into itself" " '%s'." % (src, dst)) - if not os.access(src, os.W_OK) and os.listdir(src): + if ((not os.access(src, os.W_OK) and os.listdir(src)) + or _is_immutable(src)): raise PermissionError("Cannot move the non-empty directory " "'%s': Lacking write permission to '%s'." % (src, src)) @@ -834,6 +835,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 c6fe10fa297638..4bcad51509d9c4 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -2097,10 +2097,21 @@ def test_move_dir_permission_denied(self): # if the source directory cannot be removed. try: os.mkdir(TESTFN_SRC) - os_helper.create_empty_file(os.path.join(TESTFN_SRC, 'child')) 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): From 2567fb24054c11b3c94039617362e4c99d29c7d5 Mon Sep 17 00:00:00 2001 From: winsonluk Date: Sat, 20 Feb 2021 23:25:05 -0500 Subject: [PATCH 8/8] add platform check --- Lib/shutil.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 233fb96f1024d5..89d924dec8aa4e 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -813,8 +813,9 @@ def move(src, dst, copy_function=copy2): if _destinsrc(src, dst): raise Error("Cannot move a directory '%s' into itself" " '%s'." % (src, dst)) - if ((not os.access(src, os.W_OK) and os.listdir(src)) - or _is_immutable(src)): + 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))