Skip to content
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.

Sign up for GitHub

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

Jump to bottom

UNIX remove_dir_all(): Try recursing first on the slow path #94446

Merged
merged 3 commits into from
Mar 5, 2022
Merged

UNIX remove_dir_all(): Try recursing first on the slow path #94446

merged 3 commits into from
Mar 5, 2022

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Feb 28, 2022 •
edited
Loading

This only affects the slow code path - if there is no dirent.d_type or if it is DT_UNKNOWN.

POSIX specifies that calling unlink() or unlinkat(..., 0) on a directory is allowed to succeed:

The path argument shall not name a directory unless the process has appropriate privileges and the implementation supports using unlink() on directories.

This however can cause dangling inodes requiring an fsck e.g. on Illumos UFS, so we have to avoid that in the common case. We now just try to recurse into it first and unlink() if we can't open it as a directory.

The other two commits integrate the Macos x86-64 implementation reducing redundancy. Split into two commits for better reviewing.

Fixes #94335.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2022
This was referenced Feb 28, 2022
Closed
Closed
@hkratz hkratz mentioned this pull request Feb 28, 2022
Closed
@cuviper
Copy link
Member

cuviper commented Feb 28, 2022

On Linux and FreeBSD trying to unlink() a directory always fails with EISDIR and EPERM respectively.

Even with NFS to a server OS that does allow unlinking a directory?

@hkratz
Copy link
Contributor Author

hkratz commented Feb 28, 2022 •
edited
Loading

On Linux and FreeBSD trying to unlink() a directory always fails with EISDIR and EPERM respectively.

Even with NFS to a server OS that does allow unlinking a directory?

According to the man pages it is guaranteed in deviation from the standard POSIX text and other UNIX man pages. I will look into the kernel sources.

@hkratz hkratz changed the title UNIX remove_dir_all(): Only unlink first if it is guaranteed to error for directories UNIX remove_dir_all(): Try recursing first on the slow path Feb 28, 2022
@hkratz
Copy link
Contributor Author

hkratz commented Feb 28, 2022 •
edited
Loading

On Linux and FreeBSD trying to unlink() a directory always fails with EISDIR and EPERM respectively.

Even with NFS to a server OS that does allow unlinking a directory?

After glossing over the NFSv3/v4 specs it is clear that the OS cannot guarantee that if no_root_squash is used. On the plus side that means we do not need a separate implementation for Linux/FreeBSD and the fast path is still fast anyway.

@cuviper
Copy link
Member

cuviper commented Mar 2, 2022

r? @cuviper

@rust-highfive rust-highfive assigned cuviper and unassigned kennytm Mar 2, 2022
cuviper
cuviper requested changes Mar 2, 2022
View reviewed changes
library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
@hkratz hkratz force-pushed the remove_dir_all-illumos-fix branch 3 times, most recently from da1a6cd to c654255 Compare March 3, 2022 07:24
cuviper
cuviper approved these changes Mar 3, 2022
View reviewed changes
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, r=me after squashing.

@hkratz
remove_dir_all(): try recursing first instead of trying to unlink()
e427333 
This only affects the `slow` code path, if there is no `dirent.d_type` or if
the type is `DT_UNKNOWN`.

POSIX specifies that calling `unlink()` or `unlinkat(..., 0)` on a directory can
succeed:
> "The _path_ argument shall not name a directory unless the process has
> appropriate privileges and the implementation supports using _unlink()_ on
> directories."
This however can cause orphaned directories requiring an fsck e.g. on Illumos
UFS, so we have to avoid that in the common case. We now just try to recurse
into it first and unlink() if we can't open it as a directory.
@hkratz hkratz force-pushed the remove_dir_all-illumos-fix branch from c654255 to 5ac2345 Compare March 4, 2022 12:34
@hkratz hkratz force-pushed the remove_dir_all-illumos-fix branch from 5ac2345 to 735f60c Compare March 4, 2022 12:48
@hkratz
Copy link
Contributor Author

hkratz commented Mar 4, 2022

@cuviper Squashed and reworded the macos-impl. commit messages.

@cuviper
Copy link
Member

cuviper commented Mar 5, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Mar 5, 2022

📌 Commit 735f60c has been approved by cuviper

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 5, 2022
@Dylan-DPC
Rollup merge of rust-lang#94446 - rusticstuff:remove_dir_all-illumos-…
478851c 
…fix, r=cuviper

UNIX `remove_dir_all()`: Try recursing first on the slow path

This only affects the _slow_ code path - if there is no `dirent.d_type` or if it is `DT_UNKNOWN`.

POSIX specifies that calling `unlink()` or `unlinkat(..., 0)` on a directory is allowed to succeed:
> The _path_ argument shall not name a directory unless the process has appropriate privileges and the implementation supports using _unlink()_ on directories.

This however can cause dangling inodes requiring an fsck e.g. on Illumos UFS, so we have to avoid that in the common case. We now just try to recurse into it first and unlink() if we can't open it as a directory.

The other two commits integrate the Macos x86-64 implementation reducing redundancy. Split into two commits for better reviewing.

Fixes rust-lang#94335.
This was referenced Mar 5, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2022
@bors
Auto merge of rust-lang#94634 - Dylan-DPC:rollup-8wx1yrj, r=Dylan-DPC
be72308 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#94446 (UNIX `remove_dir_all()`: Try recursing first on the slow path)
 - rust-lang#94460 (Reenable generator drop tracking tests and fix mutation handling)
 - rust-lang#94620 (Edit docs on consistency of `PartialOrd` and `PartialEq`)
 - rust-lang#94624 (Downgrade `#[test]` on macro call to warning)
 - rust-lang#94626 (Add known-bug directive to issue rust-lang#47511 test case)
 - rust-lang#94631 (Fix typo in c-variadic)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3e1e9b4 into rust-lang:master Mar 5, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper approved these changes

Assignees

@cuviper cuviper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

Fix for CVE-2022-21658 is unsafe on some file systems
6 participants
@hkratz @rust-highfive @cuviper @bors @kennytm @rustbot