Fix GetTempFileName() on Windows

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

@@ -1243,8 +1243,8 @@
   <data name="Argument_InvalidParamInfo" xml:space="preserve">
     <value>Invalid type for ParameterInfo member in Attribute class.</value>
   </data>
-  <data name="Argument_InvalidPathChars" xml:space="preserve">
-    <value>Illegal characters in path.</value>
+  <data name="Argument_NullCharInPath" xml:space="preserve">
+    <value>Null character in path.</value>
   </data>
   <data name="Argument_InvalidResourceCultureName" xml:space="preserve">
     <value>The given culture name '{0}' cannot be used to locate a resource file. Resource filenames must consist of only letters, numbers, hyphens or underscores.</value>

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

@@ -1642,9 +1642,6 @@
     <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetSystemTimes.cs">
       <Link>Common\Interop\Windows\Kernel32\Interop.GetSystemTimes.cs</Link>
     </Compile>
-    <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetTempFileNameW.cs">
-      <Link>Common\Interop\Windows\Kernel32\Interop.GetTempFileNameW.cs</Link>
-    </Compile>
     <Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.GetVolumeInformation.cs">
       <Link>Common\Interop\Windows\Kernel32\Interop.GetVolumeInformation.cs</Link>
     </Compile>

src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerableFactory.cs

@@ -30,10 +30,10 @@ internal static bool NormalizeInputs(ref string directory, ref string expression
                 throw new ArgumentException(SR.Arg_Path2IsRooted, nameof(expression));
 
             if (expression.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars, expression);
+                throw new ArgumentException(SR.Argument_NullCharInPath, expression);
 
             if (directory.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars, directory);
+                throw new ArgumentException(SR.Argument_NullCharInPath, directory);
 
             // We always allowed breaking the passed ref directory and filter to be separated
             // any way the user wanted. Looking for "C:\foo\*.cs" could be passed as "C:\" and

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs

@@ -24,7 +24,7 @@ internal static void VerifyValidPath(string path, string argName)
             ArgumentException.ThrowIfNullOrEmpty(path, argName);
             if (path.Contains('\0'))
             {
-                throw new ArgumentException(SR.Argument_InvalidPathChars, argName);
+                throw new ArgumentException(SR.Argument_NullCharInPath, argName);
             }
         }
 

src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs

@@ -28,7 +28,7 @@ public static string GetFullPath(string path)
             ArgumentException.ThrowIfNullOrEmpty(path);
 
             if (path.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars, nameof(path));
+                throw new ArgumentException(SR.Argument_NullCharInPath, nameof(path));
 
             return GetFullPathInternal(path);
         }
@@ -42,7 +42,7 @@ public static string GetFullPath(string path, string basePath)
                 throw new ArgumentException(SR.Arg_BasePathNotFullyQualified, nameof(basePath));
 
             if (basePath.Contains('\0') || path.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars);
+                throw new ArgumentException(SR.Argument_NullCharInPath);
 
             if (IsPathFullyQualified(path))
                 return GetFullPathInternal(path);
@@ -111,7 +111,8 @@ public static unsafe string GetTempFileName()
             // Create, open, and close the temp file.
             fixed (byte* pPath = path)
             {
-                IntPtr fd = Interop.CheckIo(Interop.Sys.MksTemps(pPath, SuffixByteLength));
+                // if this returns ENOENT it's because TMPDIR doesn't exist, so isDirError:true
+                IntPtr fd = Interop.CheckIo(Interop.Sys.MksTemps(pPath, SuffixByteLength), tempPath, isDirError:true);
                 Interop.Sys.Close(fd); // ignore any errors from close; nothing to do if cleanup isn't possible
             }
 

src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs

@@ -53,7 +53,7 @@ public static string GetFullPath(string path)
             // This is because the nulls will signal the end of the string to Win32 and therefore have
             // unpredictable results.
             if (path.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars, nameof(path));
+                throw new ArgumentException(SR.Argument_NullCharInPath, nameof(path));
 
             return GetFullPathInternal(path);
         }
@@ -67,7 +67,7 @@ public static string GetFullPath(string path, string basePath)
                 throw new ArgumentException(SR.Arg_BasePathNotFullyQualified, nameof(basePath));
 
             if (basePath.Contains('\0') || path.Contains('\0'))
-                throw new ArgumentException(SR.Argument_InvalidPathChars);
+                throw new ArgumentException(SR.Argument_NullCharInPath);
 
             if (IsPathFullyQualified(path))
                 return GetFullPathInternal(path);
