From 55adfebdf10fd0ccaa87515bad3ab5f940c1fb87 Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 27 May 2024 21:26:26 +0100 Subject: [PATCH 1/6] GH-89727: Partially fix `shutil.rmtree()` recursion error on deep trees Make `shutil._rmtree_unsafe()` call `os.walk()`, which is implemented without recursion. `shutil._rmtree_safe_fd()` is not affected and can still raise a recursion error. --- Lib/os.py | 9 ++++++++- Lib/shutil.py | 43 ++++++++++++++--------------------------- Lib/test/test_shutil.py | 11 +++++++++++ 3 files changed, 33 insertions(+), 30 deletions(-) diff --git a/Lib/os.py b/Lib/os.py index 7661ce68ca3be2..ae9e646361e82c 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -281,6 +281,10 @@ def renames(old, new): __all__.extend(["makedirs", "removedirs", "renames"]) +# Private sentinel that makes walk() classify all symlinks and junctions as +# regular files. +_walk_symlinks_as_files = object() + def walk(top, topdown=True, onerror=None, followlinks=False): """Directory tree generator. @@ -382,7 +386,10 @@ def walk(top, topdown=True, onerror=None, followlinks=False): break try: - is_dir = entry.is_dir() + if followlinks is _walk_symlinks_as_files: + is_dir = entry.is_dir(follow_symlinks=False) and not entry.is_junction() + else: + is_dir = entry.is_dir() except OSError: # If is_dir() raises an OSError, consider the entry not to # be a directory, same behaviour as os.path.isdir(). diff --git a/Lib/shutil.py b/Lib/shutil.py index c9b4da34b1e19b..51b7ae1e7ee074 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -606,43 +606,28 @@ def _rmtree_islink(st): # version vulnerable to race conditions def _rmtree_unsafe(path, onexc): - try: - with os.scandir(path) as scandir_it: - entries = list(scandir_it) - except FileNotFoundError: - return - except OSError as err: - onexc(os.scandir, path, err) - entries = [] - for entry in entries: - fullname = entry.path - try: - is_dir = entry.is_dir(follow_symlinks=False) - except FileNotFoundError: - continue - except OSError: - is_dir = False - - if is_dir and not entry.is_junction(): + def onerror(err): + if not isinstance(err, FileNotFoundError): + onexc(os.scandir, err.filename, err) + results = os.walk(path, False, onerror, os._walk_symlinks_as_files) + for dirpath, dirnames, filenames in results: + prefix = os.path.join(dirpath, dirpath[:0]) + for name in filenames: + fullname = prefix + name try: - if entry.is_symlink(): - # This can only happen if someone replaces - # a directory with a symlink after the call to - # os.scandir or entry.is_dir above. - raise OSError("Cannot call rmtree on a symbolic link") + os.unlink(fullname) except FileNotFoundError: continue except OSError as err: - onexc(os.path.islink, fullname, err) - continue - _rmtree_unsafe(fullname, onexc) - else: + onexc(os.unlink, fullname, err) + for name in dirnames: + fullname = prefix + name try: - os.unlink(fullname) + os.rmdir(fullname) except FileNotFoundError: continue except OSError as err: - onexc(os.unlink, fullname, err) + onexc(os.rmdir, fullname, err) try: os.rmdir(path) except FileNotFoundError: diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index df9e7a660bf29e..01f139073dcd97 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -741,6 +741,17 @@ def _onexc(fn, path, exc): shutil.rmtree(TESTFN) raise + @unittest.skipIf(shutil._use_fd_functions, "fd-based functions remain unfixed (GH-89727)") + def test_rmtree_above_recursion_limit(self): + recursion_limit = 40 + # directory_depth > recursion_limit + directory_depth = recursion_limit + 10 + base = os.path.join(TESTFN, *(['d'] * directory_depth)) + os.makedirs(base) + + with support.infinite_recursion(recursion_limit): + shutil.rmtree(TESTFN) + class TestCopyTree(BaseTest, unittest.TestCase): From e5c8cebf4aaa8dd1b318c33cc8ba49d2f6bbf4cf Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 27 May 2024 21:30:45 +0100 Subject: [PATCH 2/6] Simplify diff --- Lib/shutil.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 51b7ae1e7ee074..5c6d1a0dbb23d4 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -611,23 +611,24 @@ def onerror(err): onexc(os.scandir, err.filename, err) results = os.walk(path, False, onerror, os._walk_symlinks_as_files) for dirpath, dirnames, filenames in results: - prefix = os.path.join(dirpath, dirpath[:0]) - for name in filenames: - fullname = prefix + name + # Add trailing slash to dirpath. + dirpath = os.path.join(dirpath, dirpath[:0]) + for name in dirnames: + fullname = dirpath + name try: - os.unlink(fullname) + os.rmdir(fullname) except FileNotFoundError: continue except OSError as err: - onexc(os.unlink, fullname, err) - for name in dirnames: - fullname = prefix + name + onexc(os.rmdir, fullname, err) + for name in filenames: + fullname = dirpath + name try: - os.rmdir(fullname) + os.unlink(fullname) except FileNotFoundError: continue except OSError as err: - onexc(os.rmdir, fullname, err) + onexc(os.unlink, fullname, err) try: os.rmdir(path) except FileNotFoundError: From 96293a557e6256ac030dc81011060644805cc74d Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Wed, 29 May 2024 20:35:32 +0100 Subject: [PATCH 3/6] Update Lib/shutil.py Co-authored-by: Jelle Zijlstra --- Lib/shutil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 5c6d1a0dbb23d4..64f4de9dcec9e9 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -609,7 +609,7 @@ def _rmtree_unsafe(path, onexc): def onerror(err): if not isinstance(err, FileNotFoundError): onexc(os.scandir, err.filename, err) - results = os.walk(path, False, onerror, os._walk_symlinks_as_files) + results = os.walk(path, topdown=False, onerror=onerror, followlinks=os._walk_symlinks_as_files) for dirpath, dirnames, filenames in results: # Add trailing slash to dirpath. dirpath = os.path.join(dirpath, dirpath[:0]) From 3b1b15e3024b2f7460bdeb3b9737cd536fcf3b3a Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 29 May 2024 20:36:16 +0100 Subject: [PATCH 4/6] Undo premature optimisation --- Lib/shutil.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 64f4de9dcec9e9..03a9d756030430 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -611,10 +611,8 @@ def onerror(err): onexc(os.scandir, err.filename, err) results = os.walk(path, topdown=False, onerror=onerror, followlinks=os._walk_symlinks_as_files) for dirpath, dirnames, filenames in results: - # Add trailing slash to dirpath. - dirpath = os.path.join(dirpath, dirpath[:0]) for name in dirnames: - fullname = dirpath + name + fullname = os.path.join(dirpath, name) try: os.rmdir(fullname) except FileNotFoundError: @@ -622,7 +620,7 @@ def onerror(err): except OSError as err: onexc(os.rmdir, fullname, err) for name in filenames: - fullname = dirpath + name + fullname = os.path.join(dirpath, name) try: os.unlink(fullname) except FileNotFoundError: From 4fe3075a2e654d26e999548f5cfe4ba6043c6e45 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 29 May 2024 20:42:24 +0100 Subject: [PATCH 5/6] Add NEWS --- .../next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst diff --git a/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst b/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst new file mode 100644 index 00000000000000..17b4f16d79ea74 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst @@ -0,0 +1,3 @@ +Partially fix issue with :func:`shutil.rmtree` where a :exc:`RecursionError` +is raised on deep directory trees. A recursion error is no longer raised +when :data:`rmtree.avoids_symlink_attacks` is false. From 27ee6f0b9e1d519074dd0a3c192f8b2decb67713 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Wed, 29 May 2024 20:49:14 +0100 Subject: [PATCH 6/6] Update Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst Co-authored-by: Jelle Zijlstra --- .../next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst b/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst index 17b4f16d79ea74..3b73d2789fd6f9 100644 --- a/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst +++ b/Misc/NEWS.d/next/Library/2024-05-29-20-42-17.gh-issue-89727.5lPTTW.rst @@ -1,3 +1,3 @@ Partially fix issue with :func:`shutil.rmtree` where a :exc:`RecursionError` is raised on deep directory trees. A recursion error is no longer raised -when :data:`rmtree.avoids_symlink_attacks` is false. +when :data:`!rmtree.avoids_symlink_attacks` is false.