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.

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

std::fs::remove_dir_all fails if any of the intermediate file deletions fails with ENOENT #127576

Closed
tbu- opened this issue Jul 10, 2024 · 6 comments · Fixed by #127623
Closed
Assignees
Labels
A-filesystem Area: `std::fs` C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@tbu-
Copy link
Contributor

tbu- commented Jul 10, 2024

cvt(unsafe { unlinkat(fd, child_name.as_ptr(), 0) })?;

// unlink the directory after removing its contents
cvt(unsafe {
unlinkat(parent_fd.unwrap_or(libc::AT_FDCWD), path.as_ptr(), libc::AT_REMOVEDIR)
})?;

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 10, 2024
@tgross35
Copy link
Contributor

tgross35 commented Jul 11, 2024

Are you suggesting that this should continue removing files if one fails but still report an error, or that specifically ENOENT on a file should be considered a success?

I assume this can happen if racing with another removal operation, not sure if there is a precedent for what to do here (rm?).

@rustbot label +T-libs +C-bug +O-unix +A-filesystem -needs-triage

@rustbot rustbot added A-filesystem Area: `std::fs` C-bug Category: This is a bug. O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 11, 2024
@tgross35
Copy link
Contributor

Just checked what rm does when this happens (by making a lot of files and having two rms race), it indeed ignores ENOENT:

...
unlinkat(4, "768458", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768461", 0)                = 0
unlinkat(4, "768463", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768472", 0)                = 0
unlinkat(4, "768476", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768479", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768485", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768490", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768496", 0)                = -1 ENOENT (No such file or directory)
unlinkat(4, "768501", 0)                = 0
unlinkat(4, "768504", 0)                = 0
unlinkat(4, "768506", 0)                = 0
...
$ echo $?
0

So doing the same seems probably fine here, think this should be pretty easy. Ideally this should have a regression test, I'm not sure what the best way to do that is - @tbu- how did you identify or reproduce this?

@rustbot label +E-easy

@rustbot rustbot added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jul 11, 2024
@tbu-
Copy link
Contributor Author

tbu- commented Jul 11, 2024

I looked at the code for TOCTOU issues and found this particular problem. Nothing automatable I'm afraid.

@tbu-
Copy link
Contributor Author

tbu- commented Jul 11, 2024

Are you suggesting that this should continue removing files if one fails but still report an error, or that specifically ENOENT on a file should be considered a success?

Specifically ENOENT should be considered a success.

I assume this can happen if racing with another removal operation, not sure if there is a precedent for what to do here (rm?).

Yes, this is about racing. E.g. std::fs::create_dir_all also succeeds when EEXISTS is returned for mkdir.

@ChrisDenton
Copy link
Member

This seems like it's just an implementation bug so fixes would be welcome. On Windows we do ignore not found errors when deleting files in remove_dir_all.

@lolbinarycat
Copy link
Contributor

@rustbot claim

lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Jul 11, 2024
lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Jul 11, 2024
lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Aug 2, 2024
lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Aug 18, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 18, 2024
…=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576
lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Aug 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 19, 2024
…try>

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576

try-job: test-various
jieyouxu added a commit to jieyouxu/rust that referenced this issue Aug 20, 2024
…=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576

try-job: test-various
jieyouxu added a commit to jieyouxu/rust that referenced this issue Aug 20, 2024
…=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576

try-job: test-various
lolbinarycat added a commit to lolbinarycat/rust that referenced this issue Aug 20, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 23, 2024
…=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576

try-job: test-various
@bors bors closed this as completed in 736f773 Aug 23, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 23, 2024
Rollup merge of rust-lang#127623 - lolbinarycat:fix_remove_dir_all, r=Amanieu

fix: fs::remove_dir_all: treat internal ENOENT as success

fixes rust-lang#127576

try-job: test-various
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-filesystem Area: `std::fs` C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. O-unix Operating system: Unix-like T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants