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

Path.GetTempFileName() sometimes fails on WASM due to insufficient randomness #73721

Closed
radical opened this issue Aug 10, 2022 · 47 comments · Fixed by #81085
Closed

Path.GetTempFileName() sometimes fails on WASM due to insufficient randomness #73721

radical opened this issue Aug 10, 2022 · 47 comments · Fixed by #81085
Assignees
Labels
arch-wasm WebAssembly architecture area-System.IO disabled-test The test is disabled in source code against the issue os-windows test-failure
Milestone

Comments

@radical
Copy link
Member

radical commented Aug 10, 2022

Build, seen on multiple PRs with no related changes. Which suggests that this is on main. Failures like:

[19:23:32] info: Starting:    System.CodeDom.Tests.dll
[19:23:39] fail: [FAIL] System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(source: "")
[19:23:39] info: System.IO.IOException : File exists
[19:23:39] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:23:39] info:    at Interop.ThrowIOExceptionForLastError()
[19:23:39] info:    at System.IO.Directory.CreateTempSubdirectoryCore(String prefix)
[19:23:39] info:    at System.IO.Directory.CreateTempSubdirectory(String prefix)
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.GetTempDirectory()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.EnsureTempNameCreated()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.get_BasePath()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension, Boolean keepFile)
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension)
[19:23:39] info:    at System.CodeDom.Compiler.CodeCompiler.FromSourceBatch(CompilerParameters options, String[] sources)
[19:23:39] info:    at System.CodeDom.Compiler.CodeCompiler.FromSource(CompilerParameters options, String source)
[19:23:39] info:    at System.CodeDom.Compiler.Tests.CodeCompilerTests.Compiler.FromSourceEntryPoint(CompilerParameters options, String source)
[19:23:39] info:    at System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(String source)
[19:23:39] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)

.. and ..

[19:27:13] info: Starting:    System.Diagnostics.TextWriterTraceListener.Tests.dll
[19:27:14] fail: [FAIL] System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "")
[19:27:14] info: System.IO.IOException : File exists
[19:27:14] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(Int64 result, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(IntPtr result, String path, Boolean isDirectory)
[19:27:14] info:    at System.IO.Path.GetTempFileName()
[19:27:14] info:    at System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(String testName)
[19:27:14] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)
[19:27:14] fail: [FAIL] System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "><&")
[19:27:14] info: System.IO.IOException : File exists
[19:27:14] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(Int64 result, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(IntPtr result, String path, Boolean isDirectory)
[19:27:14] info:    at System.IO.Path.GetTempFileName()
[19:27:14] info:    at System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(String testName)
[19:27:14] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)

Just for people reading this in the future, the list of failed tests:

System.CodeDom.Compiler.Tests.CodeCompilerTests.FromDom_ValidCodeCompileUnit_ReturnsExpected(compilationUnit: CodeCompileUnit { AssemblyCustomAttributes = [], EndDirectives = [], Namespaces = [], ReferencedAssemblies = ["assembly1", "assembly2"], StartDirectives = [], ... })
System.CodeDom.Compiler.Tests.CodeCompilerTests.GetResponseFileCmdArgs_ValidCmdArgs_ReturnsExpected(cmdArgs: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.CompileAssemblyFromSourceBatch_ValidSources_ReturnsExpected(sources: [null])
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(source: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSourceBatch_ValidSources_ReturnsExpected(sources: [""])
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ThrowsPlatformNotSupportedException(source: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromDom_ValidCodeCompileUnit_ThrowsPlatformNotSupportedException(compilationUnit: CodeCompileUnit { AssemblyCustomAttributes = [], EndDirectives = [], Namespaces = [], ReferencedAssemblies = ["referenced", "assembly1"], StartDirectives = [], ... })
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSourceBatch_ValidSources_ThrowsPlatformNotSupportedException(sources: [""])

System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"...)
System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "><&")
System.Diagnostics.TextWriterTraceListenerTests.XmlWriterTraceListenerTests.SingleArgumentConstructorTest

 ystem.ServiceModel.Syndication.Tests.BasicScenarioTests.SyndicationFeed_Write_RSS_Atom

The full set can be seen with that build url. I think #73408 broke these tests.

@eerhardt

@radical radical added arch-wasm WebAssembly architecture area-System.IO os-windows blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' test-failure labels Aug 10, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 10, 2022
@ghost
Copy link

ghost commented Aug 10, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Build, seen on multiple PRs with no related changes. Which suggests that this is on main. Failures like:

[19:23:32] info: Starting:    System.CodeDom.Tests.dll
[19:23:39] fail: [FAIL] System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(source: "")
[19:23:39] info: System.IO.IOException : File exists
[19:23:39] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:23:39] info:    at Interop.ThrowIOExceptionForLastError()
[19:23:39] info:    at System.IO.Directory.CreateTempSubdirectoryCore(String prefix)
[19:23:39] info:    at System.IO.Directory.CreateTempSubdirectory(String prefix)
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.GetTempDirectory()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.EnsureTempNameCreated()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.get_BasePath()
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension, Boolean keepFile)
[19:23:39] info:    at System.CodeDom.Compiler.TempFileCollection.AddExtension(String fileExtension)
[19:23:39] info:    at System.CodeDom.Compiler.CodeCompiler.FromSourceBatch(CompilerParameters options, String[] sources)
[19:23:39] info:    at System.CodeDom.Compiler.CodeCompiler.FromSource(CompilerParameters options, String source)
[19:23:39] info:    at System.CodeDom.Compiler.Tests.CodeCompilerTests.Compiler.FromSourceEntryPoint(CompilerParameters options, String source)
[19:23:39] info:    at System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(String source)
[19:23:39] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)

