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

* 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.

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

Lib/shutil.py

@@ -687,7 +687,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 FileNotFoundError:
  continue
@@ -704,7 +709,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)
@@ -782,7 +790,12 @@ def onexc(*args):
  _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)
@@ -794,7 +807,10 @@ def onexc(*args):
  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")

Lib/test/test_shutil.py

@@ -576,6 +576,41 @@ def _raiser(*args, **kwargs):
  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()

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.