Fix `fs::remove_dir_all` #31944

pitdicker · 2016-02-27T20:18:40Z

This fixes the case where remove_dir_all is unable to remove large directories on Windows due to race conditions.
It is now also able to remove read-only files, and when the paths become longer that MAX_PATH.

Fixes #29497

This is based on #31892, I hope I didn't mess up git to much...

rust-highfive · 2016-02-27T20:18:45Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

retep998 · 2016-02-27T23:17:45Z

Your PR has three merges. You may want to consider rebasing and force pushing.

retep998 · 2016-02-27T23:20:42Z

src/libstd/sys/windows/c.rs

+
+#[repr(C)]
+pub struct FILE_DISPOSITION_INFO {
+    pub DeleteFile: BOOL, // for true use -1, not TRUE


Why does this need to be -1? I don't see anything on MSDN for why this can't be TRUE.

This struct is the same as FILE_DISPOSITION_INFORMATION which uses a BOOLEAN. But TRUE just doesn't work, it gives an error. The documentation for this fuction is not great, this one is better: https://msdn.microsoft.com/nl-nl/library/windows/hardware/ff567096(v=vs.85).aspx

Set this member to TRUE to delete the file when it is closed.

Nowhere do I see any mention of -1. What error do you get when you use TRUE instead of -1?

Something like: the file name, directory name or volume lable syntax is incorrect (I am on mobile, can't test now). But it took me hours to get working because of this bug. Only afterwards I found out this function is just a very thin wrapper around an NT internal function that takes a BOOLEAN instead of a BOOL, explaining the problem.

Can you move this comment to where it's called down in remove(&self) and put some of these comments there as well? Sounds like it'll be useful info for the next reader!

pitdicker · 2016-02-28T05:30:30Z

@retep998 thanks for helping out with FILE_RENAME_INFO on irc!

And time to improve my git skills :(

On Windows files can have a simple read-only permission flag, and the more complex access rights and access control lists. The read-only flag is one bit of the file attributes. To make sure only the read-only flag is toggled and not the other attributes, read the files attributes and write them back with only this bit changed. By reading and setting the attributes from the handle the file does not have to be opened twice. Also directories can not be read-only, here the flag has a different meaning. So we should no longer report directories as read-only, and also not allow making them read-only.

This fixes the case where `remove_dir_all` is unable to remove large directories on Windows due to race conditions. It is now also able to remove read-only files, and when the paths become longer that MAX_PATH.

pitdicker · 2016-02-28T07:53:23Z

Rebased

alexcrichton · 2016-02-29T17:58:41Z

src/libstd/sys/windows/fs.rs

+    opts.access_mode(c::FILE_READ_ATTRIBUTES | c::FILE_WRITE_ATTRIBUTES);
+    opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS);
+    let file = try!(File::open(path, &opts));
+    file.set_perm(perm)


Could you add some tests for this as well? It's fine for some of them to be windows-specific (e.g. in this module), but specifically:

Directories can't be set to readonly

Other attributes are preserved

alexcrichton · 2016-02-29T18:34:55Z

Thanks @pitdicker! This is looking great to me

pitdicker · 2016-02-29T19:23:24Z

Thanks for the review!.
I will do the other modifications during the week

pitdicker · 2016-03-05T15:55:29Z

Updated.

I messed up a lot with the permissions stuff of directories. Here is the article that explains how the read-only attribute indicates a directory is special on Windows. As it turns out you can create and delete files in a read-only directory, but nor delete or remove the dir itself. This is exactly the opposite of Unix, where you can't create or delete files in a read-only directory, but can delete the dir itself.

So I reversed the permission stuff. And we now have an other edge case to handle on Unix: readonly dirs. Before every call to remove_dir_all_recursive we now check if the dir is read-only, and adjust the permissions. I choose to do this outside of remove_dir_all_recursive, because we already do a stat in remove_dir_all that can now be reused.

I think this addresses all the comments except the suggestion to use offset_of! (I can't find it) and to tweak to_u16s (for an other pr).

alexcrichton · 2016-03-07T17:53:30Z

src/libstd/sys/unix/fs.rs

+                // readonly
+                mode.set_readonly(false);
+                try!(set_perm(&child.path(), mode));
+            }


Perhaps this function could take &Metadata as an argument and perform this logic at the start? That way it could prevent duplication between here/above.

alexcrichton · 2016-03-07T18:12:45Z

Hm now I'm getting a little more worried with this as time passes, it seems like we're definitely going out of our way to implement a arguably primitive operation, which may end up being surprising to some. This code was originally modelled after Boost which is significantly simpler than what's going on here (even on Unix). That's not necessarily either good or bad, but it's indicative to me that we should tread lightly here.

Perhaps we can try to find some other implementations of this function in other libraries? It it standard practice to attempt to so aggressively remove files/directories?

pitdicker · 2016-03-07T20:21:56Z

I think the part in the Windows version of moving files is not really over the top. You could say it is just a necessary evil to avoid races. But I share your feeling about the rest.

Personally I am finding more and more functions that could benefit from options. Overwrite or fail if something already exist. Follow symlinks or not. Delete read-only files, fail, or maybe ask for confirmation.... For copy whether we should copy times, attributes etc. Be recursive or not.

What to do about read-only files on Windows, and read-only directories on Unix is the difficult part. I kind of like it to just work, as you are also able to move the dir somewhere else and it looks deleted from that place. But I don't feel very strongly about it. (of course it would be sad, as it took me quite some time to get working...)

For a filesystem library to take inspiration from, I look a little at AFIO. I can't read it very easily, but it has the focus on correctness, avoiding races, and cross-platform consistency. But it is not all good, otherwise it would have ended up in Boost.

alexcrichton · 2016-03-09T18:29:17Z

Yeah it was somewhat planned that std::fs would grow various builders over time for options like these. You're right that fs::copy could do so much more (attributes, recursive, etc), but they'd probably want to all be encapsulated in some builder with tweakable options.

In terms of removing readonly directories on Unix, it looks like rm -rf doesn't automatically remove a readonly directory. Maybe we should stay consistent with that? Maybe we should only support removing readonly files on Windows specially?

alexcrichton · 2016-03-17T18:50:51Z

We discussed this during the libs triage meeting yesterday, and we unfortunately didn't reach too many conclusions about this. Some thoughts brought up were:

It would likely be worth looking at what other standard libraries do in this respect. For example this was copied from Boost, and it looks like Python also has a similar implementation (and similar bug reports).
We could perhaps add a remove_dir_all_force variant along the lines of rm -f to do the "extra work" to really delete something. If this is the function that basically everyone uses, however, then that seems unfortunate?
We may want to think about what would precisely be documented here. For example what are we comfortable documenting that this function does across platforms? Can we strictly document all "weird cases" it handles?
There's also the whole story of if you want to whitelist syscalls changing this over time can be hard, but this dies into whether we can document what it does today.

cc @rust-lang/libs

retep998 · 2016-03-17T19:30:44Z

The MSVC implementation of std::experimental::filesystem::remove_all is interesting. Instead of iterating over all the entries in the directory, it just gets the first entry, removes it, and repeats until the directory is empty or it encounters an error.

alexcrichton · 2016-03-18T04:03:57Z

Interesting! Is the source for that public so we can browse over?

retep998 · 2016-03-18T04:14:59Z

Well you can view the source if you install MSVC and the Windows SDK.

remove_all and _Remove_all in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\filesystem
Some other helper functions in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\crt\src\stl\filesys.h

bors · 2016-03-23T19:38:51Z

☔ The latest upstream changes (presumably #32390) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2016-06-28T20:49:42Z

Closing due to inactivity, but feel free to resubmit with a rebase!

rust-highfive assigned aturon Feb 27, 2016

retep998 reviewed Feb 27, 2016
View reviewed changes

pitdicker added 2 commits February 28, 2016 08:49

Fix fs::remove_dir_all

3d65bb2

This fixes the case where `remove_dir_all` is unable to remove large directories on Windows due to race conditions. It is now also able to remove read-only files, and when the paths become longer that MAX_PATH.

pitdicker mentioned this pull request Feb 28, 2016

Small fixes to setting Windows file permissions #31892

Closed

Nicer iterator

5f9ca88

alexcrichton reviewed Feb 29, 2016
View reviewed changes

alexcrichton assigned alexcrichton and unassigned aturon Feb 29, 2016

Address comments

9bb385e

Fix removing the contents of a readonly dir on Unix

89489ec

alexcrichton reviewed Mar 7, 2016
View reviewed changes

alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 14, 2016

alexcrichton closed this Jun 28, 2016

XAMPPRocky mentioned this pull request Jan 11, 2020

fs::remove_dir_all rarely succeeds for large directories on windows #29497

Open

VishnuJin mentioned this pull request Jun 12, 2021

Windows cannot cleanup volumes on unmount krustlet/krustlet#625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `fs::remove_dir_all` #31944

Fix `fs::remove_dir_all` #31944

pitdicker commented Feb 27, 2016

rust-highfive commented Feb 27, 2016

retep998 commented Feb 27, 2016

retep998 Feb 27, 2016

pitdicker Feb 28, 2016

retep998 Feb 28, 2016

pitdicker Feb 28, 2016

alexcrichton Feb 29, 2016

pitdicker commented Feb 28, 2016

pitdicker commented Feb 28, 2016

alexcrichton Feb 29, 2016

alexcrichton commented Feb 29, 2016

pitdicker commented Feb 29, 2016

pitdicker commented Mar 5, 2016

alexcrichton Mar 7, 2016

alexcrichton commented Mar 7, 2016

pitdicker commented Mar 7, 2016

alexcrichton commented Mar 9, 2016

alexcrichton commented Mar 17, 2016

retep998 commented Mar 17, 2016

alexcrichton commented Mar 18, 2016

retep998 commented Mar 18, 2016

bors commented Mar 23, 2016

alexcrichton commented Jun 28, 2016

Fix fs::remove_dir_all #31944

Fix fs::remove_dir_all #31944

Conversation

pitdicker commented Feb 27, 2016

rust-highfive commented Feb 27, 2016

retep998 commented Feb 27, 2016

retep998 Feb 27, 2016

Choose a reason for hiding this comment

pitdicker Feb 28, 2016

Choose a reason for hiding this comment

retep998 Feb 28, 2016

Choose a reason for hiding this comment

pitdicker Feb 28, 2016

Choose a reason for hiding this comment

alexcrichton Feb 29, 2016

Choose a reason for hiding this comment

pitdicker commented Feb 28, 2016

pitdicker commented Feb 28, 2016

alexcrichton Feb 29, 2016

Choose a reason for hiding this comment

alexcrichton commented Feb 29, 2016

pitdicker commented Feb 29, 2016

pitdicker commented Mar 5, 2016

alexcrichton Mar 7, 2016

Choose a reason for hiding this comment

alexcrichton commented Mar 7, 2016

pitdicker commented Mar 7, 2016

alexcrichton commented Mar 9, 2016

alexcrichton commented Mar 17, 2016

retep998 commented Mar 17, 2016

alexcrichton commented Mar 18, 2016

retep998 commented Mar 18, 2016

bors commented Mar 23, 2016

alexcrichton commented Jun 28, 2016

Fix `fs::remove_dir_all` #31944

Fix `fs::remove_dir_all` #31944