.. and ..

[19:27:13] info: Starting:    System.Diagnostics.TextWriterTraceListener.Tests.dll
[19:27:14] fail: [FAIL] System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "")
[19:27:14] info: System.IO.IOException : File exists
[19:27:14] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(Int64 result, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(IntPtr result, String path, Boolean isDirectory)
[19:27:14] info:    at System.IO.Path.GetTempFileName()
[19:27:14] info:    at System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(String testName)
[19:27:14] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)
[19:27:14] fail: [FAIL] System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "><&")
[19:27:14] info: System.IO.IOException : File exists
[19:27:14] info:    at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(Int64 result, String path, Boolean isDirectory)
[19:27:14] info:    at Interop.CheckIo(IntPtr result, String path, Boolean isDirectory)
[19:27:14] info:    at System.IO.Path.GetTempFileName()
[19:27:14] info:    at System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(String testName)
[19:27:14] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, Span`1 args, BindingFlags invokeAttr)

Just for people reading this in the future, the list of failed tests:

System.CodeDom.Compiler.Tests.CodeCompilerTests.FromDom_ValidCodeCompileUnit_ReturnsExpected(compilationUnit: CodeCompileUnit { AssemblyCustomAttributes = [], EndDirectives = [], Namespaces = [], ReferencedAssemblies = ["assembly1", "assembly2"], StartDirectives = [], ... })
System.CodeDom.Compiler.Tests.CodeCompilerTests.GetResponseFileCmdArgs_ValidCmdArgs_ReturnsExpected(cmdArgs: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.CompileAssemblyFromSourceBatch_ValidSources_ReturnsExpected(sources: [null])
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ReturnsExpected(source: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSourceBatch_ValidSources_ReturnsExpected(sources: [""])
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ThrowsPlatformNotSupportedException(source: "")
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromDom_ValidCodeCompileUnit_ThrowsPlatformNotSupportedException(compilationUnit: CodeCompileUnit { AssemblyCustomAttributes = [], EndDirectives = [], Namespaces = [], ReferencedAssemblies = ["referenced", "assembly1"], StartDirectives = [], ... })
System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSourceBatch_ValidSources_ThrowsPlatformNotSupportedException(sources: [""])

System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"...)
System.Diagnostics.TextWriterTraceListenerTests.CtorsDelimiterTests.TestConstructorWithFileNameAndName(testName: "><&")
System.Diagnostics.TextWriterTraceListenerTests.XmlWriterTraceListenerTests.SingleArgumentConstructorTest

 ystem.ServiceModel.Syndication.Tests.BasicScenarioTests.SyndicationFeed_Write_RSS_Atom

The full set can be seen with that build url. I think #73408 broke these tests.

@eerhardt

Author: radical
Assignees: -
Labels:

arch-wasm, area-System.IO, os-windows, blocking-clean-ci, test-failure

Milestone: -

@radical radical added this to the 7.0.0 milestone Aug 10, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 10, 2022
@eerhardt
Copy link
Member

I can't repro this locally. Is it possible the machine's TEMP directories are full?

@radical
Copy link
Member Author

radical commented Aug 11, 2022

I will try to repro this too. Locally, and on CI.

@radical
Copy link
Member Author

radical commented Aug 11, 2022

I tried debugging this on helix with #73743 .

With the exception, I also printed the list of files, and directories in the temp path, before and after the mkdtemp call.
Log:

[06:39:10] fail: [FAIL] System.CodeDom.Compiler.Tests.CodeCompilerTests.FromSource_ValidSource_ThrowsPlatformNotSupportedException(source: "")
[06:39:10] info: Assert.Throws() Failure
[06:39:10] info: Expected: typeof(System.PlatformNotSupportedException)
[06:39:10] info: Actual:   typeof(System.Exception): io: System.IO.IOException: File exists
[06:39:10] info:              at Interop.ThrowExceptionForIoErrno(ErrorInfo errorInfo, String path, Boolean isDirectory)
[06:39:10] info:              at Interop.ThrowIOExceptionForLastError()
[06:39:10] info:              at System.IO.Directory.CreateTempSubdirectoryCore(String prefix) (hresult: 20 $
[06:39:10] info:           debug: Tries to create /tmp/XXXXXX in /tmp/
[06:39:10] info:           before: files:
[06:39:10] info:           before: directories: /tmp/iBahFK,/tmp/OLKNPB,/tmp/GnFpaC
[06:39:10] info:           after: files:
[06:39:10] info:           after: directories: /tmp/iBahFK,/tmp/OLKNPB,/tmp/GnFpaC
  • One thing to note here is that it was expecting a PNSE, so the native function shouldn't have been called at all?
  • Another, is that it says File exists, but the hresult from the error is 20 ( at System.IO.Directory.CreateTempSubdirectoryCore(String prefix) (hresult: 20) - which corresponds to ENODIR, IIUC.
    • so that looks like at least one issue
  • I think ENODIR here means that some part of the /tmp/ path doesn't exist? But I'm not sure what to make of it
  • As can be seen, the list of dirs is the same before, and after the call
  • And if you look farther in the log, for other tests, and you can see that there were other directories created too in /tmp

I'm not sure what exactly might be going on. Maybe somebody familiar with node/windows can help.

cc @pavelsavara @maraf

@danmoseley
Copy link
Member

Also note that CreateTempSubdirectoryCore ought to be passing the path to ThrowExceptionForIoErrno so that it appears in the error message.

@danmoseley

This comment was marked as off-topic.

@danmoseley

This comment was marked as off-topic.

@danmoseley

This comment was marked as off-topic.

@eerhardt
Copy link
Member

Also note that CreateTempSubdirectoryCore ought to be passing the path to ThrowExceptionForIoErrno so that it appears in the error message.

What path is that? We are building up a path template like:

/tmp/XXXXXX

And then passing that to mkdtemp, which will replace the XXXXXX with random letters. Would you really expect /tmp/XXXXXX to appear in the error message?

@pavelsavara pavelsavara self-assigned this Aug 11, 2022
@pavelsavara
Copy link
Member

pavelsavara commented Aug 11, 2022

@pavelsavara
Copy link
Member

@danmoseley
Copy link
Member

Would you really expect /tmp/XXXXXX to appear in the error message?

I figured it would prove it had /tmp on it, but fair enough.

@danmoseley
Copy link
Member

Seems I didn't understand how the stack fits together here 🙂

@pavelsavara
Copy link
Member

I'm wondering how CLOCK_MONOTONIC works here

@eerhardt
Copy link
Member

So it gives 100 retries of random names, and then gives up. Do we need to clean some /tmp folders in between runs? or ensure tests are cleaning out their tmp folders?

@eerhardt
Copy link
Member

Would one option here be:

For wasm only, in SystemNative_MkdTemp, write a loop of calls to mkdtemp, check if the error is EEXIST, delay for a bit, and then try again?

@pavelsavara
Copy link
Member

busy wait ? no thank you :)

@eerhardt
Copy link
Member

busy wait ? no thank you :)

It's better to throw an exception?

@tmds @omajid - do you happen to know how many retries a "normal" mkdtemp implementation tries before giving up? 100 retries seems to be awfully small.

@lewing
Copy link
Member

lewing commented Aug 14, 2022

this isn't urgent at all and we will be looking at the fs related syscalls for wasi in 8

@tmds
Copy link
Member

tmds commented Aug 17, 2022

So it gives 100 retries of random names, and then gives up.

100 retries ought to be enough for anybody 😄

I see the above is linked to musl sources, would we have this same problem on alpine?

The random function that is used is different: https://github.com/bminor/musl/blob/master/src/temp/__randname.c.
vs https://github.com/emscripten-core/emscripten/blob/e05e72d9c49fe15578e73934ce525a894d1b712a/system/lib/libc/musl/src/temp/__randname.c.

@eerhardt
Copy link
Member

The random function that is used is different

This gave me some insight as to why #73408 affected Path.GetTempFileName() on WASM and why it started failing. In the emscripten version, the random function uses:

	r = ts.tv_nsec*65537 ^ (uintptr_t)&ts / 16 + (uintptr_t)template;

Note that last part: + (uintptr_t)template, they are adding the pointer value into the randomness.

Looking at my change in #73408:

-            byte[] name = Encoding.UTF8.GetBytes(template);
+            string tempPath = Path.GetTempPath();
+            int tempPathByteCount = Encoding.UTF8.GetByteCount(tempPath);
+            int totalByteCount = tempPathByteCount + fileTemplate.Length + 1;

+            Span<byte> path = totalByteCount <= 256 ? stackalloc byte[256].Slice(0, totalByteCount) : new byte[totalByteCount];

Previously, we were always allocating the byte[] on the heap. Thus the pointer value was more than likely different every time we called GetTempFileName(). But, in order to reduce allocations, I changed the function to stackalloc the byte[], which means if GetTempFileName() is called twice in a row from the same method, the pointer value is going to be the same, because it will occupy the same space on the stack.

@danmoseley
Copy link
Member

Wow!

@danmoseley
Copy link
Member

Who owns next step on this one, and what is it?

@tmds
Copy link
Member

tmds commented Sep 7, 2022

I think the next step is to create our own random name function for wasm that has more entropy.

@danmoseley
Copy link
Member

Yup. Anyone interested..

@danmoseley
Copy link
Member

This, or some form of it, will need a backport into 7.0..

@pavelsavara
Copy link
Member

Fixing it upstream would take long time and we don't have easy way how to patch it in C code on our side.
Perhaps WASM specific version of SystemNative_MkdTemp would do it ?

@eerhardt
Copy link
Member

eerhardt commented Sep 7, 2022

We could also use a WASM specific C# method that always called Encoding.UTF8.GetBytes on the string. This would allocate memory on the heap, which would change the template pointer passed into the __randname function every time GetTempFileName was called.

@danmoseley
Copy link
Member

Depending on what other allocations are happening concurrently perhaps it might also need a random sized dummy allocation (e.g. after the first try)

radical added a commit to radical/performance that referenced this issue Sep 7, 2022
On wasm/aot, `System.IO.FileSystem/Perf.File`'s `CopyTo`, `CopyToOverride`, and `ReadAllBytes` benchmarks fail randomly with:
```
---> System.IO.IOException: The file '/tmp/' already exists.
  at BenchmarkDotNet.Autogenerated.Runnable_48.Run(IHost host, String benchmarkName)
  at System.Reflection.MethodInvoker.InterpretedInvoke(Object , Span`1 , BindingFlags )
  --- End of inner exception stack trace ---
  at BenchmarkDotNet.Autogenerated.UniqueProgramName.AfterAssemblyLoadingAttached(String[] args)
```

.. due to `Path.GetTempFileName()` being buggy - dotnet/runtime#73721.

In `SetupReadAllBytes`, we are building the test file path as:
```csharp
_testFilePath = Path.Combine(baseDir, Path.GetTempFileName())
```

.. but the `Path.GetTempFileName()` is not appropriate here, as it will
return a full path, and it will create the file. Instead we can use
`Path.GetRandomFileName()` which should avoid the underlying issue
completely.

Fixes dotnet/runtime#74104 .
adamsitnik pushed a commit to dotnet/performance that referenced this issue Sep 8, 2022
On wasm/aot, `System.IO.FileSystem/Perf.File`'s `CopyTo`, `CopyToOverride`, and `ReadAllBytes` benchmarks fail randomly with:
```
---> System.IO.IOException: The file '/tmp/' already exists.
  at BenchmarkDotNet.Autogenerated.Runnable_48.Run(IHost host, String benchmarkName)
  at System.Reflection.MethodInvoker.InterpretedInvoke(Object , Span`1 , BindingFlags )
  --- End of inner exception stack trace ---
  at BenchmarkDotNet.Autogenerated.UniqueProgramName.AfterAssemblyLoadingAttached(String[] args)
```

.. due to `Path.GetTempFileName()` being buggy - dotnet/runtime#73721.

In `SetupReadAllBytes`, we are building the test file path as:
```csharp
_testFilePath = Path.Combine(baseDir, Path.GetTempFileName())
```

.. but the `Path.GetTempFileName()` is not appropriate here, as it will
return a full path, and it will create the file. Instead we can use
`Path.GetRandomFileName()` which should avoid the underlying issue
completely.

Fixes dotnet/runtime#74104 .
@danmoseley danmoseley changed the title [wasm] Some IO tests failing on Windows/NodeJS Path.GetTempFileName() sometimes fails on WASM due to insufficient randomness Dec 15, 2022
@lewing
Copy link
Member

lewing commented Jan 24, 2023

@maraf what is the status on this? SyndicationFeed_Write_RSS_Atom is failing pretty frequently now

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 24, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.IO disabled-test The test is disabled in source code against the issue os-windows test-failure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants