From 417b829af9dce5edcf963e664f40461fcd816b8f Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 07:53:35 +0100 Subject: [PATCH 01/13] Tidy up `_rmtree_safe_fd()` implementation. --- Lib/shutil.py | 162 +++++++++++++++++--------------------------------- 1 file changed, 55 insertions(+), 107 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 03a9d756030430..9499df6231223a 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -635,81 +635,67 @@ def onerror(err): onexc(os.rmdir, path, err) # Version using fd-based APIs to protect against races -def _rmtree_safe_fd(topfd, path, onexc): +def _rmtree_safe_fd(is_root, dir_fd, path, name, onexc): + # Note: To guard against symlink races, we use the standard + # lstat()/open()/fstat() trick. try: - with os.scandir(topfd) as scandir_it: - entries = list(scandir_it) - except FileNotFoundError: + orig_st = os.lstat(name, dir_fd=dir_fd) + except OSError as err: + if is_root or not isinstance(err, FileNotFoundError): + onexc(os.lstat, path, err) return + try: + fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) except OSError as err: - err.filename = path - onexc(os.scandir, path, err) + if is_root or not isinstance(err, FileNotFoundError): + onexc(os.open, path, err) return - for entry in entries: - fullname = os.path.join(path, entry.name) - try: - is_dir = entry.is_dir(follow_symlinks=False) - except FileNotFoundError: - continue - except OSError: - is_dir = False - else: - if is_dir: - try: - orig_st = entry.stat(follow_symlinks=False) - is_dir = stat.S_ISDIR(orig_st.st_mode) - except FileNotFoundError: - continue - except OSError as err: - onexc(os.lstat, fullname, err) - continue - if is_dir: + + try: + if not os.path.samestat(orig_st, os.fstat(fd)): try: - dirfd = os.open(entry.name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=topfd) - dirfd_closed = False - except FileNotFoundError: - continue + # symlinks to directories are forbidden, see bug #1669 + raise OSError("Cannot call rmtree on a symbolic link") except OSError as err: - onexc(os.open, fullname, err) - else: + onexc(os.path.islink, path, err) + return + + try: + with os.scandir(fd) as scandir_it: + entries = list(scandir_it) + for entry in entries: + fullname = os.path.join(path, entry.name) try: - if os.path.samestat(orig_st, os.fstat(dirfd)): - _rmtree_safe_fd(dirfd, fullname, onexc) - try: - os.close(dirfd) - except OSError as err: - # close() should not be retried after an error. - dirfd_closed = True - onexc(os.close, fullname, err) - dirfd_closed = True - try: - os.rmdir(entry.name, dir_fd=topfd) - except FileNotFoundError: - continue - except OSError as err: - onexc(os.rmdir, fullname, err) - else: - try: - # This can only happen if someone replaces - # a directory with a symlink after the call to - # os.scandir or stat.S_ISDIR above. - raise OSError("Cannot call rmtree on a symbolic " - "link") - except OSError as err: - onexc(os.path.islink, fullname, err) - finally: - if not dirfd_closed: - try: - os.close(dirfd) - except OSError as err: - onexc(os.close, fullname, err) - else: - try: - os.unlink(entry.name, dir_fd=topfd) - except FileNotFoundError: - continue - except OSError as err: - onexc(os.unlink, fullname, err) + is_dir = entry.is_dir(follow_symlinks=False) + except OSError: + is_dir = False + if is_dir: + _rmtree_safe_fd(False, fd, fullname, entry.name, onexc) + else: + try: + os.unlink(entry.name, dir_fd=fd) + except FileNotFoundError: + continue + except OSError as err: + + if is_root or not isinstance(err, FileNotFoundError): + onexc(os.unlink, fullname, err) + except OSError as err: + if is_root or not isinstance(err, FileNotFoundError): + err.filename = path + onexc(os.scandir, path, err) + + finally: + try: + os.close(fd) + except OSError as err: + onexc(os.close, path, err) + + try: + os.rmdir(name, dir_fd=dir_fd) + except OSError as err: + if is_root or not isinstance(err, FileNotFoundError): + onexc(os.rmdir, path, err) _use_fd_functions = ({os.open, os.stat, os.unlink, os.rmdir} <= os.supports_dir_fd and @@ -762,45 +748,7 @@ def onexc(*args): # While the unsafe rmtree works fine on bytes, the fd based does not. if isinstance(path, bytes): path = os.fsdecode(path) - # Note: To guard against symlink races, we use the standard - # lstat()/open()/fstat() trick. - try: - orig_st = os.lstat(path, dir_fd=dir_fd) - except OSError as err: - onexc(os.lstat, path, err) - return - try: - fd = os.open(path, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) - fd_closed = False - except OSError as err: - onexc(os.open, path, err) - return - try: - if os.path.samestat(orig_st, os.fstat(fd)): - _rmtree_safe_fd(fd, path, onexc) - try: - os.close(fd) - except OSError as err: - # close() should not be retried after an error. - fd_closed = True - onexc(os.close, path, err) - fd_closed = True - try: - os.rmdir(path, dir_fd=dir_fd) - except OSError as err: - onexc(os.rmdir, path, err) - else: - try: - # symlinks to directories are forbidden, see bug #1669 - raise OSError("Cannot call rmtree on a symbolic link") - except OSError as err: - onexc(os.path.islink, path, err) - finally: - if not fd_closed: - try: - os.close(fd) - except OSError as err: - onexc(os.close, path, err) + _rmtree_safe_fd(True, dir_fd, path, path, onexc) else: if dir_fd is not None: raise NotImplementedError("dir_fd unavailable on this platform") From 8d42ea89661599ceebf6ae31f8262faf316c3e61 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 20:20:58 +0100 Subject: [PATCH 02/13] Rewrite using list as stack. --- Lib/shutil.py | 79 +++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 43 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 9499df6231223a..1ddd58e715b67e 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -635,32 +635,27 @@ def onerror(err): onexc(os.rmdir, path, err) # Version using fd-based APIs to protect against races -def _rmtree_safe_fd(is_root, dir_fd, path, name, onexc): - # Note: To guard against symlink races, we use the standard - # lstat()/open()/fstat() trick. +def _rmtree_safe_fd(stack, onexc): + func, dir_fd, name, path = stack.pop() try: - orig_st = os.lstat(name, dir_fd=dir_fd) - except OSError as err: - if is_root or not isinstance(err, FileNotFoundError): - onexc(os.lstat, path, err) - return - try: - fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) - except OSError as err: - if is_root or not isinstance(err, FileNotFoundError): - onexc(os.open, path, err) - return - - try: - if not os.path.samestat(orig_st, os.fstat(fd)): + if func is os.close: + os.close(dir_fd) + elif func is os.rmdir: + os.rmdir(name, dir_fd=dir_fd) + else: + assert func is os.lstat + orig_st = os.lstat(name, dir_fd=dir_fd) + func = os.open # For error reporting. + fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) try: - # symlinks to directories are forbidden, see bug #1669 - raise OSError("Cannot call rmtree on a symbolic link") - except OSError as err: - onexc(os.path.islink, path, err) - return - - try: + func = os.path.islink # For error reporting. + if not os.path.samestat(orig_st, os.fstat(fd)): + raise OSError("Cannot call rmtree on a symbolic link") + stack.append((os.rmdir, dir_fd, name, path)) + finally: + stack.append((os.close, fd, name, path)) + + func = os.scandir # For error reporting. with os.scandir(fd) as scandir_it: entries = list(scandir_it) for entry in entries: @@ -670,32 +665,20 @@ def _rmtree_safe_fd(is_root, dir_fd, path, name, onexc): except OSError: is_dir = False if is_dir: - _rmtree_safe_fd(False, fd, fullname, entry.name, onexc) + stack.append((os.lstat, fd, entry.name, fullname)) else: try: os.unlink(entry.name, dir_fd=fd) except FileNotFoundError: continue except OSError as err: + err.filename = fullname + onexc(os.unlink, fullname, err) - if is_root or not isinstance(err, FileNotFoundError): - onexc(os.unlink, fullname, err) - except OSError as err: - if is_root or not isinstance(err, FileNotFoundError): - err.filename = path - onexc(os.scandir, path, err) - - finally: - try: - os.close(fd) - except OSError as err: - onexc(os.close, path, err) - - try: - os.rmdir(name, dir_fd=dir_fd) except OSError as err: - if is_root or not isinstance(err, FileNotFoundError): - onexc(os.rmdir, path, err) + if func is os.close or name == path or not isinstance(err, FileNotFoundError): + err.filename = path + onexc(func, path, err) _use_fd_functions = ({os.open, os.stat, os.unlink, os.rmdir} <= os.supports_dir_fd and @@ -748,7 +731,17 @@ def onexc(*args): # While the unsafe rmtree works fine on bytes, the fd based does not. if isinstance(path, bytes): path = os.fsdecode(path) - _rmtree_safe_fd(True, dir_fd, path, path, onexc) + # Note: To guard against symlink races, we use the standard + # lstat()/open()/fstat() trick. + stack = [(os.lstat, dir_fd, path, path)] + try: + while stack: + _rmtree_safe_fd(stack, onexc) + finally: + while stack: + func, dir_fd, name, path = stack.pop() + if func is os.close: + os.close(dir_fd) else: if dir_fd is not None: raise NotImplementedError("dir_fd unavailable on this platform") From 70183709e3a33f1aa6a12407206a81a25c492c5e Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 21:37:25 +0100 Subject: [PATCH 03/13] Add NEWS --- .../next/Library/2024-05-30-21-37-05.gh-issue-89727.D6S9ig.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-05-30-21-37-05.gh-issue-89727.D6S9ig.rst diff --git a/Misc/NEWS.d/next/Library/2024-05-30-21-37-05.gh-issue-89727.D6S9ig.rst b/Misc/NEWS.d/next/Library/2024-05-30-21-37-05.gh-issue-89727.D6S9ig.rst new file mode 100644 index 00000000000000..854c56609acb8c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-05-30-21-37-05.gh-issue-89727.D6S9ig.rst @@ -0,0 +1,2 @@ +Fix issue with :func:`shutil.rmtree` where a :exc:`RecursionError` is raised +on deep directory trees. From 9f13194cc27edfa4d977749d413fff2d561921f2 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 21:37:34 +0100 Subject: [PATCH 04/13] Enable test --- Lib/test/test_shutil.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 01f139073dcd97..bccb81e0737c57 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -741,7 +741,6 @@ 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 From aee87db34504dd05682fcbe3ad5d0661337f27ab Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 21:56:31 +0100 Subject: [PATCH 05/13] Use DirEntry.stat() where possible. --- Lib/shutil.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 1ddd58e715b67e..2ed936eab4da2e 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -636,7 +636,7 @@ def onerror(err): # Version using fd-based APIs to protect against races def _rmtree_safe_fd(stack, onexc): - func, dir_fd, name, path = stack.pop() + func, dir_fd, name, path, entry = stack.pop() try: if func is os.close: os.close(dir_fd) @@ -644,16 +644,20 @@ def _rmtree_safe_fd(stack, onexc): os.rmdir(name, dir_fd=dir_fd) else: assert func is os.lstat - orig_st = os.lstat(name, dir_fd=dir_fd) + if entry is None: + orig_st = os.lstat(name, dir_fd=dir_fd) + else: + orig_st = entry.stat(follow_symlinks=False) + func = os.open # For error reporting. fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) try: func = os.path.islink # For error reporting. if not os.path.samestat(orig_st, os.fstat(fd)): raise OSError("Cannot call rmtree on a symbolic link") - stack.append((os.rmdir, dir_fd, name, path)) + stack.append((os.rmdir, dir_fd, name, path, None)) finally: - stack.append((os.close, fd, name, path)) + stack.append((os.close, fd, name, path, None)) func = os.scandir # For error reporting. with os.scandir(fd) as scandir_it: @@ -665,7 +669,7 @@ def _rmtree_safe_fd(stack, onexc): except OSError: is_dir = False if is_dir: - stack.append((os.lstat, fd, entry.name, fullname)) + stack.append((os.lstat, fd, entry.name, fullname, entry)) else: try: os.unlink(entry.name, dir_fd=fd) @@ -733,13 +737,13 @@ def onexc(*args): path = os.fsdecode(path) # Note: To guard against symlink races, we use the standard # lstat()/open()/fstat() trick. - stack = [(os.lstat, dir_fd, path, path)] + stack = [(os.lstat, dir_fd, path, path, None)] try: while stack: _rmtree_safe_fd(stack, onexc) finally: while stack: - func, dir_fd, name, path = stack.pop() + func, dir_fd, name, path, entry = stack.pop() if func is os.close: os.close(dir_fd) else: From d9d0386949299c2f0a3bf15fc0ca49dae2937513 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 22:37:01 +0100 Subject: [PATCH 06/13] Comments, naming, line limit. --- Lib/shutil.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 2ed936eab4da2e..360f480631bf24 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -636,7 +636,8 @@ def onerror(err): # Version using fd-based APIs to protect against races def _rmtree_safe_fd(stack, onexc): - func, dir_fd, name, path, entry = stack.pop() + func, dir_fd, path, orig_entry = stack.pop() + name = path if orig_entry is None else orig_entry.name try: if func is os.close: os.close(dir_fd) @@ -644,20 +645,21 @@ def _rmtree_safe_fd(stack, onexc): os.rmdir(name, dir_fd=dir_fd) else: assert func is os.lstat - if entry is None: + if orig_entry is None: orig_st = os.lstat(name, dir_fd=dir_fd) else: - orig_st = entry.stat(follow_symlinks=False) + orig_st = orig_entry.stat(follow_symlinks=False) func = os.open # For error reporting. fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) + + func = os.path.islink # For error reporting. try: - func = os.path.islink # For error reporting. if not os.path.samestat(orig_st, os.fstat(fd)): raise OSError("Cannot call rmtree on a symbolic link") - stack.append((os.rmdir, dir_fd, name, path, None)) + stack.append((os.rmdir, dir_fd, path, orig_entry)) finally: - stack.append((os.close, fd, name, path, None)) + stack.append((os.close, fd, path, None)) func = os.scandir # For error reporting. with os.scandir(fd) as scandir_it: @@ -669,7 +671,8 @@ def _rmtree_safe_fd(stack, onexc): except OSError: is_dir = False if is_dir: - stack.append((os.lstat, fd, entry.name, fullname, entry)) + # Traverse into sub-directory. + stack.append((os.lstat, fd, fullname, entry)) else: try: os.unlink(entry.name, dir_fd=fd) @@ -680,7 +683,7 @@ def _rmtree_safe_fd(stack, onexc): onexc(os.unlink, fullname, err) except OSError as err: - if func is os.close or name == path or not isinstance(err, FileNotFoundError): + if orig_entry is None or not isinstance(err, FileNotFoundError): err.filename = path onexc(func, path, err) @@ -737,15 +740,16 @@ def onexc(*args): path = os.fsdecode(path) # Note: To guard against symlink races, we use the standard # lstat()/open()/fstat() trick. - stack = [(os.lstat, dir_fd, path, path, None)] + stack = [(os.lstat, dir_fd, path, None)] try: while stack: _rmtree_safe_fd(stack, onexc) finally: + # Close any file descriptors still on the stack. while stack: - func, dir_fd, name, path, entry = stack.pop() + func, fd, path, entry = stack.pop() if func is os.close: - os.close(dir_fd) + os.close(fd) else: if dir_fd is not None: raise NotImplementedError("dir_fd unavailable on this platform") From 4daf2f20a5922230eab6b970a8103f9c6982f0e9 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 23:46:08 +0100 Subject: [PATCH 07/13] Restore comment. --- Lib/shutil.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/shutil.py b/Lib/shutil.py index 360f480631bf24..ef7956349410e8 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -656,6 +656,7 @@ def _rmtree_safe_fd(stack, onexc): func = os.path.islink # For error reporting. try: if not os.path.samestat(orig_st, os.fstat(fd)): + # Symlinks to directories are forbidden, see GH-46010. raise OSError("Cannot call rmtree on a symbolic link") stack.append((os.rmdir, dir_fd, path, orig_entry)) finally: From a335a63d1e81640e86b9302fcf152c340b2a43b6 Mon Sep 17 00:00:00 2001 From: barneygale Date: Thu, 30 May 2024 23:53:41 +0100 Subject: [PATCH 08/13] Simplify overall diff --- Lib/shutil.py | 76 +++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index ef7956349410e8..435e297a7e70e4 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -636,52 +636,52 @@ def onerror(err): # Version using fd-based APIs to protect against races def _rmtree_safe_fd(stack, onexc): - func, dir_fd, path, orig_entry = stack.pop() + func, dirfd, path, orig_entry = stack.pop() name = path if orig_entry is None else orig_entry.name try: if func is os.close: - os.close(dir_fd) - elif func is os.rmdir: - os.rmdir(name, dir_fd=dir_fd) + os.close(dirfd) + return + if func is os.rmdir: + os.rmdir(name, dir_fd=dirfd) + return + + assert func is os.lstat + if orig_entry is None: + orig_st = os.lstat(name, dir_fd=dirfd) else: - assert func is os.lstat - if orig_entry is None: - orig_st = os.lstat(name, dir_fd=dir_fd) - else: - orig_st = orig_entry.stat(follow_symlinks=False) + orig_st = orig_entry.stat(follow_symlinks=False) - func = os.open # For error reporting. - fd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dir_fd) + func = os.open # For error reporting. + topfd = os.open(name, os.O_RDONLY | os.O_NONBLOCK, dir_fd=dirfd) - func = os.path.islink # For error reporting. + func = os.path.islink # For error reporting. + try: + if not os.path.samestat(orig_st, os.fstat(topfd)): + # Symlinks to directories are forbidden, see GH-46010. + raise OSError("Cannot call rmtree on a symbolic link") + stack.append((os.rmdir, dirfd, path, orig_entry)) + finally: + stack.append((os.close, topfd, path, None)) + + func = os.scandir # For error reporting. + with os.scandir(topfd) as scandir_it: + entries = list(scandir_it) + for entry in entries: + fullname = os.path.join(path, entry.name) try: - if not os.path.samestat(orig_st, os.fstat(fd)): - # Symlinks to directories are forbidden, see GH-46010. - raise OSError("Cannot call rmtree on a symbolic link") - stack.append((os.rmdir, dir_fd, path, orig_entry)) - finally: - stack.append((os.close, fd, path, None)) - - func = os.scandir # For error reporting. - with os.scandir(fd) as scandir_it: - entries = list(scandir_it) - for entry in entries: - fullname = os.path.join(path, entry.name) - try: - is_dir = entry.is_dir(follow_symlinks=False) - except OSError: - is_dir = False - if is_dir: + if entry.is_dir(follow_symlinks=False): # Traverse into sub-directory. - stack.append((os.lstat, fd, fullname, entry)) - else: - try: - os.unlink(entry.name, dir_fd=fd) - except FileNotFoundError: - continue - except OSError as err: - err.filename = fullname - onexc(os.unlink, fullname, err) + stack.append((os.lstat, topfd, fullname, entry)) + continue + except OSError: + pass + try: + os.unlink(entry.name, dir_fd=topfd) + except FileNotFoundError: + continue + except OSError as err: + onexc(os.unlink, fullname, err) except OSError as err: if orig_entry is None or not isinstance(err, FileNotFoundError): From 1d4772aece7e38e78b5fab37491e07c4a08127a5 Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 31 May 2024 12:54:15 +0100 Subject: [PATCH 09/13] Address some review feedback --- Lib/shutil.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 435e297a7e70e4..e783c0782214f6 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -646,6 +646,8 @@ def _rmtree_safe_fd(stack, onexc): os.rmdir(name, dir_fd=dirfd) return + # Note: To guard against symlink races, we use the standard + # lstat()/open()/fstat() trick. assert func is os.lstat if orig_entry is None: orig_st = os.lstat(name, dir_fd=dirfd) @@ -682,11 +684,13 @@ def _rmtree_safe_fd(stack, onexc): continue except OSError as err: onexc(os.unlink, fullname, err) - - except OSError as err: - if orig_entry is None or not isinstance(err, FileNotFoundError): + except FileNotFoundError as err: + if orig_entry is None: err.filename = path onexc(func, path, err) + except OSError as err: + err.filename = path + onexc(func, path, err) _use_fd_functions = ({os.open, os.stat, os.unlink, os.rmdir} <= os.supports_dir_fd and @@ -739,8 +743,6 @@ def onexc(*args): # While the unsafe rmtree works fine on bytes, the fd based does not. if isinstance(path, bytes): path = os.fsdecode(path) - # Note: To guard against symlink races, we use the standard - # lstat()/open()/fstat() trick. stack = [(os.lstat, dir_fd, path, None)] try: while stack: From 462cfc13ef85015804f6b0953506573c0ccfb554 Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 31 May 2024 13:01:29 +0100 Subject: [PATCH 10/13] Handle further errors in finally: block --- Lib/shutil.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index e783c0782214f6..0349ac78b239b1 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -751,8 +751,12 @@ def onexc(*args): # Close any file descriptors still on the stack. while stack: func, fd, path, entry = stack.pop() - if func is os.close: + if func is not os.close: + continue + try: os.close(fd) + except OSError as err: + onexc(os.close, path, err) else: if dir_fd is not None: raise NotImplementedError("dir_fd unavailable on this platform") From 8d7ba8386304d34ddcb0f39837584302dd9eb184 Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 31 May 2024 13:29:59 +0100 Subject: [PATCH 11/13] Comments. --- Lib/shutil.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Lib/shutil.py b/Lib/shutil.py index 0349ac78b239b1..1b9ac2ae1f6439 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -636,6 +636,19 @@ def onerror(err): # Version using fd-based APIs to protect against races def _rmtree_safe_fd(stack, onexc): + # Each stack item has four elements: + # * func: The first operation to perform: os.lstat, os.close or os.rmdir. + # Walking a directory starts with an os.lstat() to detect symlinks; in + # this case, func is updated before subsequent operations and passed to + # onexc() if an error occurs. + # * dirfd: Open file descriptor, or None if we're processing the top-level + # directory given to rmtree() and the user didn't supply dir_fd. + # * path: Path of file to operate upon. This is passed to onexc() if an + # error occurs. + # * orig_entry: os.DirEntry, or None if we're processing the top-level + # directory given to rmtree(), or the func is os.close. We used the + # cached stat() of the entry to save a call to os.lstat() when walking + # interior directories. func, dirfd, path, orig_entry = stack.pop() name = path if orig_entry is None else orig_entry.name try: @@ -662,8 +675,14 @@ def _rmtree_safe_fd(stack, onexc): if not os.path.samestat(orig_st, os.fstat(topfd)): # Symlinks to directories are forbidden, see GH-46010. raise OSError("Cannot call rmtree on a symbolic link") + # Schedule the directory to be removed, now that we've established + # it isn't a symlink. stack.append((os.rmdir, dirfd, path, orig_entry)) finally: + # Schedule the file descriptor to be closed. We omit orig_entry so + # that any FileNotFoundError from os.close() is reported to + # onexc(); normally FileNotFoundError is suppressed for operations + # on subdirectories, but os.close() is an exception. stack.append((os.close, topfd, path, None)) func = os.scandir # For error reporting. From 11deeabb9dfa27b64d00dcfaca610bcb36f5cff4 Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 31 May 2024 13:35:15 +0100 Subject: [PATCH 12/13] Handle os.close explicitly --- Lib/shutil.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/Lib/shutil.py b/Lib/shutil.py index 1b9ac2ae1f6439..1350391e1882b4 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -646,9 +646,8 @@ def _rmtree_safe_fd(stack, onexc): # * path: Path of file to operate upon. This is passed to onexc() if an # error occurs. # * orig_entry: os.DirEntry, or None if we're processing the top-level - # directory given to rmtree(), or the func is os.close. We used the - # cached stat() of the entry to save a call to os.lstat() when walking - # interior directories. + # directory given to rmtree(). We used the cached stat() of the entry to + # save a call to os.lstat() when walking subdirectories. func, dirfd, path, orig_entry = stack.pop() name = path if orig_entry is None else orig_entry.name try: @@ -675,15 +674,9 @@ def _rmtree_safe_fd(stack, onexc): if not os.path.samestat(orig_st, os.fstat(topfd)): # Symlinks to directories are forbidden, see GH-46010. raise OSError("Cannot call rmtree on a symbolic link") - # Schedule the directory to be removed, now that we've established - # it isn't a symlink. stack.append((os.rmdir, dirfd, path, orig_entry)) finally: - # Schedule the file descriptor to be closed. We omit orig_entry so - # that any FileNotFoundError from os.close() is reported to - # onexc(); normally FileNotFoundError is suppressed for operations - # on subdirectories, but os.close() is an exception. - stack.append((os.close, topfd, path, None)) + stack.append((os.close, topfd, path, orig_entry)) func = os.scandir # For error reporting. with os.scandir(topfd) as scandir_it: @@ -704,7 +697,7 @@ def _rmtree_safe_fd(stack, onexc): except OSError as err: onexc(os.unlink, fullname, err) except FileNotFoundError as err: - if orig_entry is None: + if orig_entry is None or func is os.close: err.filename = path onexc(func, path, err) except OSError as err: From a53e4736d6ce28cc11e68b63b69fec3951831dcb Mon Sep 17 00:00:00 2001 From: barneygale Date: Fri, 31 May 2024 20:41:12 +0100 Subject: [PATCH 13/13] Skip os.unlink() if os.DirEntry.is_dir() raises FileNotFoundError --- Lib/shutil.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/shutil.py b/Lib/shutil.py index 1350391e1882b4..b0d49e98cfe5f9 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -688,6 +688,8 @@ def _rmtree_safe_fd(stack, onexc): # Traverse into sub-directory. stack.append((os.lstat, topfd, fullname, entry)) continue + except FileNotFoundError: + continue except OSError: pass try: