Address Copilot review feedback on rm_rf permission handling

- Use stat.S_IMODE() to extract only permission bits before
  comparing/setting, avoiding file-type bit leakage into os.chmod
- Only add S_IXUSR for directories; regular files get S_IRUSR|S_IWUSR
  only, avoiding unnecessary execute side-effect on files
- Fix recursion guard: track whether *either* parent or path chmod
  changed permissions, so fixing the parent alone (the common case for
  missing S_IXUSR) is sufficient to proceed with removal
- Add test for parent-only permission fix scenario

Co-authored-by: Cursor AI <ai@cursor.sh>
Co-authored-by: Claude Opus <claude@anthropic.com>

Author: 3 people

src/_pytest/pathlib.py

@@ -71,7 +71,11 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath:
 
 
 def _chmod_rwx(p: str) -> bool:
-    """Grant owner read, write, and execute permissions.
+    """Grant owner sufficient permissions for deletion.
+
+    Directories get ``S_IRWXU`` (read+write+exec for traversal).
+    Regular files get ``S_IRUSR | S_IWUSR`` only, to avoid making
+    non-executable files executable as a side effect.
 
     Returns True if permissions were actually changed, False if they were
     already sufficient or couldn't be changed.
@@ -80,8 +84,10 @@ def _chmod_rwx(p: str) -> bool:
 
     try:
         old_mode = os.stat(p).st_mode
-        new_mode = old_mode | stat.S_IRWXU
-        if old_mode == new_mode:
+        perm_mode = stat.S_IMODE(old_mode)
+        bits = stat.S_IRWXU if stat.S_ISDIR(old_mode) else stat.S_IRUSR | stat.S_IWUSR
+        new_mode = perm_mode | bits
+        if perm_mode == new_mode:
             return False
         os.chmod(p, new_mode)
     except OSError:
@@ -123,9 +129,11 @@ def on_rm_rf_error(
         # skips entries after the error handler returns.
         # See: https://github.com/pytest-dev/pytest/issues/7940
         p = Path(path)
+        parent_changed = False
         if p.parent != p:
-            _chmod_rwx(str(p.parent))
-        if not _chmod_rwx(path):
+            parent_changed = _chmod_rwx(str(p.parent))
+        path_changed = _chmod_rwx(path)
+        if not (parent_changed or path_changed):
             return False
         if os.path.isdir(path):
             rm_rf(Path(path))

testing/test_tmpdir.py

@@ -582,6 +582,26 @@ def test_on_rm_rf_error_os_open_handles_directory(self, tmp_path: Path) -> None:
             assert not adir.exists()
             assert not [x.message for x in w]
 
+    @pytest.mark.skipif(not hasattr(os, "getuid"), reason="unix permissions")
+    def test_on_rm_rf_error_os_open_parent_perms(self, tmp_path: Path) -> None:
+        """When the PermissionError is caused by the *parent* directory lacking
+        S_IXUSR, fixing the parent is sufficient even if the child already has
+        correct permissions."""
+        parent = tmp_path / "parent"
+        parent.mkdir()
+        child = parent / "child"
+        child.mkdir()
+        (child / "file.txt").touch()
+        # Child has full perms, but parent lacks execute -> os.open(child) fails.
+        os.chmod(str(parent), 0o600)
+
+        with warnings.catch_warnings(record=True) as w:
+            exc_info = PermissionError()
+            result = on_rm_rf_error(os.open, str(child), exc_info, start_path=tmp_path)
+            assert result is True
+            assert not child.exists()
+            assert not [x.message for x in w]
+
 
 def attempt_symlink_to(path, to_path):
     """Try to make a symlink from "path" to "to_path", skipping in case this platform