Unix file enumerator isn't thread safe

[JeremyKuhne]

New test in dotnet/corefx#25426 uncovered this.

Unhandled Exception of Type System.NullReferenceException
Message :
System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace :
   at Interop.Sys.ReadDir(SafeDirectoryHandle dir, DirectoryEntry& outputEntry)
   at System.IO.UnixFileSystem.FileSystemEnumerable`1.<Enumerate>d__11.MoveNext()
   at System.IO.Tests.Regressions.ThreadSafeRepro.Race(IEnumerator`1 s) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.IO.FileSystem/tests/Directory/Regressions.cs:line 30
   at System.IO.Tests.Regressions.ThreadSafeRepro.Execute(String directory) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.IO.FileSystem/tests/Directory/Regressions.cs:line 53
   at System.IO.Tests.Regressions.FileEnumeratorIsThreadSafe() in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.IO.FileSystem/tests/Directory/Regressions.cs:line 21

[stephentoub]

It doesn't need to be; thread safety isn't a requirement of an enumerator. In fact, it's impossible to have an enumerator that can be correctly accessed by multiple threads concurrently, because there's no way for the implementation to make the separate MoveNext/Current calls atomic. If you access such enumerators from multiple threads, you're likely to get exceptions, including null refs. What we need to ensure is that you can't violate type safety or cause memory corruption with such access, and I see no evidence of that being a problem here. If there is such a problem, we need to fix it, though... do you see such a problem?

[JeremyKuhne]

Windows never fails. Weird results, but it never throws- shouldn't we meet the same bar here? @jkotas, any thoughts?

[stephentoub]

Weird results, but it never throws- shouldn't we meet the same bar here?

That's not necessary. Tons of instance methods across the framework throw strange exceptions if the methods are used incorrectly.

[jkotas]

There is no problem to fix here. I have said a similar thing as Stephen in dotnet/corefx#25426 (comment) :

Race condition in (safe) C# can manifest as odd managed exception, but it should not ever turn into (exploitable) buffer overrun.

Clean System.NullReferenceException is not a memory corruption.

[JeremyKuhne]

So what should I check or allow?

[jkotas]

You should allow any clean exceptions to be thrown. But the test should not tear down the process (it is what happens in .NET Core unconditionally on memory corruptions that lead to AV).

[JeremyKuhne]

So you're suggesting I try {} catch {} around the entire thing? Note that I did find other issues with handle recycling on this test.

[jkotas]

try {} catch {} would be my baseline solution.

Making it more fine grained is fine too e.g. flagging specific exceptions that can be only triggered by handle recycling bug as failures is fine.

[JeremyKuhne]

Ok, thanks.

[stephentoub]

@JeremyKuhne, the test is still disabled on Unix against this issue:

runtime/src/libraries/System.IO.FileSystem/tests/Directory/EnumerableTests.cs

Lines 14 to 16 in 04f2226

	[Fact]
	[ActiveIssue("https://github.com/dotnet/corefx/issues/25613", TestPlatforms.AnyUnix)]
	public void FileEnumeratorIsThreadSafe()