Runtime: FileSystemEnumerable throws UnauthorizedAccessException when IgnoreInaccessible is used

[cocowalla]

I'm on .NET Core 2.1, and I've been testing the new FileSystemEnumerable on Linux (specifically CentOS 7). I've found it throws UnauthorizedAccessException when trying to recurse into folders the current user doesn't have access to, even if EnumerationOptions.IgnoreInaccessible is set.

Stack trace:

System.UnauthorizedAccessException: Access to the path '/home/user/root' is denied. ---> System.IO.IOException: Permission denied
   --- End of inner exception stack trace ---
   at System.IO.Enumeration.FileSystemEnumerator`1.CreateDirectoryHandle(String path)
   at System.IO.Enumeration.FileSystemEnumerator`1.DirectoryFinished()
   at System.IO.Enumeration.FileSystemEnumerator`1.FindNextEntry()
   at System.IO.Enumeration.FileSystemEnumerator`1.MoveNext()
   at LinuxFileTest.Program.Main(String[] args) in C:\MyProj\Program.cs:line XX

This ends the enumeration, so effectively renders FileSystemEnumerable unusable on Linux if there is any chance of recursing into a folder the current user can't access :(

@JeremyKuhne I think this is your baby?

[danmosemsft]

Yes we hope to service for this
https://github.com/dotnet/corefx/issues/30126

Can you confirm it works well for you on master?

[cocowalla]

I'm unable to download the latest daily build, as all the links give ResourceNotFound :( Have opened an issue over on the cli repo.

[danmosemsft]

@eerhardt is the installer on the CLI repo including post 2.1 CoreFX bits now?

@cocowalla if not, you would have to patch it per these instructions.

[eerhardt]

is the installer on the CLI repo including post 2.1 CoreFX bits now?

Yes, the latest CLI builds from master include a runtime build from 10 days ago.

[cocowalla]

@danmosemsft confirmed it works with the daily build available from the Core CLI repo! I'll go ahead and close this issue.

[danmosemsft]

@cocowalla excellent, thank you for doing that.

[JeremyKuhne]

@cocowalla thanks for validating! Please continue to tag me if you find any issues or have any other feedback. I've been reading, but didn't respond as @danmosemsft beat me to it. :)

[peter-bertok]

Is this API supposed to throw exceptions on MoveNext() in general?

To clarify, this is not just a Linux-specific bug, this is a general issue that occurs on all platforms and makes this API unusable with foreach, Linq, or practically any use-case except hand rolled do-while loops.

For example, this straightforward use-case of enumerating through my C:\ crashes with a FileNotFoundExcpetion:

https://gist.github.com/peter-bertok/e4f4f6971c0263166158608094de9d8b

The cause? Visual Studio 2015!

Under "C:\Program Files (x86)\Microsoft Visual Studio\VS15Preview\Common7\IDE\Extensions" there is some sort of symlink called "Visual Studio Preview Updater" pointing at a deleted file or directory. I previously had 2015 installed, uninstalled it, and this got left behind.

This type of thing is not unusual. File enumeration is expected to hit a wide range of I/O errors, including but not exclusive to:

• Race conditions such as files being deleted out of a directory as it is being enumerated.
• Files in the middle of operations that make it totally inaccessible, even to enumeration. For example, NetApp filers can put files into a "Delete pending" state for seconds, which is technically allowed by the SMB protocol. This will cause practically all I/O calls to fail, including retrieval of attributes.
• Corrupt NTFS or FAT metadata is expected in the wild and needs to be handled.
• Recursive symlinks. This can occur on Windows as well, and is well-known cause of robocopy getting "stuck" when copying user profiles, as an example.
• Files inaccessible to the user. This is the only common case that appears to be handled, via the IgnoreInaccessible option. This however looks an awful lot like a band-aid applied to work around this specific issue only, and obviously won't cater for any of the easily predictable scenarios above...

An API that is based on IEnumerable<T> really should handle errors internally, such as providing a HandleIOErrorPredicate or something along those lines.

[danmosemsft]

@peter-bertok what platform and what version of.NET Core are you seeing this on? Did you set EnumerationOptions.IgnoreInaccessible?

[peter-bertok]

@danmosemsft dotnet core SDK version 2.1.300 on Windows 10 v1803. Yes, I set the flag, as per the gist I linked. That only ignores Access Denied errors, there are a vast array of errors this does not include.

