Path.GetTempFileName() sometimes fails on WASM due to insufficient randomness #73721
Comments
radical
added
arch-wasm
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsBuild, seen on multiple PRs with no related changes. Which suggests that this is on .. and .. Just for people reading this in the future, the list of failed tests: The full set can be seen with that build url. I think #73408 broke these tests.
|
|
I can't repro this locally. Is it possible the machine's TEMP directories are full? |
|
I will try to repro this too. Locally, and on CI. |
|
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
I'm not sure what exactly might be going on. Maybe somebody familiar with node/windows can help. |
|
Also note that CreateTempSubdirectoryCore ought to be passing the path to ThrowExceptionForIoErrno so that it appears in the error message. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
What path is that? We are building up a path template like:
And then passing that to |
|
So I think it's not random enough. |
|
oh, it's even more direct to timer, which is not very precise. |
I figured it would prove it had /tmp on it, but fair enough. |
|
Seems I didn't understand how the stack fits together here 🙂 |
|
I'm wondering how CLOCK_MONOTONIC works here |
|
So it gives 100 retries of random names, and then gives up. Do we need to clean some |
|
Would one option here be: For wasm only, in SystemNative_MkdTemp, write a loop of calls to |
|
busy wait ? no thank you :) |
It's better to throw an exception? @tmds @omajid - do you happen to know how many retries a "normal" |
|
this isn't urgent at all and we will be looking at the fs related syscalls for wasi in 8 |
100 retries ought to be enough for anybody 😄
The random function that is used is different: https://github.com/bminor/musl/blob/master/src/temp/__randname.c. |
This gave me some insight as to why #73408 affected Note that last part: 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 |
|
Wow! |
|
Who owns next step on this one, and what is it? |
|
I think the next step is to create our own random name function for wasm that has more entropy. |
|
Yup. Anyone interested.. |
|
This, or some form of it, will need a backport into 7.0.. |
|
Fixing it upstream would take long time and we don't have easy way how to patch it in C code on our side. |
|
We could also use a WASM specific C# method that always called |
|
Depending on what other allocations are happening concurrently perhaps it might also need a random sized dummy allocation (e.g. after the first try) |
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 .
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
changed the title
|
@maraf what is the status on this? SyndicationFeed_Write_RSS_Atom is failing pretty frequently now |
ghost
added
the
in-pr
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
Build, seen on multiple PRs with no related changes. Which suggests that this is on
main. Failures like:.. and ..
Just for people reading this in the future, the list of failed tests:
The full set can be seen with that build url. I think #73408 broke these tests.
@eerhardt
The text was updated successfully, but these errors were encountered: