gh-102828: add onexc arg to shutil.rmtree. Deprecate onerror. (#102829)

5 files changed, 256 insertions, 56 deletions

diff --git a/Doc/library/shutil.rst b/Doc/library/shutil.rst
index b33dbe2..acba662 100644
--- a/Doc/library/shutil.rst
+++ b/Doc/library/shutil.rst
@@ -292,15 +292,15 @@ Directory and files operations
    .. versionadded:: 3.8
       The *dirs_exist_ok* parameter.
 
-.. function:: rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None)
+.. function:: rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None)
 
    .. index:: single: directory; deleting
 
    Delete an entire directory tree; *path* must point to a directory (but not a
    symbolic link to a directory).  If *ignore_errors* is true, errors resulting
    from failed removals will be ignored; if false or omitted, such errors are
-   handled by calling a handler specified by *onerror* or, if that is omitted,
-   they raise an exception.
+   handled by calling a handler specified by *onexc* or *onerror* or, if both
+   are omitted, exceptions are propagated to the caller.
 
    This function can support :ref:`paths relative to directory descriptors
    <dir_fd>`.
@@ -315,14 +315,17 @@ Directory and files operations
       otherwise.  Applications can use the :data:`rmtree.avoids_symlink_attacks`
       function attribute to determine which case applies.
 
-   If *onerror* is provided, it must be a callable that accepts three
-   parameters: *function*, *path*, and *excinfo*.
+   If *onexc* is provided, it must be a callable that accepts three parameters:
+   *function*, *path*, and *excinfo*.
 
    The first parameter, *function*, is the function which raised the exception;
    it depends on the platform and implementation.  The second parameter,
    *path*, will be the path name passed to *function*.  The third parameter,
-   *excinfo*, will be the exception information returned by
-   :func:`sys.exc_info`.  Exceptions raised by *onerror* will not be caught.
+   *excinfo*, is the exception that was raised. Exceptions raised by *onexc*
+   will not be caught.
+
+   The deprecated *onerror* is similar to *onexc*, except that the third
+   parameter it receives is the tuple returned from :func:`sys.exc_info`.
 
    .. audit-event:: shutil.rmtree path,dir_fd shutil.rmtree
 
@@ -337,6 +340,9 @@ Directory and files operations
    .. versionchanged:: 3.11
       The *dir_fd* parameter.
 
+   .. versionchanged:: 3.12
+      Added the *onexc* parameter, deprecated *onerror*.
+
    .. attribute:: rmtree.avoids_symlink_attacks
 
       Indicates whether the current platform and implementation provides a
@@ -509,7 +515,7 @@ rmtree example
 ~~~~~~~~~~~~~~
 
 This example shows how to remove a directory tree on Windows where some
-of the files have their read-only bit set. It uses the onerror callback
+of the files have their read-only bit set. It uses the onexc callback
 to clear the readonly bit and reattempt the remove. Any subsequent failure
 will propagate. ::
 
@@ -521,7 +527,7 @@ will propagate. ::
         os.chmod(path, stat.S_IWRITE)
         func(path)
 
-    shutil.rmtree(directory, onerror=remove_readonly)
+    shutil.rmtree(directory, onexc=remove_readonly)
 
 .. _archiving-operations:
 
diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst
index 32fec96..a36e685 100644
--- a/Doc/whatsnew/3.12.rst
+++ b/Doc/whatsnew/3.12.rst
@@ -337,6 +337,11 @@ shutil
   of the process to *root_dir* to perform archiving.
   (Contributed by Serhiy Storchaka in :gh:`74696`.)
 
+* :func:`shutil.rmtree` now accepts a new argument *onexc* which is an
+  error handler like *onerror* but which expects an exception instance
+  rather than a *(typ, val, tb)* triplet. *onerror* is deprecated.
+  (Contributed by Irit Katriel in :gh:`102828`.)
+
 
 sqlite3
 -------
@@ -498,6 +503,10 @@ Deprecated
   fields are deprecated. Use :data:`sys.last_exc` instead.
   (Contributed by Irit Katriel in :gh:`102778`.)
 
+* The *onerror* argument of :func:`shutil.rmtree` is deprecated. Use *onexc*
+  instead. (Contributed by Irit Katriel in :gh:`102828`.)
+
+
 Pending Removal in Python 3.13
 ------------------------------
 
diff --git a/Lib/shutil.py b/Lib/shutil.py
index 867925a..89a7ac7 100644
--- a/Lib/shutil.py
+++ b/Lib/shutil.py
@@ -575,12 +575,12 @@ else:
         return os.path.islink(path)
 
 # version vulnerable to race conditions
-def _rmtree_unsafe(path, onerror):
+def _rmtree_unsafe(path, onexc):
     try:
         with os.scandir(path) as scandir_it:
             entries = list(scandir_it)
-    except OSError:
-        onerror(os.scandir, path, sys.exc_info())
+    except OSError as err:
+        onexc(os.scandir, path, err)
         entries = []
     for entry in entries:
         fullname = entry.path
@@ -596,28 +596,28 @@ def _rmtree_unsafe(path, onerror):
                     # 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")
-            except OSError:
-                onerror(os.path.islink, fullname, sys.exc_info())
+            except OSError as err:
+                onexc(os.path.islink, fullname, err)
                 continue
-            _rmtree_unsafe(fullname, onerror)
+            _rmtree_unsafe(fullname, onexc)
         else:
             try:
                 os.unlink(fullname)
-            except OSError:
-                onerror(os.unlink, fullname, sys.exc_info())
+            except OSError as err:
+                onexc(os.unlink, fullname, err)
     try:
         os.rmdir(path)
-    except OSError:
-        onerror(os.rmdir, path, sys.exc_info())
+    except OSError as err:
+        onexc(os.rmdir, path, err)
 
 # Version using fd-based APIs to protect against races
-def _rmtree_safe_fd(topfd, path, onerror):
+def _rmtree_safe_fd(topfd, path, onexc):
     try:
         with os.scandir(topfd) as scandir_it:
             entries = list(scandir_it)
     except OSError as err:
         err.filename = path
-        onerror(os.scandir, path, sys.exc_info())
+        onexc(os.scandir, path, err)
         return
     for entry in entries:
         fullname = os.path.join(path, entry.name)
@@ -630,25 +630,25 @@ def _rmtree_safe_fd(topfd, path, onerror):
                 try:
                     orig_st = entry.stat(follow_symlinks=False)
                     is_dir = stat.S_ISDIR(orig_st.st_mode)
-                except OSError:
-                    onerror(os.lstat, fullname, sys.exc_info())
+                except OSError as err:
+                    onexc(os.lstat, fullname, err)
                     continue
         if is_dir:
             try:
                 dirfd = os.open(entry.name, os.O_RDONLY, dir_fd=topfd)
                 dirfd_closed = False
-            except OSError:
-                onerror(os.open, fullname, sys.exc_info())
+            except OSError as err:
+                onexc(os.open, fullname, err)
             else:
                 try:
                     if os.path.samestat(orig_st, os.fstat(dirfd)):
-                        _rmtree_safe_fd(dirfd, fullname, onerror)
+                        _rmtree_safe_fd(dirfd, fullname, onexc)
                         try:
                             os.close(dirfd)
                             dirfd_closed = True
                             os.rmdir(entry.name, dir_fd=topfd)
-                        except OSError:
-                            onerror(os.rmdir, fullname, sys.exc_info())
+                        except OSError as err:
+                            onexc(os.rmdir, fullname, err)
                     else:
                         try:
                             # This can only happen if someone replaces
@@ -656,23 +656,23 @@ def _rmtree_safe_fd(topfd, path, onerror):
                             # os.scandir or stat.S_ISDIR above.
                             raise OSError("Cannot call rmtree on a symbolic "
                                           "link")
-                        except OSError:
-                            onerror(os.path.islink, fullname, sys.exc_info())
+                        except OSError as err:
+                            onexc(os.path.islink, fullname, err)
                 finally:
                     if not dirfd_closed:
                         os.close(dirfd)
         else:
             try:
                 os.unlink(entry.name, dir_fd=topfd)
-            except OSError:
-                onerror(os.unlink, fullname, sys.exc_info())
+            except OSError as err:
+                onexc(os.unlink, fullname, err)
 
 _use_fd_functions = ({os.open, os.stat, os.unlink, os.rmdir} <=
                      os.supports_dir_fd and
                      os.scandir in os.supports_fd and
                      os.stat in os.supports_follow_symlinks)
 
-def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
+def rmtree(path, ignore_errors=False, onerror=None, *, onexc=None, dir_fd=None):
     """Recursively delete a directory tree.
 
     If dir_fd is not None, it should be a file descriptor open to a directory;
@@ -680,21 +680,39 @@ def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
     dir_fd may not be implemented on your platform.
     If it is unavailable, using it will raise a NotImplementedError.
 
-    If ignore_errors is set, errors are ignored; otherwise, if onerror
-    is set, it is called to handle the error with arguments (func,
+    If ignore_errors is set, errors are ignored; otherwise, if onexc or
+    onerror is set, it is called to handle the error with arguments (func,
     path, exc_info) where func is platform and implementation dependent;
     path is the argument to that function that caused it to fail; and
-    exc_info is a tuple returned by sys.exc_info().  If ignore_errors
-    is false and onerror is None, an exception is raised.
+    the value of exc_info describes the exception. For onexc it is the
+    exception instance, and for onerror it is a tuple as returned by
+    sys.exc_info().  If ignore_errors is false and both onexc and
+    onerror are None, the exception is reraised.
 
+    onerror is deprecated and only remains for backwards compatibility.
+    If both onerror and onexc are set, onerror is ignored and onexc is used.
     """
     sys.audit("shutil.rmtree", path, dir_fd)
     if ignore_errors:
-        def onerror(*args):
+        def onexc(*args):
             pass
-    elif onerror is None:
-        def onerror(*args):
+    elif onerror is None and onexc is None:
+        def onexc(*args):
             raise
+    elif onexc is None:
+        if onerror is None:
+            def onexc(*args):
+                raise
+        else:
+            # delegate to onerror
+            def onexc(*args):
+                func, path, exc = args
+                if exc is None:
+                    exc_info = None, None, None
+                else:
+                    exc_info = type(exc), exc, exc.__traceback__
+                return onerror(func, path, exc_info)
+
     if _use_fd_functions:
         # While the unsafe rmtree works fine on bytes, the fd based does not.
         if isinstance(path, bytes):
@@ -703,30 +721,30 @@ def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
         # lstat()/open()/fstat() trick.
         try:
             orig_st = os.lstat(path, dir_fd=dir_fd)
-        except Exception:
-            onerror(os.lstat, path, sys.exc_info())
+        except Exception as err:
+            onexc(os.lstat, path, err)
             return
         try:
             fd = os.open(path, os.O_RDONLY, dir_fd=dir_fd)
             fd_closed = False
-        except Exception:
-            onerror(os.open, path, sys.exc_info())
+        except Exception as err:
+            onexc(os.open, path, err)
             return
         try:
             if os.path.samestat(orig_st, os.fstat(fd)):
-                _rmtree_safe_fd(fd, path, onerror)
+                _rmtree_safe_fd(fd, path, onexc)
                 try:
                     os.close(fd)
                     fd_closed = True
                     os.rmdir(path, dir_fd=dir_fd)