I did some additional research and found that the unit test for this code subclasses the FileSystemEnumerator<T> class to create a "safe" version:

https://github.com/dotnet/corefx/blob/d3ed110eff677ceefb9346109dbd1c681e080f34/src/System.IO.FileSystem/tests/Enumeration/ErrorHandlingTests.netcoreapp.cs#L24-L28

This is rather... icky.

Outside of tests, all implementations in corefx simply return false.

Hence, this API is only safely usable if it is subclassed and this method is overridden:

https://github.com/dotnet/corefx/blob/95b21631b4f72e5ecbb6338a3fc7ae9345b5144e/src/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs#L39

This is... bad. An API like this leads users into a pit of failure by default, and minimally documented magic needs to be applied consistently by users to handle common errors that robust code is _expected_ to cater for.

Overriding the virtual method is not a suitable solution in general because:

• The provided error code is platform-specific, so any attempt at using it is going to lead to fragile code tied to a specific set of platforms.
• It does not pass through the current ref FileSystemEntry, severely limiting the decision-making that can be made without polluting the TResult output type returned from TransformEntry().
• It assumes all platforms will only ever use 32-bit integers to report errors from low-level filesystem calls, and it cannot be extended to support any other type of error handling because it is a public API that is practically mandatory to use for robust code. Changes to this method signature will be a breaking API change...

Literally the only other reference -- other than issue reports -- I can find to error handling in relation to FileSystemEnumerator<T> is on this blog entry by @JeremyKuhne, where very near the end there is this gem:

As the errors are native error codes, you would need to check the platform in addition to the code to know how to respond. It is not intended to be easy or commonly needed.

ಠ_ಠ

[danmosemsft]

Thanks this is good feedback. I hope we can improve this. @jeremykuhne?

[JeremyKuhne]

I appreciate the feedback. Before I respond to specific points I want to be clear that shipping this new functionality was not a declaration of "done" with enumeration. There were a few goals:

1. Improve enumeration performance and provide some commonly requested features
2. Make it possible to customize enumeration to unblock people
3. Not take multiple years to ship, and yet not block further improvements

And one other thing- we welcome community contributions. If you're passionate about this and can help I'm happy to work with you to move this stuff forward.

This is rather... icky.

It is a test, not meant to be an example of production code. :)

where very near the end there is this gem:

It doesn't mean that we can't do more convenient helpers in the future. Rather than not give any ability to customize the error handling in the first release (or kick the feature to a future release) I provided a "back door" of sorts so it was actually possible to write code that can do the advanced error handling you are discussing (without waiting for 3.0 or some later release).

Note that while we can provide "mapped" errors, it isn't possible to do for all cases. There are very platform and driver specific errors that are useful in some cases (i.e. the current API is useful). I expect the likely thing we'll add is a static helper like "TryMapNativeError(int nativeError)" that returns either the traditional Windows error code or some new enum that we expand over time.

and minimally documented magic

The documentation team is still working on 2.1 docs (using my blog posts as a reference). I didn't expect immediate interest in this method, I'll change my priorities accordingly.

It does not pass through the current ref FileSystemEntry

We can look to see if we can find an rational design that explains the current context. You don't have an entry when the error occurs- without incurring an allocation tax for those that don't care about context.

Recursive symlinks.

This is hugely expensive to track. I do want to see the option in the future and even discussed it in the API design docs. How to do this without regressing the performance for those who don't care is tricky.

enumerating through my C:\ crashes with a FileNotFoundExcpetion

Can you provide the call stack? It would really help. We generally try to be resilient to vanishing files/directories. That said, I did make an update for this specific bug that tries to tidy up missing file/directory errors.

Again, thanks for the interest and feedback.

[cocowalla]

@JeremyKuhne I just noticed this got added to the 3.0 milestone - does that mean we can't expect any fixes or improvements to FileSystemEnumerable in 2.2?

[JeremyKuhne]

@karelz did a bulk move, I'm not sure why he moved closed items as well.

That said, we are, in fact, only doing critical changes for 2.2. New features & functionality would be in 3.0.

I'd love to continue to make/facilitate improvements here. I can't guarantee timelines as I'm juggling a lot of other things but I'll always find time to at least help facilitate if anyone gets/finds time to push this area further forward. :)