[3.12] bpo-35332: Handle os.close() errors in shutil.rmtree() (GH-23766) (GH-112763)

* Ignore os.close() errors when ignore_errors is True.
* Pass os.close() errors to the error handler if specified.
* os.close no longer retried after error.

(cherry picked from commit 11d88a178b077e42025da538b890db3151a47070)

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

diff --git a/Lib/shutil.py b/Lib/shutil.py
index 15774d23c03d24a66a9bd64b6e91745aa92a51ad..b99614fabcf4ce242e5420496d3e1ca7a4b3b1ce 100644 (file)
--- a/Lib/shutil.py
+++ b/Lib/shutil.py
@@ -674,7 +674,12 @@ def _rmtree_safe_fd(topfd, path, onexc):
                         _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 OSError as err:
                             onexc(os.rmdir, fullname, err)
@@ -689,7 +694,10 @@ def _rmtree_safe_fd(topfd, path, onexc):
                             onexc(os.path.islink, fullname, err)
                 finally:
                     if not dirfd_closed:
-                        os.close(dirfd)
+                        try:
+                            os.close(dirfd)
+                        except OSError as err:
+                            onexc(os.close, fullname, err)
         else:
             try:
                 os.unlink(entry.name, dir_fd=topfd)
@@ -765,7 +773,12 @@ def rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None):
                 _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)
@@ -777,7 +790,10 @@ def rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None):
                     onexc(os.path.islink, path, err)
         finally:
             if not fd_closed:
-                os.close(fd)
+                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")

diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py
index f7ba707e771a5925c63091771e2774d8f68e0b6b..5fd8fb497f102aa0fea37525d43899f654ed7daa 100644 (file)
--- a/Lib/test/test_shutil.py
+++ b/Lib/test/test_shutil.py
@@ -578,6 +578,41 @@ class TestRmTree(BaseTest, unittest.TestCase):
             self.assertFalse(shutil._use_fd_functions)
             self.assertFalse(shutil.rmtree.avoids_symlink_attacks)
 
+    @unittest.skipUnless(shutil._use_fd_functions, "requires safe rmtree")
+    def test_rmtree_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.mkdtemp()
+        dir1 = os.path.join(tmp, 'dir1')
+        os.mkdir(dir1)
+        dir2 = os.path.join(dir1, 'dir2')
+        os.mkdir(dir2)
+        def close(fd):
+            orig_close(fd)
+            nonlocal close_count
+            close_count += 1
+            raise OSError
+
+        close_count = 0
+        with support.swap_attr(os, 'close', close) as orig_close:
+            with self.assertRaises(OSError):
+                shutil.rmtree(dir1)
+        self.assertTrue(os.path.isdir(dir2))
+        self.assertEqual(close_count, 2)
+
+        close_count = 0
+        errors = []
+        def onexc(*args):
+            errors.append(args)
+        with support.swap_attr(os, 'close', close) as orig_close:
+            shutil.rmtree(dir1, onexc=onexc)
+        self.assertEqual(len(errors), 2)
+        self.assertIs(errors[0][0], close)
+        self.assertEqual(errors[0][1], dir2)
+        self.assertIs(errors[1][0], close)
+        self.assertEqual(errors[1][1], dir1)
+        self.assertEqual(close_count, 2)
+
     @unittest.skipUnless(shutil._use_fd_functions, "dir_fd is not supported")
     def test_rmtree_with_dir_fd(self):
         tmp_dir = self.mkdtemp()

diff --git a/Misc/NEWS.d/next/Library/2020-12-14-09-31-13.bpo-35332.s22wAx.rst b/Misc/NEWS.d/next/Library/2020-12-14-09-31-13.bpo-35332.s22wAx.rst
new file mode 100644 (file)
index 0000000..80564b9
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2020-12-14-09-31-13.bpo-35332.s22wAx.rst
@@ -0,0 +1,3 @@
+The :func:`shutil.rmtree` function now ignores errors when calling
+:func:`os.close` when *ignore_errors* is ``True``, and
+:func:`os.close` no longer retried after error.