@@ -208,25 +208,57 @@ static uint GetTempPathW(int bufferLen, ref char buffer)
         // name on disk.
         public static string GetTempFileName()
         {
-            var tempPathBuilder = new ValueStringBuilder(stackalloc char[PathInternal.MaxShortPath]);
-
-            GetTempPath(ref tempPathBuilder);
-
-            var builder = new ValueStringBuilder(stackalloc char[PathInternal.MaxShortPath]);
+            // Avoid GetTempFileNameW because it is limited to 0xFFFF possibilities, which both
+            // means that it may have to make many attempts to create the file before
+            // finding an unused name, and also that if an app "leaks" such temp files,
+            // it can prevent GetTempFileNameW succeeding at all.
+            //
+            // To make this a little more robust, generate our own name with more
+            // entropy. We could use GetRandomFileName() here, but for consistency
+            // with Unix and to retain the ".tmp" extension we will use the "tmpXXXXXX.tmp" pattern.
+            // Using 32 characters for convenience, that gives us 32^^6 ~= 10^^9 possibilities,
+            // but we'll still loop to handle the unlikely case the file already exists.
+
+            const int KeyLength = 4;
+            byte* bytes = stackalloc byte[KeyLength];
+
+            Span<char> span = stackalloc char[13]; // tmpXXXXXX.tmp
+            span[0] = span[10] = 't';
+            span[1] = span[11] = 'm';
+            span[2] = span[12] = 'p';
+            span[9] = '.';
+
+            int i = 0;
+            while (true)
+            {
+                Interop.GetRandomBytes(bytes, KeyLength);  // 4 bytes = more than 6 x 5 bits
 
-            uint result = Interop.Kernel32.GetTempFileNameW(
-                ref tempPathBuilder.GetPinnableReference(), "tmp", 0, ref builder.GetPinnableReference());
+                byte b0 = bytes[0];
+                byte b1 = bytes[1];
+                byte b2 = bytes[2];
+                byte b3 = bytes[3];
 
-            tempPathBuilder.Dispose();
+                span[3] = (char)Base32Char[b0 & 0b0001_1111];
+                span[4] = (char)Base32Char[b1 & 0b0001_1111];
+                span[5] = (char)Base32Char[b2 & 0b0001_1111];
+                span[6] = (char)Base32Char[b3 & 0b0001_1111];
+                span[7] = (char)Base32Char[((b0 & 0b1110_0000) >> 5) | ((b1 & 0b1100_0000) >> 3)];
+                span[8] = (char)Base32Char[((b2 & 0b1110_0000) >> 5) | ((b3 & 0b1100_0000) >> 3)];
 
-            if (result == 0)
-                throw Win32Marshal.GetExceptionForLastWin32Error();
+                string path = string.Concat(Path.GetTempPath(), span);
 
-            builder.Length = builder.RawChars.IndexOf('\0');
+                try
+                {
+                    File.OpenHandle(path, FileMode.CreateNew, FileAccess.Write).Dispose();
+                }
+                catch (IOException ex) when (i < 100 && Win32Marshal.TryMakeWin32ErrorCodeFromHR(ex.HResult) == Interop.Errors.ERROR_FILE_EXISTS)
+                {
+                    i++; // Don't let unforeseen circumstances cause us to loop forever
+                    continue; // File already exists: very, very unlikely
+                }
 
-            string path = PathHelper.Normalize(ref builder);
-            builder.Dispose();
-            return path;
+                return path;
+            }
         }
 
         // Tests if the given path contains a root. A path is considered rooted

src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs

@@ -182,7 +182,18 @@ public void GetTempFileName()
             try
             {
                 Assert.True(File.Exists(tmpFile));
-                Assert.Equal(".tmp", Path.GetExtension(tmpFile), ignoreCase: true, ignoreLineEndingDifferences: false, ignoreWhiteSpaceDifferences: false);
+                string fileName = Path.GetFileName(tmpFile);
+                Assert.StartsWith("tmp", fileName);
+                Assert.Equal("tmpXXXXXX.tmp".Length, fileName.Length);
+                Assert.EndsWith(".tmp", fileName);
+
+                const string ValidChars = "abcdefghijklmnopqrstuvwxyz0123456789";
+                for (int i = 3; i < 9; i++)
+                {
+                    // Unix allows upper and lower case
+                    Assert.True(ValidChars.Contains(char.ToLowerInvariant(fileName[i])));
+                }
+
                 Assert.Equal(-1, tmpFile.IndexOfAny(Path.GetInvalidPathChars()));
                 using (FileStream fs = File.OpenRead(tmpFile))
                 {
@@ -222,6 +233,42 @@ public void GetTempFileNameTempUnicode()
             }).Dispose();
         }
 
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public void GetTempFileNameTempInvalidTemp()
+        {
+            // GetTempFileName has retries in it. It shouldn't hang in the face of a
+            // bad temp path.
+            RemoteExecutor.Invoke(() =>
+            {
+                string goodTemp = Path.GetTempPath();
+                string tempEnvVar = OperatingSystem.IsWindows() ? "TMP" : "TMPDIR";
+
+                try
+                {
+                    string badTemp = Path.GetTempFileName();
+                    Assert.True(File.Exists(badTemp));
+
+                    Environment.SetEnvironmentVariable(tempEnvVar, badTemp);
+
+                    Assert.StartsWith(badTemp, Path.GetTempPath());
+
+                    Assert.Throws<DirectoryNotFoundException>(() => Path.GetTempFileName()); // file not directory
+                    File.Delete(badTemp);
+                    Assert.Throws<DirectoryNotFoundException>(() => Path.GetTempFileName()); // non existent
+
+                    if (OperatingSystem.IsWindows())
+                    {
+                        Environment.SetEnvironmentVariable(tempEnvVar, "|||");
+                        Assert.Throws<IOException>(() => Path.GetTempFileName()); // invalid path
+                    }
+                }
+                finally
+                {
+                    Environment.SetEnvironmentVariable(tempEnvVar, goodTemp);
+                }
+            }).Dispose();
+        }
+
         [Fact]
         public void GetFullPath_InvalidArgs()
         {