-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix: rm_rf now handles directories without S_IXUSR permission #14331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fixed ``rm_rf`` failing to remove directories when the ``S_IXUSR`` (owner execute) permission bit is missing -- supersedes :pr:`7941`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,6 +70,31 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath: | |
| return path.joinpath(".lock") | ||
|
|
||
|
|
||
| def _chmod_rwx(p: str) -> bool: | ||
| """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. | ||
| """ | ||
| import stat | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for lazy import |
||
|
|
||
| try: | ||
| old_mode = os.stat(p).st_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: | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def on_rm_rf_error( | ||
| func: Callable[..., Any] | None, | ||
| path: str, | ||
|
|
@@ -98,32 +123,46 @@ def on_rm_rf_error( | |
| ) | ||
| return False | ||
|
|
||
| if func in (os.open, os.scandir): | ||
| # Directory traversal failed (e.g. missing S_IXUSR). Fix permissions | ||
|
Comment on lines
+126
to
+127
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the implicit assumption here that |
||
| # 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we already do |
||
| parent_changed = False | ||
| if p.parent != p: | ||
| parent_changed = _chmod_rwx(str(p.parent)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm isn't chmoding parent kinda dangerous? Say I do |
||
| path_changed = _chmod_rwx(path) | ||
| if not (parent_changed or path_changed): | ||
| return False | ||
| if os.path.isdir(path): | ||
| rm_rf(Path(path)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| else: | ||
| try: | ||
| os.unlink(path) | ||
| except OSError: | ||
| return False | ||
| return True | ||
|
|
||
| if func not in (os.rmdir, os.remove, os.unlink): | ||
| if func not in (os.open,): | ||
| warnings.warn( | ||
| PytestWarning( | ||
| f"(rm_rf) unknown function {func} when removing {path}:\n{type(exc)}: {exc}" | ||
| ) | ||
| warnings.warn( | ||
| PytestWarning( | ||
| f"(rm_rf) unknown function {func} when removing {path}:\n{type(exc)}: {exc}" | ||
| ) | ||
| ) | ||
| return False | ||
|
|
||
| # Chmod + retry. | ||
| import stat | ||
|
|
||
| def chmod_rw(p: str) -> None: | ||
| mode = os.stat(p).st_mode | ||
| os.chmod(p, mode | stat.S_IRUSR | stat.S_IWUSR) | ||
|
|
||
| # For files, we need to recursively go upwards in the directories to | ||
| # ensure they all are also writable. | ||
| # ensure they all are also accessible and writable. | ||
| p = Path(path) | ||
| if p.is_file(): | ||
| for parent in p.parents: | ||
| chmod_rw(str(parent)) | ||
| _chmod_rwx(str(parent)) | ||
| # Stop when we reach the original path passed to rm_rf. | ||
| if parent == start_path: | ||
| break | ||
| chmod_rw(str(path)) | ||
| _chmod_rwx(str(path)) | ||
|
|
||
| func(path) | ||
| return True | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm_rfis, it's an internal function. Better to explain in terms of user behavior.