-                except OSError:
-                    onerror(os.rmdir, path, sys.exc_info())
+                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:
-                    onerror(os.path.islink, path, sys.exc_info())
+                except OSError as err:
+                    onexc(os.path.islink, path, err)
         finally:
             if not fd_closed:
                 os.close(fd)
@@ -737,11 +755,11 @@ def rmtree(path, ignore_errors=False, onerror=None, *, dir_fd=None):
             if _rmtree_islink(path):
                 # symlinks to directories are forbidden, see bug #1669
                 raise OSError("Cannot call rmtree on a symbolic link")
-        except OSError:
-            onerror(os.path.islink, path, sys.exc_info())
-            # can't continue even if onerror hook returns
+        except OSError as err:
+            onexc(os.path.islink, path, err)
+            # can't continue even if onexc hook returns
             return
-        return _rmtree_unsafe(path, onerror)
+        return _rmtree_unsafe(path, onexc)
 
 # Allow introspection of whether or not the hardening against symlink
 # attacks is supported on the current platform
diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py
index 8fe6221..fee3e7f 100644
--- a/Lib/test/test_shutil.py
+++ b/Lib/test/test_shutil.py
@@ -195,7 +195,7 @@ class TestRmTree(BaseTest, unittest.TestCase):
         shutil.rmtree(victim)
 
     @os_helper.skip_unless_symlink
-    def test_rmtree_fails_on_symlink(self):
+    def test_rmtree_fails_on_symlink_onerror(self):
         tmp = self.mkdtemp()
         dir_ = os.path.join(tmp, 'dir')
         os.mkdir(dir_)
@@ -214,6 +214,25 @@ class TestRmTree(BaseTest, unittest.TestCase):
         self.assertIsInstance(errors[0][2][1], OSError)
 
     @os_helper.skip_unless_symlink
+    def test_rmtree_fails_on_symlink_onexc(self):
+        tmp = self.mkdtemp()
+        dir_ = os.path.join(tmp, 'dir')
+        os.mkdir(dir_)
+        link = os.path.join(tmp, 'link')
+        os.symlink(dir_, link)
+        self.assertRaises(OSError, shutil.rmtree, link)
+        self.assertTrue(os.path.exists(dir_))
+        self.assertTrue(os.path.lexists(link))
+        errors = []
+        def onexc(*args):
+            errors.append(args)
+        shutil.rmtree(link, onexc=onexc)
+        self.assertEqual(len(errors), 1)
+        self.assertIs(errors[0][0], os.path.islink)
+        self.assertEqual(errors[0][1], link)
+        self.assertIsInstance(errors[0][2], OSError)
+
+    @os_helper.skip_unless_symlink
     def test_rmtree_works_on_symlinks(self):
         tmp = self.mkdtemp()
         dir1 = os.path.join(tmp, 'dir1')
@@ -236,7 +255,7 @@ class TestRmTree(BaseTest, unittest.TestCase):
         self.assertTrue(os.path.exists(file1))
 
     @unittest.skipUnless(_winapi, 'only relevant on Windows')
-    def test_rmtree_fails_on_junctions(self):
+    def test_rmtree_fails_on_junctions_onerror(self):
         tmp = self.mkdtemp()
         dir_ = os.path.join(tmp, 'dir')
         os.mkdir(dir_)
@@ -256,6 +275,26 @@ class TestRmTree(BaseTest, unittest.TestCase):
         self.assertIsInstance(errors[0][2][1], OSError)
 
     @unittest.skipUnless(_winapi, 'only relevant on Windows')
+    def test_rmtree_fails_on_junctions_onexc(self):
+        tmp = self.mkdtemp()
+        dir_ = os.path.join(tmp, 'dir')
+        os.mkdir(dir_)
+        link = os.path.join(tmp, 'link')
+        _winapi.CreateJunction(dir_, link)
+        self.addCleanup(os_helper.unlink, link)
+        self.assertRaises(OSError, shutil.rmtree, link)
+        self.assertTrue(os.path.exists(dir_))
+        self.assertTrue(os.path.lexists(link))
+        errors = []
+        def onexc(*args):
+            errors.append(args)
+        shutil.rmtree(link, onexc=onexc)
+        self.assertEqual(len(errors), 1)
+        self.assertIs(errors[0][0], os.path.islink)
+        self.assertEqual(errors[0][1], link)
+        self.assertIsInstance(errors[0][2], OSError)
+
+    @unittest.skipUnless(_winapi, 'only relevant on Windows')
     def test_rmtree_works_on_junctions(self):
         tmp = self.mkdtemp()
         dir1 = os.path.join(tmp, 'dir1')
@@ -277,7 +316,7 @@ class TestRmTree(BaseTest, unittest.TestCase):
         self.assertTrue(os.path.exists(dir3))
         self.assertTrue(os.path.exists(file1))
 
-    def test_rmtree_errors(self):
+    def test_rmtree_errors_onerror(self):
         # filename is guaranteed not to exist
         filename = tempfile.mktemp(dir=self.mkdtemp())
         self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
@@ -309,6 +348,37 @@ class TestRmTree(BaseTest, unittest.TestCase):
         self.assertIsInstance(errors[1][2][1], NotADirectoryError)
         self.assertEqual(errors[1][2][1].filename, filename)
 
+    def test_rmtree_errors_onexc(self):
+        # filename is guaranteed not to exist
+        filename = tempfile.mktemp(dir=self.mkdtemp())
+        self.assertRaises(FileNotFoundError, shutil.rmtree, filename)
+        # test that ignore_errors option is honored
+        shutil.rmtree(filename, ignore_errors=True)
+
+        # existing file
+        tmpdir = self.mkdtemp()
+        write_file((tmpdir, "tstfile"), "")
+        filename = os.path.join(tmpdir, "tstfile")
+        with self.assertRaises(NotADirectoryError) as cm:
+            shutil.rmtree(filename)
+        self.assertEqual(cm.exception.filename, filename)
+        self.assertTrue(os.path.exists(filename))
+        # test that ignore_errors option is honored
+        shutil.rmtree(filename, ignore_errors=True)
+        self.assertTrue(os.path.exists(filename))
+        errors = []
+        def onexc(*args):
+            errors.append(args)
+        shutil.rmtree(filename, onexc=onexc)
+        self.assertEqual(len(errors), 2)
+        self.assertIs(errors[0][0], os.scandir)
+        self.assertEqual(errors[0][1], filename)
+        self.assertIsInstance(errors[0][2], NotADirectoryError)
+        self.assertEqual(errors[0][2].filename, filename)
+        self.assertIs(errors[1][0], os.rmdir)
+        self.assertEqual(errors[1][1], filename)
+        self.assertIsInstance(errors[1][2], NotADirectoryError)
+        self.assertEqual(errors[1][2].filename, filename)
 
     @unittest.skipIf(sys.platform[:6] == 'cygwin',
                      "This test can't be run on Cygwin (issue #1071513).")
@@ -368,6 +438,100 @@ class TestRmTree(BaseTest, unittest.TestCase):
             self.assertTrue(issubclass(exc[0], OSError))
             self.errorState = 3
 
