fix: rm_rf now handles directories without S_IXUSR permission by RonnyPfannschmidt · Pull Request #14331 · pytest-dev/pytest

RonnyPfannschmidt · 2026-03-27T19:26:40Z

Summary

Closes #7940.
Supersedes #7941.

The on_rm_rf_error handler had two gaps that prevented rm_rf from removing directories lacking the S_IXUSR (owner execute) permission bit:

chmod_rw only set S_IRUSR | S_IWUSR (read+write) — missing S_IXUSR meant directories couldn't be traversed by shutil.rmtree.
os.open / os.scandir PermissionError was silently skipped — shutil.rmtree (Python 3.12+ uses os.open, 3.10–3.11 uses os.scandir) could not enter directories without execute permission, and the error handler simply returned False.

Changes

Extracted _chmod_rwx() helper that uses stat.S_IRWXU (read+write+exec) and returns whether permissions actually changed (acts as a recursion guard).
os.open and os.scandir failures are now handled: permissions are fixed on the path and its parent, then the entry is removed directly (files via os.unlink, directories via recursive rm_rf).
All existing chmod_rw calls replaced with _chmod_rwx.

This takes the "fix it in on_rm_rf_error directly" approach rather than #7941's approach of delegating to tempfile.TemporaryDirectory._rmtree, keeping rm_rf robust for all callers.

Test plan

New test test_rm_rf_with_no_exec_permission_directories — exact scenario from Pytest cannot delete tmp_path directories without stat.S_IXUSR mode bit set #7940 (nested dirs + files with chmod(mode=0))
New test test_on_rm_rf_error_os_open_handles_file — os.open PermissionError on a file
New test test_on_rm_rf_error_os_open_handles_directory — os.open PermissionError on a directory tree
All existing TestRmRf tests pass unchanged
Full testing/test_tmpdir.py suite passes (41 passed, 1 skipped)
All pre-commit hooks pass (ruff, mypy, etc.)

Made with Cursor

…-dev#7940) The on_rm_rf_error handler previously only set S_IRUSR|S_IWUSR (read+write) when retrying failed removals, and silently skipped os.open/os.scandir failures. This meant directories lacking execute permission (S_IXUSR) could not be traversed or removed by shutil.rmtree. Now the handler: - Uses S_IRWXU (read+write+execute) for all permission fixes - Handles os.open and os.scandir PermissionError by fixing permissions on the path and its parent, then removing the entry directly - Includes a guard against infinite recursion when chmod has no effect Supersedes pytest-dev#7941 which took the approach of delegating to tempfile.TemporaryDirectory._rmtree. Co-authored-by: Cursor AI <ai@cursor.sh> Co-authored-by: Claude Opus <claude@anthropic.com>

Copilot

Pull request overview

This PR fixes rm_rf failures when removing directory trees that include directories missing the owner execute (S_IXUSR) permission bit, by improving on_rm_rf_error and adding regression tests for the affected traversal code paths.

Changes:

Added _chmod_rwx() to grant owner rwx permissions and replaced prior chmod_rw usage.
Enhanced on_rm_rf_error to handle os.open / os.scandir PermissionError by repairing permissions and removing entries directly.
Added new tests covering no-exec directory trees and os.open PermissionError handling; added a changelog entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
testing/test_tmpdir.py	Adds regression tests for removing trees with no-exec directories and for `os.open` PermissionError handling.
src/_pytest/pathlib.py	Introduces `_chmod_rwx()` and updates `on_rm_rf_error` to fix permissions and handle traversal errors from `os.open`/`os.scandir`.
changelog/7940.bugfix.rst	Documents the bugfix in the changelog.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/_pytest/pathlib.py

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

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bluetech · 2026-03-28T20:33:50Z

src/_pytest/pathlib.py

+    Returns True if permissions were actually changed, False if they were
+    already sufficient or couldn't be changed.
+    """
+    import stat


No need for lazy import

bluetech · 2026-03-28T20:44:48Z

src/_pytest/pathlib.py

+        p = Path(path)
+        parent_changed = False
+        if p.parent != p:
+            parent_changed = _chmod_rwx(str(p.parent))


Hmm isn't chmoding parent kinda dangerous? Say I do rm_rf('/tmp/foo/'), now it might try to go and chmod /tmp?

bluetech · 2026-03-28T20:46:45Z

changelog/7940.bugfix.rst

@@ -0,0 +1 @@
+Fixed ``rm_rf`` failing to remove directories when the ``S_IXUSR`` (owner execute) permission bit is missing -- supersedes :pr:`7941`.


User doesn't know what rm_rf is, it's an internal function. Better to explain in terms of user behavior.

Is the "supersedes" part needed in a changelog entry?

bluetech · 2026-03-28T20:50:38Z

src/_pytest/pathlib.py

+        # on the path and its parent, then remove it ourselves since rmtree
+        # skips entries after the error handler returns.
+        # See: https://github.com/pytest-dev/pytest/issues/7940
+        p = Path(path)


Since we already do p = Path(path) below in the function, I think it will be clearer to just move it up above the if and do everything in terms of it (including _chmod_rwx)

bluetech · 2026-03-28T20:51:29Z

src/_pytest/pathlib.py

+    if func in (os.open, os.scandir):
+        # Directory traversal failed (e.g. missing S_IXUSR). Fix permissions


Is the implicit assumption here that os.open is always on a directory? I think it will be better to make this explicit with an is_dir check.

bluetech · 2026-03-28T21:01:44Z

src/_pytest/pathlib.py

+        if not (parent_changed or path_changed):
+            return False
+        if os.path.isdir(path):
+            rm_rf(Path(path))


If for some reason the chmod didn't help (e.g. maybe some sandboxing prevents the deletion via some non-permission mechanism), this will end up in infinite recursion.

Copilot AI review requested due to automatic review settings March 27, 2026 19:26

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Mar 27, 2026

Copilot started reviewing on behalf of RonnyPfannschmidt March 27, 2026 19:27 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

src/_pytest/pathlib.py Outdated Show resolved Hide resolved

src/_pytest/pathlib.py Outdated Show resolved Hide resolved

src/_pytest/pathlib.py Outdated Show resolved Hide resolved

RonnyPfannschmidt requested a review from Copilot March 27, 2026 19:45

Copilot started reviewing on behalf of RonnyPfannschmidt March 27, 2026 19:47 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

bluetech reviewed Mar 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: rm_rf now handles directories without S_IXUSR permission#14331

fix: rm_rf now handles directories without S_IXUSR permission#14331
RonnyPfannschmidt wants to merge 2 commits intopytest-dev:mainfrom
RonnyPfannschmidt:fix/rm-rf-no-exec-permission-7940

RonnyPfannschmidt commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

bluetech Mar 28, 2026

Uh oh!

bluetech Mar 28, 2026

Uh oh!

bluetech Mar 28, 2026

Uh oh!

bluetech Mar 28, 2026

Uh oh!

bluetech Mar 28, 2026

Uh oh!

bluetech Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1 @@
		Fixed ``rm_rf`` failing to remove directories when the ``S_IXUSR`` (owner execute) permission bit is missing -- supersedes :pr:`7941`.

		if func in (os.open, os.scandir):
		# Directory traversal failed (e.g. missing S_IXUSR). Fix permissions

Uh oh!

Conversation

RonnyPfannschmidt commented Mar 27, 2026

Summary

Changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

bluetech Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants