diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetTempFileNameW.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetTempFileNameW.cs deleted file mode 100644 index fba61bf01a1029..00000000000000 --- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetTempFileNameW.cs +++ /dev/null @@ -1,13 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Runtime.InteropServices; - -internal static partial class Interop -{ - internal static partial class Kernel32 - { - [LibraryImport(Libraries.Kernel32, SetLastError = true, StringMarshalling = StringMarshalling.Utf16)] - internal static partial uint GetTempFileNameW(ref char lpPathName, string lpPrefixString, uint uUnique, ref char lpTempFileName); - } -} diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 9b8eee748e7132..b961ea2eea716c 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -1243,8 +1243,8 @@ Invalid type for ParameterInfo member in Attribute class. - - Illegal characters in path. + + Null character in path. The given culture name '{0}' cannot be used to locate a resource file. Resource filenames must consist of only letters, numbers, hyphens or underscores. diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index c334d0a1dbd908..e0c795ebb994a5 100644 --- a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems +++ b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems @@ -1644,9 +1644,6 @@ Common\Interop\Windows\Kernel32\Interop.GetSystemTimes.cs - - Common\Interop\Windows\Kernel32\Interop.GetTempFileNameW.cs - Common\Interop\Windows\Kernel32\Interop.GetVolumeInformation.cs diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerableFactory.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerableFactory.cs index 085983305bea9c..dbe0a624ed82b0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerableFactory.cs +++ b/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 diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs index 392791fd462b68..21f54d0c864eff 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.cs +++ b/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); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs index edb8f35dee500f..c6b4d904e25865 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs +++ b/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 } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs index cb5943720131fa..8f31c70c96cb86 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs +++ b/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 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 diff --git a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs index aa1709024868b7..b248728a79da0a 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/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(() => Path.GetTempFileName()); // file not directory + File.Delete(badTemp); + Assert.Throws(() => Path.GetTempFileName()); // non existent + + if (OperatingSystem.IsWindows()) + { + Environment.SetEnvironmentVariable(tempEnvVar, "|||"); + Assert.Throws(() => Path.GetTempFileName()); // invalid path + } + } + finally + { + Environment.SetEnvironmentVariable(tempEnvVar, goodTemp); + } + }).Dispose(); + } + [Fact] public void GetFullPath_InvalidArgs() {