std: make remove_dir_all() fix up directory permissions on EACCES#158733
std: make remove_dir_all() fix up directory permissions on EACCES#158733daandemeyer wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @aapoalas (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
This is inconsistent with the behavior of |
I am happy to make it a separate function if that is preferred. |
This comment has been minimized.
This comment has been minimized.
Removing a directory entry requires write and search permission on the containing directory, and recursing into a directory requires read and search permission on it. A tree containing directories without these permissions could previously only be deleted by first running the equivalent of chmod -R u+rwx on it, even though its owner may unlink it. On Unix, when unlinkat() or openat() fail with EACCES during remove_dir_all(), try adding the owner read/write/search bits to the affected directory and retry. The mode is only changed for directories owned by the caller's effective user ID (chmod would fail otherwise anyway), only for directories that are themselves scheduled for deletion, and is restored if the retried operation still fails. The chmod is performed either through an already-open file descriptor or with fchmodat(AT_SYMLINK_NOFOLLOW) so that a concurrently swapped-in symlink cannot redirect it to a directory outside the tree. This also brings the Unix behavior closer to Windows, where files are deleted regardless of their read-only attribute. WASI, l4re and NuttX keep the old behavior since they lack the required libc functions.
daandemeyer
force-pushed
the
remove-dir-all-chmod
branch
from
b161704 to
52a9c9c
Compare
| unsafe { | ||
| let dirfd = parent_fd.unwrap_or(libc::AT_FDCWD); | ||
| let mut st: stat64 = mem::zeroed(); | ||
| if fstatat64(dirfd, path.as_ptr(), &mut st, libc::AT_SYMLINK_NOFOLLOW) < 0 { | ||
| return None; | ||
| } | ||
| let (old_mode, new_mode) = patched_mode(&st)?; | ||
| // AT_SYMLINK_NOFOLLOW ensures we cannot be tricked into changing the mode of a | ||
| // directory elsewhere through a concurrently swapped-in symlink. Platforms that | ||
| // do not support the flag for fchmodat() fail, keeping the original error. | ||
| if libc::fchmodat(dirfd, path.as_ptr(), new_mode, libc::AT_SYMLINK_NOFOLLOW) < 0 { | ||
| return None; | ||
| } | ||
| Some(old_mode) | ||
| } |
There was a problem hiding this comment.
Do not wrap everything in a big comment-free unsafe block.
If you see other code like that, that's because it has taken years, because it is no one's top priority, to clean it up. So some code is still basically how it was when it was written in early 2015 when people were rushing to meet the release date.
|
so there's absolutely nothing insulating this code against TOCTOU errors, right? it's just relying on the hypothesis that if we have permission to modify the file, it's fine to apply arbitrary permissions modifications? |
6 participants
Removing a directory entry requires write and search permission on the containing directory, and recursing into a directory requires read and search permission on it. A tree containing directories without these permissions could previously only be deleted by first running the equivalent of chmod -R u+rwx on it, even though its owner may unlink it.
On Unix, when unlinkat() or openat() fail with EACCES during remove_dir_all(), try adding the owner read/write/search bits to the affected directory and retry. The mode is only changed for directories owned by the caller's effective user ID (chmod would fail otherwise anyway), only for directories that are themselves scheduled for deletion, and is restored if the retried operation still fails. The chmod is performed either through an already-open file descriptor or with fchmodat(AT_SYMLINK_NOFOLLOW) so that a concurrently swapped-in symlink cannot redirect it to a directory outside the tree.
This also brings the Unix behavior closer to Windows, where files are deleted regardless of their read-only attribute.
WASI, l4re and NuttX keep the old behavior since they lack the required libc functions.