From 339a85fa06e1d027291bc076ad759bd18459b886 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 27 Jul 2024 22:37:52 +0100 Subject: [PATCH 1/9] GH-73991: Rework `pathlib.Path.rmtree()` into `delete()` Rename `pathlib.Path.rmtree()` to `delete()`, and add support for deleting non-directories. This simplifies the interface for users, and nicely complements the upcoming `move()` and `copy()` methods (which will also accept any type of file.) --- Doc/library/pathlib.rst | 48 +++++----- Doc/whatsnew/3.14.rst | 3 +- Lib/pathlib/_abc.py | 11 +-- Lib/pathlib/_local.py | 28 ++++-- Lib/test/test_pathlib/test_pathlib.py | 74 +++++++-------- Lib/test/test_pathlib/test_pathlib_abc.py | 91 +++++++------------ ...4-05-15-01-21-44.gh-issue-73991.bNDqQN.rst | 2 +- 7 files changed, 117 insertions(+), 140 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 60c099af928123..01a804c58418a0 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1633,29 +1633,9 @@ Copying, renaming and deleting Added return value, return the new :class:`!Path` instance. -.. method:: Path.unlink(missing_ok=False) - - Remove this file or symbolic link. If the path points to a directory, - use :func:`Path.rmdir` instead. - - If *missing_ok* is false (the default), :exc:`FileNotFoundError` is - raised if the path does not exist. - - If *missing_ok* is true, :exc:`FileNotFoundError` exceptions will be - ignored (same behavior as the POSIX ``rm -f`` command). - - .. versionchanged:: 3.8 - The *missing_ok* parameter was added. - - -.. method:: Path.rmdir() - - Remove this directory. The directory must be empty. - - -.. method:: Path.rmtree(ignore_errors=False, on_error=None) +.. method:: Path.delete(ignore_errors=False, on_error=None) - Recursively delete this entire directory tree. The path must not refer to a symlink. + Recursively delete this file or directory tree. If *ignore_errors* is true, errors resulting from failed removals will be ignored. If *ignore_errors* is false or omitted, and a function is given to @@ -1666,8 +1646,8 @@ Copying, renaming and deleting .. note:: On platforms that support the necessary fd-based functions, a symlink - attack-resistant version of :meth:`~Path.rmtree` is used by default. On - other platforms, the :func:`~Path.rmtree` implementation is susceptible + attack-resistant version of :meth:`~Path.delete` is used by default. On + other platforms, the :func:`~Path.delete` implementation is susceptible to a symlink attack: given proper timing and circumstances, attackers can manipulate symlinks on the filesystem to delete files they would not be able to access otherwise. @@ -1681,6 +1661,26 @@ Copying, renaming and deleting .. versionadded:: 3.14 +.. method:: Path.unlink(missing_ok=False) + + Remove this file or symbolic link. If the path points to a directory, + use :func:`Path.rmdir` instead. + + If *missing_ok* is false (the default), :exc:`FileNotFoundError` is + raised if the path does not exist. + + If *missing_ok* is true, :exc:`FileNotFoundError` exceptions will be + ignored (same behavior as the POSIX ``rm -f`` command). + + .. versionchanged:: 3.8 + The *missing_ok* parameter was added. + + +.. method:: Path.rmdir() + + Remove this directory. The directory must be empty. + + Permissions and ownership ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/Doc/whatsnew/3.14.rst b/Doc/whatsnew/3.14.rst index 7450597e8597ad..221e7e7930d12a 100644 --- a/Doc/whatsnew/3.14.rst +++ b/Doc/whatsnew/3.14.rst @@ -141,8 +141,7 @@ pathlib :func:`shutil.copyfile`. * :meth:`~pathlib.Path.copytree` copies one directory tree to another, like :func:`shutil.copytree`. - * :meth:`~pathlib.Path.rmtree` recursively removes a directory tree, like - :func:`shutil.rmtree`. + * :meth:`~pathlib.Path.delete` removes a file or directory tree. (Contributed by Barney Gale in :gh:`73991`.) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index c32e7762cefea3..022d921f92c341 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -919,9 +919,9 @@ def rmdir(self): """ raise UnsupportedOperation(self._unsupported_msg('rmdir()')) - def rmtree(self, ignore_errors=False, on_error=None): + def delete(self, ignore_errors=False, on_error=None): """ - Recursively delete this directory tree. + Recursively delete this file or directory tree. If *ignore_errors* is true, exceptions raised from scanning the tree and removing files and directories are ignored. Otherwise, if @@ -936,10 +936,9 @@ def on_error(err): def on_error(err): raise err try: - if self.is_symlink(): - raise OSError("Cannot call rmtree on a symbolic link") - elif self.is_junction(): - raise OSError("Cannot call rmtree on a junction") + if not self.is_dir(follow_symlinks=False): + self.unlink() + return results = self.walk( on_error=on_error, top_down=False, # Bottom-up so we rmdir() empty directories. diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 4fd5279f9fe9ce..0c0a430fc23b26 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -830,9 +830,9 @@ def rmdir(self): """ os.rmdir(self) - def rmtree(self, ignore_errors=False, on_error=None): + def delete(self, ignore_errors=False, on_error=None): """ - Recursively delete this directory tree. + Recursively delete this file or directory tree. If *ignore_errors* is true, exceptions raised from scanning the tree and removing files and directories are ignored. Otherwise, if @@ -840,14 +840,24 @@ def rmtree(self, ignore_errors=False, on_error=None): *ignore_errors* nor *on_error* are set, exceptions are propagated to the caller. """ - if on_error: - def onexc(func, filename, err): - err.filename = filename - on_error(err) - else: + if self.is_dir(follow_symlinks=False): onexc = None - import shutil - shutil.rmtree(str(self), ignore_errors, onexc=onexc) + if on_error: + def onexc(func, filename, err): + err.filename = filename + on_error(err) + import shutil + shutil.rmtree(str(self), onexc=onexc) + else: + try: + self.unlink() + except OSError as err: + if not ignore_errors: + if on_error: + on_error(err) + else: + raise + def rename(self, target): """ diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 7160e764dfb2fa..a911f746276100 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -32,7 +32,7 @@ if hasattr(os, 'geteuid'): root_in_posix = (os.geteuid() == 0) -rmtree_use_fd_functions = ( +delete_use_fd_functions = ( {os.open, os.stat, os.unlink, os.rmdir} <= os.supports_dir_fd and os.listdir in os.supports_fd and os.stat in os.supports_follow_symlinks) @@ -862,8 +862,8 @@ def test_group_no_follow_symlinks(self): self.assertEqual(expected_gid, gid_2) self.assertEqual(expected_name, link.group(follow_symlinks=False)) - def test_rmtree_uses_safe_fd_version_if_available(self): - if rmtree_use_fd_functions: + def test_delete_uses_safe_fd_version_if_available(self): + if delete_use_fd_functions: d = self.cls(self.base, 'a') d.mkdir() try: @@ -876,7 +876,7 @@ def _raiser(*args, **kwargs): raise Called os.open = _raiser - self.assertRaises(Called, d.rmtree) + self.assertRaises(Called, d.delete) finally: os.open = real_open @@ -884,8 +884,8 @@ def _raiser(*args, **kwargs): "This test can't be run on Cygwin (issue #1071513).") @os_helper.skip_if_dac_override @os_helper.skip_unless_working_chmod - def test_rmtree_unwritable(self): - tmp = self.cls(self.base, 'rmtree') + def test_delete_unwritable(self): + tmp = self.cls(self.base, 'delete') tmp.mkdir() child_file_path = tmp / 'a' child_dir_path = tmp / 'b' @@ -902,7 +902,7 @@ def test_rmtree_unwritable(self): tmp.chmod(new_mode) errors = [] - tmp.rmtree(on_error=errors.append) + tmp.delete(on_error=errors.append) # Test whether onerror has actually been called. self.assertEqual(len(errors), 3) finally: @@ -911,9 +911,9 @@ def test_rmtree_unwritable(self): child_dir_path.chmod(old_child_dir_mode) @needs_windows - def test_rmtree_inner_junction(self): + def test_delete_inner_junction(self): import _winapi - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') tmp.mkdir() dir1 = tmp / 'dir1' dir2 = dir1 / 'dir2' @@ -929,15 +929,15 @@ def test_rmtree_inner_junction(self): link3 = dir1 / 'link3' _winapi.CreateJunction(str(file1), str(link3)) # make sure junctions are removed but not followed - dir1.rmtree() + dir1.delete() self.assertFalse(dir1.exists()) self.assertTrue(dir3.exists()) self.assertTrue(file1.exists()) @needs_windows - def test_rmtree_outer_junction(self): + def test_delete_outer_junction(self): import _winapi - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') tmp.mkdir() try: src = tmp / 'cheese' @@ -946,22 +946,22 @@ def test_rmtree_outer_junction(self): spam = src / 'spam' spam.write_text('') _winapi.CreateJunction(str(src), str(dst)) - self.assertRaises(OSError, dst.rmtree) - dst.rmtree(ignore_errors=True) + self.assertRaises(OSError, dst.delete) + dst.delete(ignore_errors=True) finally: - tmp.rmtree(ignore_errors=True) + tmp.delete(ignore_errors=True) @needs_windows - def test_rmtree_outer_junction_on_error(self): + def test_delete_outer_junction_on_error(self): import _winapi - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') tmp.mkdir() dir_ = tmp / 'dir' dir_.mkdir() link = tmp / 'link' _winapi.CreateJunction(str(dir_), str(link)) try: - self.assertRaises(OSError, link.rmtree) + self.assertRaises(OSError, link.delete) self.assertTrue(dir_.exists()) self.assertTrue(link.exists(follow_symlinks=False)) errors = [] @@ -969,18 +969,18 @@ def test_rmtree_outer_junction_on_error(self): def on_error(error): errors.append(error) - link.rmtree(on_error=on_error) + link.delete(on_error=on_error) self.assertEqual(len(errors), 1) self.assertIsInstance(errors[0], OSError) self.assertEqual(errors[0].filename, str(link)) finally: os.unlink(str(link)) - @unittest.skipUnless(rmtree_use_fd_functions, "requires safe rmtree") - def test_rmtree_fails_on_close(self): + @unittest.skipUnless(delete_use_fd_functions, "requires safe delete") + def test_delete_fails_on_close(self): # Test that the error handler is called for failed os.close() and that # os.close() is only called once for a file descriptor. - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') tmp.mkdir() dir1 = tmp / 'dir1' dir1.mkdir() @@ -996,7 +996,7 @@ def close(fd): close_count = 0 with swap_attr(os, 'close', close) as orig_close: with self.assertRaises(OSError): - dir1.rmtree() + dir1.delete() self.assertTrue(dir2.is_dir()) self.assertEqual(close_count, 2) @@ -1004,7 +1004,7 @@ def close(fd): errors = [] with swap_attr(os, 'close', close) as orig_close: - dir1.rmtree(on_error=errors.append) + dir1.delete(on_error=errors.append) self.assertEqual(len(errors), 2) self.assertEqual(errors[0].filename, str(dir2)) self.assertEqual(errors[1].filename, str(dir1)) @@ -1013,27 +1013,23 @@ def close(fd): @unittest.skipUnless(hasattr(os, "mkfifo"), 'requires os.mkfifo()') @unittest.skipIf(sys.platform == "vxworks", "fifo requires special path on VxWorks") - def test_rmtree_on_named_pipe(self): + def test_delete_on_named_pipe(self): p = self.cls(self.base, 'pipe') os.mkfifo(p) - try: - with self.assertRaises(NotADirectoryError): - p.rmtree() - self.assertTrue(p.exists()) - finally: - p.unlink() + p.delete() + self.assertFalse(p.exists()) p = self.cls(self.base, 'dir') p.mkdir() os.mkfifo(p / 'mypipe') - p.rmtree() + p.delete() self.assertFalse(p.exists()) @unittest.skipIf(sys.platform[:6] == 'cygwin', "This test can't be run on Cygwin (issue #1071513).") @os_helper.skip_if_dac_override @os_helper.skip_unless_working_chmod - def test_rmtree_deleted_race_condition(self): + def test_delete_deleted_race_condition(self): # bpo-37260 # # Test that a file or a directory deleted after it is enumerated @@ -1057,7 +1053,7 @@ def on_error(exc): if p != keep: p.unlink() - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') tmp.mkdir() paths = [tmp] + [tmp / f'child{i}' for i in range(6)] dirs = paths[1::2] @@ -1075,7 +1071,7 @@ def on_error(exc): path.chmod(new_mode) try: - tmp.rmtree(on_error=on_error) + tmp.delete(on_error=on_error) except: # Test failed, so cleanup artifacts. for path, mode in zip(paths, old_modes): @@ -1083,13 +1079,13 @@ def on_error(exc): path.chmod(mode) except OSError: pass - tmp.rmtree() + tmp.delete() raise - def test_rmtree_does_not_choke_on_failing_lstat(self): + def test_delete_does_not_choke_on_failing_lstat(self): try: orig_lstat = os.lstat - tmp = self.cls(self.base, 'rmtree') + tmp = self.cls(self.base, 'delete') def raiser(fn, *args, **kwargs): if fn != str(tmp): @@ -1102,7 +1098,7 @@ def raiser(fn, *args, **kwargs): tmp.mkdir() foo = tmp / 'foo' foo.write_text('') - tmp.rmtree() + tmp.delete() finally: os.lstat = orig_lstat diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index 37678c5d799e9a..443a4e989fb54b 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -2641,85 +2641,43 @@ def test_rmdir(self): self.assertFileNotFound(p.stat) self.assertFileNotFound(p.unlink) - def test_rmtree(self): + def test_delete_file(self): + p = self.cls(self.base) / 'fileA' + p.delete() + self.assertFileNotFound(p.stat) + self.assertFileNotFound(p.unlink) + + def test_delete_dir(self): base = self.cls(self.base) - base.joinpath('dirA').rmtree() + base.joinpath('dirA').delete() self.assertRaises(FileNotFoundError, base.joinpath('dirA').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirA', 'linkC').lstat) - base.joinpath('dirB').rmtree() + base.joinpath('dirB').delete() self.assertRaises(FileNotFoundError, base.joinpath('dirB').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirB', 'fileB').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirB', 'linkD').lstat) - base.joinpath('dirC').rmtree() + base.joinpath('dirC').delete() self.assertRaises(FileNotFoundError, base.joinpath('dirC').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirC', 'dirD').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirC', 'dirD', 'fileD').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirC', 'fileC').stat) self.assertRaises(FileNotFoundError, base.joinpath('dirC', 'novel.txt').stat) - def test_rmtree_errors(self): - tmp = self.cls(self.base, 'rmtree') - tmp.mkdir() - # filename is guaranteed not to exist - filename = tmp / 'foo' - self.assertRaises(FileNotFoundError, filename.rmtree) - # test that ignore_errors option is honored - filename.rmtree(ignore_errors=True) - - # existing file - filename = tmp / "tstfile" - filename.write_text("") - with self.assertRaises(NotADirectoryError) as cm: - filename.rmtree() - self.assertEqual(cm.exception.filename, str(filename)) - self.assertTrue(filename.exists()) - # test that ignore_errors option is honored - filename.rmtree(ignore_errors=True) - self.assertTrue(filename.exists()) - - def test_rmtree_on_error(self): - tmp = self.cls(self.base, 'rmtree') - tmp.mkdir() - filename = tmp / "tstfile" - filename.write_text("") - errors = [] - - def on_error(error): - errors.append(error) - - filename.rmtree(on_error=on_error) - self.assertEqual(len(errors), 2) - # First from scandir() - self.assertIsInstance(errors[0], NotADirectoryError) - self.assertEqual(errors[0].filename, str(filename)) - # Then from munlink() - self.assertIsInstance(errors[1], NotADirectoryError) - self.assertEqual(errors[1].filename, str(filename)) - @needs_symlinks - def test_rmtree_outer_symlink(self): - tmp = self.cls(self.base, 'rmtree') + def test_delete_symlink(self): + tmp = self.cls(self.base, 'delete') tmp.mkdir() dir_ = tmp / 'dir' dir_.mkdir() link = tmp / 'link' link.symlink_to(dir_) - self.assertRaises(OSError, link.rmtree) + link.delete() self.assertTrue(dir_.exists()) - self.assertTrue(link.exists(follow_symlinks=False)) - errors = [] - - def on_error(error): - errors.append(error) - - link.rmtree(on_error=on_error) - self.assertEqual(len(errors), 1) - self.assertIsInstance(errors[0], OSError) - self.assertEqual(errors[0].filename, str(link)) + self.assertFalse(link.exists(follow_symlinks=False)) @needs_symlinks - def test_rmtree_inner_symlink(self): - tmp = self.cls(self.base, 'rmtree') + def test_delete_inner_symlink(self): + tmp = self.cls(self.base, 'delete') tmp.mkdir() dir1 = tmp / 'dir1' dir2 = dir1 / 'dir2' @@ -2735,11 +2693,26 @@ def test_rmtree_inner_symlink(self): link3 = dir1 / 'link3' link3.symlink_to(file1) # make sure symlinks are removed but not followed - dir1.rmtree() + dir1.delete() self.assertFalse(dir1.exists()) self.assertTrue(dir3.exists()) self.assertTrue(file1.exists()) + def test_delete_missing(self): + tmp = self.cls(self.base, 'delete') + tmp.mkdir() + # filename is guaranteed not to exist + filename = tmp / 'foo' + self.assertRaises(FileNotFoundError, filename.delete) + # test that ignore_errors option is honored + filename.delete(ignore_errors=True) + # test on_error + errors = [] + filename.delete(on_error=errors.append) + self.assertEqual(len(errors), 1) + self.assertIsInstance(errors[0], FileNotFoundError) + self.assertEqual(errors[0].filename, str(filename)) + def setUpWalk(self): # Build: # TESTFN/ diff --git a/Misc/NEWS.d/next/Library/2024-05-15-01-21-44.gh-issue-73991.bNDqQN.rst b/Misc/NEWS.d/next/Library/2024-05-15-01-21-44.gh-issue-73991.bNDqQN.rst index 9aa7a7dba666af..5806fed91c7880 100644 --- a/Misc/NEWS.d/next/Library/2024-05-15-01-21-44.gh-issue-73991.bNDqQN.rst +++ b/Misc/NEWS.d/next/Library/2024-05-15-01-21-44.gh-issue-73991.bNDqQN.rst @@ -1 +1 @@ -Add :meth:`pathlib.Path.rmtree`, which recursively removes a directory. +Add :meth:`pathlib.Path.delete`, which recursively removes a file or directory. From 632279a49c5fc6dc22a6ea744f3a1b971ed84462 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 27 Jul 2024 23:36:19 +0100 Subject: [PATCH 2/9] Fix ignore_errors --- Lib/pathlib/_local.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 0c0a430fc23b26..6e041035d3094a 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -847,7 +847,7 @@ def onexc(func, filename, err): err.filename = filename on_error(err) import shutil - shutil.rmtree(str(self), onexc=onexc) + shutil.rmtree(str(self), ignore_errors, onexc=onexc) else: try: self.unlink() From 66609fbc8d33527dd1629069dc292c1fa9edf821 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 23:48:03 +0100 Subject: [PATCH 3/9] Docs, style. --- Doc/library/pathlib.rst | 24 ++++++++++----------- Lib/pathlib/_abc.py | 48 ++++++++++++++++++++--------------------- Lib/pathlib/_local.py | 12 +++++------ 3 files changed, 42 insertions(+), 42 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 01a804c58418a0..7cf4d3c9a36867 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1635,7 +1635,7 @@ Copying, renaming and deleting .. method:: Path.delete(ignore_errors=False, on_error=None) - Recursively delete this file or directory tree. + Recursively remove this file or directory tree. If *ignore_errors* is true, errors resulting from failed removals will be ignored. If *ignore_errors* is false or omitted, and a function is given to @@ -1643,20 +1643,20 @@ Copying, renaming and deleting *ignore_errors* nor *on_error* are supplied, exceptions are propagated to the caller. - .. note:: - - On platforms that support the necessary fd-based functions, a symlink - attack-resistant version of :meth:`~Path.delete` is used by default. On - other platforms, the :func:`~Path.delete` implementation is susceptible - to a symlink attack: given proper timing and circumstances, attackers - can manipulate symlinks on the filesystem to delete files they would not - be able to access otherwise. - If the optional argument *on_error* is specified, it should be a callable; it will be called with one argument of type :exc:`OSError`. The callable can handle the error to continue the deletion process or re-raise - it to stop. Note that the filename is available as the :attr:`~OSError.filename` - attribute of the exception object. + it to stop. Note that the filename is available as the + :attr:`~OSError.filename` attribute of the exception object. + + .. note:: + + On platforms that support the necessary fd-based functions, a symlink + attack-resistant version of :meth:`~Path.delete` is used to delete + directory trees. On other platforms, the :func:`~Path.delete` + implementation is susceptible to a symlink attack: given proper timing + and circumstances, attackers can manipulate symlinks on the filesystem + to delete files they would not be able to access otherwise. .. versionadded:: 3.14 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 022d921f92c341..45aff358de5ce3 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -921,13 +921,13 @@ def rmdir(self): def delete(self, ignore_errors=False, on_error=None): """ - Recursively delete this file or directory tree. + Recursively remove this file or directory tree. - If *ignore_errors* is true, exceptions raised from scanning the tree - and removing files and directories are ignored. Otherwise, if - *on_error* is set, it will be called to handle the error. If neither - *ignore_errors* nor *on_error* are set, exceptions are propagated to - the caller. + If *ignore_errors* is true, exceptions raised from scanning the + filesystem and removing files and directories are ignored. Otherwise, + if *on_error* is set, it will be called to handle the error. If + neither *ignore_errors* nor *on_error* are set, exceptions are + propagated to the caller. """ if ignore_errors: def on_error(err): @@ -936,25 +936,25 @@ def on_error(err): def on_error(err): raise err try: - if not self.is_dir(follow_symlinks=False): + if self.is_dir(follow_symlinks=False): + results = self.walk( + on_error=on_error, + top_down=False, # So we rmdir() empty directories. + follow_symlinks=False) + for dirpath, dirnames, filenames in results: + for name in filenames: + try: + dirpath.joinpath(name).unlink() + except OSError as err: + on_error(err) + for name in dirnames: + try: + dirpath.joinpath(name).rmdir() + except OSError as err: + on_error(err) + self.rmdir() + else: self.unlink() - return - results = self.walk( - on_error=on_error, - top_down=False, # Bottom-up so we rmdir() empty directories. - follow_symlinks=False) - for dirpath, dirnames, filenames in results: - for name in filenames: - try: - dirpath.joinpath(name).unlink() - except OSError as err: - on_error(err) - for name in dirnames: - try: - dirpath.joinpath(name).rmdir() - except OSError as err: - on_error(err) - self.rmdir() except OSError as err: err.filename = str(self) on_error(err) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 6e041035d3094a..dacb8890175091 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -832,13 +832,13 @@ def rmdir(self): def delete(self, ignore_errors=False, on_error=None): """ - Recursively delete this file or directory tree. + Recursively remove this file or directory tree. - If *ignore_errors* is true, exceptions raised from scanning the tree - and removing files and directories are ignored. Otherwise, if - *on_error* is set, it will be called to handle the error. If neither - *ignore_errors* nor *on_error* are set, exceptions are propagated to - the caller. + If *ignore_errors* is true, exceptions raised from scanning the + filesystem and removing files and directories are ignored. Otherwise, + if *on_error* is set, it will be called to handle the error. If + neither *ignore_errors* nor *on_error* are set, exceptions are + propagated to the caller. """ if self.is_dir(follow_symlinks=False): onexc = None From b7c72d06d79bfde15ba23367111e0805192cefb4 Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 29 Jul 2024 04:16:36 +0100 Subject: [PATCH 4/9] Address docs feedback. --- Doc/library/pathlib.rst | 32 ++++++++++++++------------------ Lib/pathlib/_abc.py | 2 +- Lib/pathlib/_local.py | 2 +- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 7cf4d3c9a36867..b5363d7894eae6 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1635,28 +1635,24 @@ Copying, renaming and deleting .. method:: Path.delete(ignore_errors=False, on_error=None) - Recursively remove this file or directory tree. + Delete this file or directory (including all sub-directories). - If *ignore_errors* is true, errors resulting from failed removals will be - ignored. If *ignore_errors* is false or omitted, and a function is given to - *on_error*, it will be called each time an exception is raised. If neither - *ignore_errors* nor *on_error* are supplied, exceptions are propagated to - the caller. - - If the optional argument *on_error* is specified, it should be a callable; - it will be called with one argument of type :exc:`OSError`. The - callable can handle the error to continue the deletion process or re-raise - it to stop. Note that the filename is available as the - :attr:`~OSError.filename` attribute of the exception object. + If *ignore_errors* is true, errors resulting from failed deletions will be + ignored. If *ignore_errors* is false or omitted, and a callable is given as + the optional *on_error* argument, it will be called with one argument of + type :exc:`OSError` each time an exception is raised. The callable can + handle the error to continue the deletion process or re-raise it to stop. + Note that the filename is available as the :attr:`~OSError.filename` + attribute of the exception object. If neither *ignore_errors* nor + *on_error* are supplied, exceptions are propagated to the caller. .. note:: - On platforms that support the necessary fd-based functions, a symlink - attack-resistant version of :meth:`~Path.delete` is used to delete - directory trees. On other platforms, the :func:`~Path.delete` - implementation is susceptible to a symlink attack: given proper timing - and circumstances, attackers can manipulate symlinks on the filesystem - to delete files they would not be able to access otherwise. + On platforms that lack the necessary file descriptor-based functions, + :func:`~Path.delete` implementation is susceptible to a symlink attack: + given proper timing and circumstances, attackers can manipulate symlinks + on the filesystem to delete files they would not be able to access + otherwise. .. versionadded:: 3.14 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 45aff358de5ce3..5ce04203d76d93 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -921,7 +921,7 @@ def rmdir(self): def delete(self, ignore_errors=False, on_error=None): """ - Recursively remove this file or directory tree. + Delete this file or directory (including all sub-directories). If *ignore_errors* is true, exceptions raised from scanning the filesystem and removing files and directories are ignored. Otherwise, diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index dacb8890175091..ec5d5ca7c21563 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -832,7 +832,7 @@ def rmdir(self): def delete(self, ignore_errors=False, on_error=None): """ - Recursively remove this file or directory tree. + Delete this file or directory (including all sub-directories). If *ignore_errors* is true, exceptions raised from scanning the filesystem and removing files and directories are ignored. Otherwise, From f5b788e04db4f593da6fa96a046c79630cba6a1d Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 29 Jul 2024 04:19:51 +0100 Subject: [PATCH 5/9] Shrink overlarge `try:` block. --- Lib/pathlib/_abc.py | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 5ce04203d76d93..f1cdfd66e49970 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -935,26 +935,27 @@ def on_error(err): elif on_error is None: def on_error(err): raise err + if self.is_dir(follow_symlinks=False): + results = self.walk( + on_error=on_error, + top_down=False, # So we rmdir() empty directories. + follow_symlinks=False) + for dirpath, dirnames, filenames in results: + for name in filenames: + try: + dirpath.joinpath(name).unlink() + except OSError as err: + on_error(err) + for name in dirnames: + try: + dirpath.joinpath(name).rmdir() + except OSError as err: + on_error(err) + delete_self = self.rmdir + else: + delete_self = self.unlink try: - if self.is_dir(follow_symlinks=False): - results = self.walk( - on_error=on_error, - top_down=False, # So we rmdir() empty directories. - follow_symlinks=False) - for dirpath, dirnames, filenames in results: - for name in filenames: - try: - dirpath.joinpath(name).unlink() - except OSError as err: - on_error(err) - for name in dirnames: - try: - dirpath.joinpath(name).rmdir() - except OSError as err: - on_error(err) - self.rmdir() - else: - self.unlink() + delete_self() except OSError as err: err.filename = str(self) on_error(err) From fcf1caea6541a1407d996b188026a03832c9ce78 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 30 Jul 2024 18:02:37 +0100 Subject: [PATCH 6/9] Address some review comments. --- Doc/library/pathlib.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index b5363d7894eae6..a628ea6c1fe5f5 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1635,7 +1635,8 @@ Copying, renaming and deleting .. method:: Path.delete(ignore_errors=False, on_error=None) - Delete this file or directory (including all sub-directories). + Delete this file or directory. If this path refers to a non-empty + directory, its files and sub-directories are deleted recursively. If *ignore_errors* is true, errors resulting from failed deletions will be ignored. If *ignore_errors* is false or omitted, and a callable is given as @@ -1660,7 +1661,7 @@ Copying, renaming and deleting .. method:: Path.unlink(missing_ok=False) Remove this file or symbolic link. If the path points to a directory, - use :func:`Path.rmdir` instead. + use :func:`Path.rmdir` or :func:`Path.delete` instead. If *missing_ok* is false (the default), :exc:`FileNotFoundError` is raised if the path does not exist. @@ -1674,7 +1675,8 @@ Copying, renaming and deleting .. method:: Path.rmdir() - Remove this directory. The directory must be empty. + Remove this directory. The directory must be empty; use + :meth:`Path.delete` to remove a non-empty directory. Permissions and ownership From e9835e097bf92c294f3a7e50eeadfb2087fd8559 Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 30 Jul 2024 18:08:12 +0100 Subject: [PATCH 7/9] Move method docs after rmdir and unlink We generally put the more primitive functions first. --- Doc/library/pathlib.rst | 42 ++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index a628ea6c1fe5f5..00c9319ba0277b 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1633,6 +1633,27 @@ Copying, renaming and deleting Added return value, return the new :class:`!Path` instance. +.. method:: Path.unlink(missing_ok=False) + + Remove this file or symbolic link. If the path points to a directory, + use :func:`Path.rmdir` or :func:`Path.delete` instead. + + If *missing_ok* is false (the default), :exc:`FileNotFoundError` is + raised if the path does not exist. + + If *missing_ok* is true, :exc:`FileNotFoundError` exceptions will be + ignored (same behavior as the POSIX ``rm -f`` command). + + .. versionchanged:: 3.8 + The *missing_ok* parameter was added. + + +.. method:: Path.rmdir() + + Remove this directory. The directory must be empty; use + :meth:`Path.delete` to remove a non-empty directory. + + .. method:: Path.delete(ignore_errors=False, on_error=None) Delete this file or directory. If this path refers to a non-empty @@ -1658,27 +1679,6 @@ Copying, renaming and deleting .. versionadded:: 3.14 -.. method:: Path.unlink(missing_ok=False) - - Remove this file or symbolic link. If the path points to a directory, - use :func:`Path.rmdir` or :func:`Path.delete` instead. - - If *missing_ok* is false (the default), :exc:`FileNotFoundError` is - raised if the path does not exist. - - If *missing_ok* is true, :exc:`FileNotFoundError` exceptions will be - ignored (same behavior as the POSIX ``rm -f`` command). - - .. versionchanged:: 3.8 - The *missing_ok* parameter was added. - - -.. method:: Path.rmdir() - - Remove this directory. The directory must be empty; use - :meth:`Path.delete` to remove a non-empty directory. - - Permissions and ownership ^^^^^^^^^^^^^^^^^^^^^^^^^ From b84b0e067157c7ad85301d53eec178e0dff61a42 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 3 Aug 2024 22:11:32 +0100 Subject: [PATCH 8/9] Add `Path.delete.avoids_symlink_attacks` --- Doc/library/pathlib.rst | 12 +++++++----- Lib/pathlib/_abc.py | 1 + Lib/pathlib/_local.py | 3 ++- Lib/test/test_pathlib/test_pathlib.py | 3 +++ 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 00c9319ba0277b..edc2a7f0761053 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1670,11 +1670,13 @@ Copying, renaming and deleting .. note:: - On platforms that lack the necessary file descriptor-based functions, - :func:`~Path.delete` implementation is susceptible to a symlink attack: - given proper timing and circumstances, attackers can manipulate symlinks - on the filesystem to delete files they would not be able to access - otherwise. + When deleting non-empty directories on platforms that lack the necessary + file descriptor-based functions, the :meth:`Path.delete` implementation + is susceptible to a symlink attack: given proper timing and + circumstances, attackers can manipulate symlinks on the filesystem to + delete files they would not be able to access otherwise. Applications + can use the :data:`Path.delete.avoids_symlink_attacks` method attribute + to determine whether the implementation is immune to this attack. .. versionadded:: 3.14 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index f1cdfd66e49970..a028878c073455 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -959,6 +959,7 @@ def on_error(err): except OSError as err: err.filename = str(self) on_error(err) + delete.avoids_symlink_attacks = False def owner(self, *, follow_symlinks=True): """ diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index ec5d5ca7c21563..6e2f88c93422a4 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -3,6 +3,7 @@ import operator import os import posixpath +import shutil import sys from glob import _StringGlobber from itertools import chain @@ -846,7 +847,6 @@ def delete(self, ignore_errors=False, on_error=None): def onexc(func, filename, err): err.filename = filename on_error(err) - import shutil shutil.rmtree(str(self), ignore_errors, onexc=onexc) else: try: @@ -858,6 +858,7 @@ def onexc(func, filename, err): else: raise + delete.avoids_symlink_attacks = shutil.rmtree.avoids_symlink_attacks def rename(self, target): """ diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index a911f746276100..9e922259cbaa6a 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -864,6 +864,7 @@ def test_group_no_follow_symlinks(self): def test_delete_uses_safe_fd_version_if_available(self): if delete_use_fd_functions: + self.assertTrue(self.cls.delete.avoids_symlink_attacks) d = self.cls(self.base, 'a') d.mkdir() try: @@ -879,6 +880,8 @@ def _raiser(*args, **kwargs): self.assertRaises(Called, d.delete) finally: os.open = real_open + else: + self.assertFalse(self.cls.delete.avoids_symlink_attacks) @unittest.skipIf(sys.platform[:6] == 'cygwin', "This test can't be run on Cygwin (issue #1071513).") From 1e5ad10cb79a7f38388ed8d89c111617b588cfff Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 3 Aug 2024 22:19:54 +0100 Subject: [PATCH 9/9] Missing docs def --- Doc/library/pathlib.rst | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index edc2a7f0761053..3df446a39ea8e9 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1671,13 +1671,20 @@ Copying, renaming and deleting .. note:: When deleting non-empty directories on platforms that lack the necessary - file descriptor-based functions, the :meth:`Path.delete` implementation + file descriptor-based functions, the :meth:`~Path.delete` implementation is susceptible to a symlink attack: given proper timing and circumstances, attackers can manipulate symlinks on the filesystem to delete files they would not be able to access otherwise. Applications - can use the :data:`Path.delete.avoids_symlink_attacks` method attribute + can use the :data:`~Path.delete.avoids_symlink_attacks` method attribute to determine whether the implementation is immune to this attack. + .. attribute:: delete.avoids_symlink_attacks + + Indicates whether the current platform and implementation provides a + symlink attack resistant version of :meth:`~Path.delete`. Currently + this is only true for platforms supporting fd-based directory access + functions. + .. versionadded:: 3.14