+    @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_on_exc(self):
+        self.errorState = 0
+        os.mkdir(TESTFN)
+        self.addCleanup(shutil.rmtree, TESTFN)
+
+        self.child_file_path = os.path.join(TESTFN, 'a')
+        self.child_dir_path = os.path.join(TESTFN, 'b')
+        os_helper.create_empty_file(self.child_file_path)
+        os.mkdir(self.child_dir_path)
+        old_dir_mode = os.stat(TESTFN).st_mode
+        old_child_file_mode = os.stat(self.child_file_path).st_mode
+        old_child_dir_mode = os.stat(self.child_dir_path).st_mode
+        # Make unwritable.
+        new_mode = stat.S_IREAD|stat.S_IEXEC
+        os.chmod(self.child_file_path, new_mode)
+        os.chmod(self.child_dir_path, new_mode)
+        os.chmod(TESTFN, new_mode)
+
+        self.addCleanup(os.chmod, TESTFN, old_dir_mode)
+        self.addCleanup(os.chmod, self.child_file_path, old_child_file_mode)
+        self.addCleanup(os.chmod, self.child_dir_path, old_child_dir_mode)
+
+        shutil.rmtree(TESTFN, onexc=self.check_args_to_onexc)
+        # Test whether onexc has actually been called.
+        self.assertEqual(self.errorState, 3,
+                         "Expected call to onexc function did not happen.")
+
+    def check_args_to_onexc(self, func, arg, exc):
+        # test_rmtree_errors deliberately runs rmtree
+        # on a directory that is chmod 500, which will fail.
+        # This function is run when shutil.rmtree fails.
+        # 99.9% of the time it initially fails to remove
+        # a file in the directory, so the first time through
+        # func is os.remove.
+        # However, some Linux machines running ZFS on
+        # FUSE experienced a failure earlier in the process
+        # at os.listdir.  The first failure may legally
+        # be either.
+        if self.errorState < 2:
+            if func is os.unlink:
+                self.assertEqual(arg, self.child_file_path)
+            elif func is os.rmdir:
+                self.assertEqual(arg, self.child_dir_path)
+            else:
+                self.assertIs(func, os.listdir)
+                self.assertIn(arg, [TESTFN, self.child_dir_path])
+            self.assertTrue(isinstance(exc, OSError))
+            self.errorState += 1
+        else:
+            self.assertEqual(func, os.rmdir)
+            self.assertEqual(arg, TESTFN)
+            self.assertTrue(isinstance(exc, OSError))
+            self.errorState = 3
+
+    def test_both_onerror_and_onexc(self):
+        onerror_called = False
+        onexc_called = False
+
+        def onerror(*args):
+            nonlocal onerror_called
+            onerror_called = True
+
+        def onexc(*args):
+            nonlocal onexc_called
+            onexc_called = True
+
+        os.mkdir(TESTFN)
+        self.addCleanup(shutil.rmtree, TESTFN)
+
+        self.child_file_path = os.path.join(TESTFN, 'a')
+        self.child_dir_path = os.path.join(TESTFN, 'b')
+        os_helper.create_empty_file(self.child_file_path)
+        os.mkdir(self.child_dir_path)
+        old_dir_mode = os.stat(TESTFN).st_mode
+        old_child_file_mode = os.stat(self.child_file_path).st_mode
+        old_child_dir_mode = os.stat(self.child_dir_path).st_mode
+        # Make unwritable.
+        new_mode = stat.S_IREAD|stat.S_IEXEC
+        os.chmod(self.child_file_path, new_mode)
+        os.chmod(self.child_dir_path, new_mode)
+        os.chmod(TESTFN, new_mode)
+
+        self.addCleanup(os.chmod, TESTFN, old_dir_mode)
+        self.addCleanup(os.chmod, self.child_file_path, old_child_file_mode)
+        self.addCleanup(os.chmod, self.child_dir_path, old_child_dir_mode)
+
+        shutil.rmtree(TESTFN, onerror=onerror, onexc=onexc)
+        self.assertTrue(onexc_called)
+        self.assertFalse(onerror_called)
+
     def test_rmtree_does_not_choke_on_failing_lstat(self):
         try:
             orig_lstat = os.lstat
diff --git a/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst b/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst
new file mode 100644
index 0000000..be9b2ba
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2023-03-19-15-30-59.gh-issue-102828.NKClXg.rst
@@ -0,0 +1,3 @@
+Add the ``onexc`` arg to :func:`shutil.rmtree`, which is like ``onerror``
+but expects an exception instance rather than an exc_info tuple. Deprecate
+``onerror``.