From 5af7a7898b95c4a4220c3f74ea4530c96aa88590 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Tue, 30 Aug 2022 20:49:12 -0600 Subject: [PATCH 01/14] GetTempFileName() --- .../src/Resources/Strings.resx | 2 +- .../src/System/IO/Path.Windows.cs | 68 ++++++++++++++----- .../tests/System/IO/PathTests.cs | 2 + 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 9b8eee748e71..894a6c4c4748 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -1244,7 +1244,7 @@ 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/IO/Path.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs index cb5943720131..05abd92babe9 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 @@ -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]); - - uint result = Interop.Kernel32.GetTempFileNameW( - ref tempPathBuilder.GetPinnableReference(), "tmp", 0, ref builder.GetPinnableReference()); - - tempPathBuilder.Dispose(); - - if (result == 0) - throw Win32Marshal.GetExceptionForLastWin32Error(); - - builder.Length = builder.RawChars.IndexOf('\0'); + // 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. + + while (true) + { + var builder = new ValueStringBuilder(stackalloc char[PathInternal.MaxShortPath]); + builder.Append(Path.GetTempPath()); + Debug.Assert(builder[builder.Length - 1] == PathInternal.DirectorySeparatorChar); + + Span span = builder.AppendSpan(13); // "tmpXXXXXX.tmp" + + const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits + byte* bytes = stackalloc byte[KeyLength]; + Interop.GetRandomBytes(bytes, KeyLength); + + byte b0 = bytes[0]; + byte b1 = bytes[1]; + byte b2 = bytes[2]; + byte b3 = bytes[3]; + + span[0] = span[10] = 't'; + span[1] = span[11] = 'm'; + span[2] = span[12] = 'p'; + 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)]; + span[9] = '.'; + + string path = builder.ToString(); + try + { + new FileStream(path, FileMode.CreateNew).Dispose(); + } + catch (IOException) + { + 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 aa1709024868..269776f4ec25 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -182,6 +182,8 @@ public void GetTempFileName() try { Assert.True(File.Exists(tmpFile)); + Assert.StartsWith("tmp", Path.GetFileName(tmpFile)); + Assert.Equal("tmpXXXXXX.tmp".Length, Path.GetFileName(tmpFile).Length); Assert.Equal(".tmp", Path.GetExtension(tmpFile), ignoreCase: true, ignoreLineEndingDifferences: false, ignoreWhiteSpaceDifferences: false); Assert.Equal(-1, tmpFile.IndexOfAny(Path.GetInvalidPathChars())); using (FileStream fs = File.OpenRead(tmpFile)) From d376c908f617ce251bf321afbeac046c6c32c32b Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 12:32:09 -0600 Subject: [PATCH 02/14] feedback --- .../src/System/IO/Path.Windows.cs | 58 ++++++++++--------- .../tests/System/IO/PathTests.cs | 14 ++++- 2 files changed, 41 insertions(+), 31 deletions(-) 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 05abd92babe9..87fd996b484e 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 @@ -219,38 +219,40 @@ public static string GetTempFileName() // 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. + string tempPath = Path.GetTempPath(); + byte* bytes = stackalloc byte[KeyLength]; + while (true) { - var builder = new ValueStringBuilder(stackalloc char[PathInternal.MaxShortPath]); - builder.Append(Path.GetTempPath()); - Debug.Assert(builder[builder.Length - 1] == PathInternal.DirectorySeparatorChar); - - Span span = builder.AppendSpan(13); // "tmpXXXXXX.tmp" - - const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits - byte* bytes = stackalloc byte[KeyLength]; - Interop.GetRandomBytes(bytes, KeyLength); - - byte b0 = bytes[0]; - byte b1 = bytes[1]; - byte b2 = bytes[2]; - byte b3 = bytes[3]; - - span[0] = span[10] = 't'; - span[1] = span[11] = 'm'; - span[2] = span[12] = 'p'; - 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)]; - span[9] = '.'; - - string path = builder.ToString(); + string path = string.Create(tempPath.Length + 13 /* tmpXXXXXX.tmp */, (IntPtr)bytes, (span, state) => + { + tempPath.TryCopyTo(span); + + span = span.Slice(tempPath.Length); + + const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits + Interop.GetRandomBytes(bytes, KeyLength); + + byte b0 = bytes[0]; + byte b1 = bytes[1]; + byte b2 = bytes[2]; + byte b3 = bytes[3]; + + span[0] = span[10] = 't'; + span[1] = span[11] = 'm'; + span[2] = span[12] = 'p'; + 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)]; + span[9] = '.'; + }); + try { - new FileStream(path, FileMode.CreateNew).Dispose(); + File.OpenHandle(path, FileMode.CreateNew, FileAccess.Write).Dispose(); } catch (IOException) { 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 269776f4ec25..77fafa0147c8 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -182,9 +182,17 @@ public void GetTempFileName() try { Assert.True(File.Exists(tmpFile)); - Assert.StartsWith("tmp", Path.GetFileName(tmpFile)); - Assert.Equal("tmpXXXXXX.tmp".Length, Path.GetFileName(tmpFile).Length); - 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 = "abcdefghijklmnopqrstuvwxyz012345"; + for (int i = 3; i < 9; i++) + { + Assert.True(validChars.Contains(fileName[i])); + } + Assert.Equal(-1, tmpFile.IndexOfAny(Path.GetInvalidPathChars())); using (FileStream fs = File.OpenRead(tmpFile)) { From a0764f2a286a8d4bc4903b535d23ea87bc021c1b Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 12:56:19 -0600 Subject: [PATCH 03/14] feedback --- .../src/System/IO/Path.Windows.cs | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) 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 87fd996b484e..ae3cabf71a02 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 @@ -219,36 +219,31 @@ public static string GetTempFileName() // 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. - string tempPath = Path.GetTempPath(); byte* bytes = stackalloc byte[KeyLength]; + Span span = stackalloc char[13]; // tmpXXXXXX.tmp while (true) { - string path = string.Create(tempPath.Length + 13 /* tmpXXXXXX.tmp */, (IntPtr)bytes, (span, state) => - { - tempPath.TryCopyTo(span); - - span = span.Slice(tempPath.Length); - - const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits - Interop.GetRandomBytes(bytes, KeyLength); - - byte b0 = bytes[0]; - byte b1 = bytes[1]; - byte b2 = bytes[2]; - byte b3 = bytes[3]; - - span[0] = span[10] = 't'; - span[1] = span[11] = 'm'; - span[2] = span[12] = 'p'; - 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)]; - span[9] = '.'; - }); + const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits + Interop.GetRandomBytes(bytes, KeyLength); + + byte b0 = bytes[0]; + byte b1 = bytes[1]; + byte b2 = bytes[2]; + byte b3 = bytes[3]; + + span[0] = span[10] = 't'; + span[1] = span[11] = 'm'; + span[2] = span[12] = 'p'; + 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)]; + span[9] = '.'; + + string path = string.Concat(Path.GetTempPath(), span); try { From ee2851036b0de77a7af705721d0293d9289efdbb Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 14:11:01 -0600 Subject: [PATCH 04/14] Unix allows caps --- .../System.Runtime.Extensions/tests/System/IO/PathTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 77fafa0147c8..b49e73976146 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -187,10 +187,10 @@ public void GetTempFileName() Assert.Equal("tmpXXXXXX.tmp".Length, fileName.Length); Assert.EndsWith(".tmp", fileName); - const string validChars = "abcdefghijklmnopqrstuvwxyz012345"; + const string validChars = "abcdefghijklmnopqrstuvwxyz0123456789"; for (int i = 3; i < 9; i++) { - Assert.True(validChars.Contains(fileName[i])); + Assert.True(validChars.Contains(char.ToLowerInvariant(fileName[i]))); } Assert.Equal(-1, tmpFile.IndexOfAny(Path.GetInvalidPathChars())); From 0948de5942e5d3ba09876b31f90db600387cbd56 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 14:20:40 -0600 Subject: [PATCH 05/14] avoid hang --- .../System.Private.CoreLib/src/System/IO/Path.Windows.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 ae3cabf71a02..040f8451d506 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 @@ -222,6 +222,7 @@ public static string GetTempFileName() byte* bytes = stackalloc byte[KeyLength]; Span span = stackalloc char[13]; // tmpXXXXXX.tmp + int i = 0; while (true) { const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits @@ -249,8 +250,9 @@ public static string GetTempFileName() { File.OpenHandle(path, FileMode.CreateNew, FileAccess.Write).Dispose(); } - catch (IOException) + catch (IOException) when (i < 100) { + i++; // If TMP is invalid, don't loop forever continue; // File already exists: very, very unlikely } From 0c7d3e9d5dca754ae4ba2640341c5af3c9083cdf Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 17:32:30 -0600 Subject: [PATCH 06/14] feedback --- .../src/System/IO/Path.Windows.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 040f8451d506..119ed5ea78f9 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 @@ -219,30 +219,31 @@ public static string GetTempFileName() // 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) { - const int KeyLength = 4; // 4 bytes = more than 6 x 5 bits - Interop.GetRandomBytes(bytes, KeyLength); + Interop.GetRandomBytes(bytes, KeyLength); // 4 bytes = more than 6 x 5 bits byte b0 = bytes[0]; byte b1 = bytes[1]; byte b2 = bytes[2]; byte b3 = bytes[3]; - span[0] = span[10] = 't'; - span[1] = span[11] = 'm'; - span[2] = span[12] = 'p'; 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)]; - span[9] = '.'; string path = string.Concat(Path.GetTempPath(), span); @@ -250,9 +251,9 @@ public static string GetTempFileName() { File.OpenHandle(path, FileMode.CreateNew, FileAccess.Write).Dispose(); } - catch (IOException) when (i < 100) + catch (IOException ex) when (i < 100 && Win32Marshal.TryMakeWin32ErrorCodeFromHR(ex.HResult) == Interop.Errors.ERROR_FILE_EXISTS) { - i++; // If TMP is invalid, don't loop forever + i++; // If there's a read-only temp volume, don't loop forever continue; // File already exists: very, very unlikely } From eeac0f294933c56465ed937fefb7eda1794b9178 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 17:50:32 -0600 Subject: [PATCH 07/14] Test for bad temp --- .../tests/System/IO/PathTests.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) 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 b49e73976146..11701442add0 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -232,6 +232,36 @@ 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 + } + finally + { + Environment.SetEnvironmentVariable(tempEnvVar, goodTemp); + } + }).Dispose(); + } + [Fact] public void GetFullPath_InvalidArgs() { From 7b537bb71046c283a9bcdb1c13dee23462032d03 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 17:52:24 -0600 Subject: [PATCH 08/14] Remove interop --- .../Windows/Kernel32/Interop.GetTempFileNameW.cs | 13 ------------- .../src/System.Private.CoreLib.Shared.projitems | 3 --- 2 files changed, 16 deletions(-) delete mode 100644 src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetTempFileNameW.cs 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 fba61bf01a10..000000000000 --- 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/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems index 86bde088f4ad..bee616454c0e 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 @@ -1642,9 +1642,6 @@ Common\Interop\Windows\Kernel32\Interop.GetSystemTimes.cs - - Common\Interop\Windows\Kernel32\Interop.GetTempFileNameW.cs - Common\Interop\Windows\Kernel32\Interop.GetVolumeInformation.cs From 88a827ad727e411f26015ea6b0d6cef5ab66a861 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Wed, 31 Aug 2022 17:55:54 -0600 Subject: [PATCH 09/14] IOException variation --- .../System.Runtime.Extensions/tests/System/IO/PathTests.cs | 3 +++ 1 file changed, 3 insertions(+) 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 11701442add0..dae790487424 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -254,6 +254,9 @@ public void GetTempFileNameTempInvalidTemp() Assert.Throws(() => Path.GetTempFileName()); // file not directory File.Delete(badTemp); Assert.Throws(() => Path.GetTempFileName()); // non existent + + Environment.SetEnvironmentVariable(tempEnvVar, "|||"); + Assert.Throws(() => Path.GetTempFileName()); // non existent } finally { From face677db9ce8ae0b87a746c4864c564b7d037ea Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Thu, 1 Sep 2022 11:51:56 -0600 Subject: [PATCH 10/14] comment --- .../System.Private.CoreLib/src/System/IO/Path.Windows.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 119ed5ea78f9..4a2cc11f8fc3 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 @@ -253,7 +253,7 @@ public static string GetTempFileName() } catch (IOException ex) when (i < 100 && Win32Marshal.TryMakeWin32ErrorCodeFromHR(ex.HResult) == Interop.Errors.ERROR_FILE_EXISTS) { - i++; // If there's a read-only temp volume, don't loop forever + i++; // Don't let unforeseen circumstances cause us to loop forever continue; // File already exists: very, very unlikely } From 94bad6d9ab09237ee63aa15367a25eed892fcd07 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Thu, 1 Sep 2022 11:53:02 -0600 Subject: [PATCH 11/14] rename --- .../System.Runtime.Extensions/tests/System/IO/PathTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 dae790487424..9201a3acbeac 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -187,10 +187,11 @@ public void GetTempFileName() Assert.Equal("tmpXXXXXX.tmp".Length, fileName.Length); Assert.EndsWith(".tmp", fileName); - const string validChars = "abcdefghijklmnopqrstuvwxyz0123456789"; + const string ValidChars = "abcdefghijklmnopqrstuvwxyz0123456789"; for (int i = 3; i < 9; i++) { - Assert.True(validChars.Contains(char.ToLowerInvariant(fileName[i]))); + // Unix allows upper and lower case + Assert.True(ValidChars.Contains(char.ToLowerInvariant(fileName[i]))); } Assert.Equal(-1, tmpFile.IndexOfAny(Path.GetInvalidPathChars())); From 71f194f07aa2bdbe1168ea2aa3aa95b950d8dfb5 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Thu, 1 Sep 2022 11:53:43 -0600 Subject: [PATCH 12/14] comment --- .../System.Runtime.Extensions/tests/System/IO/PathTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9201a3acbeac..3c9b1dc12259 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -257,7 +257,7 @@ public void GetTempFileNameTempInvalidTemp() Assert.Throws(() => Path.GetTempFileName()); // non existent Environment.SetEnvironmentVariable(tempEnvVar, "|||"); - Assert.Throws(() => Path.GetTempFileName()); // non existent + Assert.Throws(() => Path.GetTempFileName()); // invalid path } finally { From 337f84141ff10c1d04a05eb4f728501c8a387956 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Thu, 1 Sep 2022 11:56:29 -0600 Subject: [PATCH 13/14] rename resource --- .../System.Private.CoreLib/src/Resources/Strings.resx | 2 +- .../src/System/IO/Enumeration/FileSystemEnumerableFactory.cs | 4 ++-- .../System.Private.CoreLib/src/System/IO/FileSystem.cs | 2 +- .../System.Private.CoreLib/src/System/IO/Path.Unix.cs | 4 ++-- .../System.Private.CoreLib/src/System/IO/Path.Windows.cs | 4 ++-- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 894a6c4c4748..b961ea2eea71 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -1243,7 +1243,7 @@ Invalid type for ParameterInfo member in Attribute class. - + Null character in path. 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 085983305bea..dbe0a624ed82 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 392791fd462b..21f54d0c864e 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 edb8f35dee50..65ac631afa4a 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); 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 4a2cc11f8fc3..8f31c70c96cb 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); From fd5b7e37da32b13f186b59450b318ecedf83e150 Mon Sep 17 00:00:00 2001 From: Dan Moseley Date: Thu, 1 Sep 2022 18:14:01 -0600 Subject: [PATCH 14/14] Fix GetTempPath on Unix to throw correct exception --- .../System.Private.CoreLib/src/System/IO/Path.Unix.cs | 3 ++- .../System.Runtime.Extensions/tests/System/IO/PathTests.cs | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) 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 65ac631afa4a..c6b4d904e258 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 @@ -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.Runtime.Extensions/tests/System/IO/PathTests.cs b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs index 3c9b1dc12259..b248728a79da 100644 --- a/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs +++ b/src/libraries/System.Runtime.Extensions/tests/System/IO/PathTests.cs @@ -256,8 +256,11 @@ public void GetTempFileNameTempInvalidTemp() File.Delete(badTemp); Assert.Throws(() => Path.GetTempFileName()); // non existent - Environment.SetEnvironmentVariable(tempEnvVar, "|||"); - Assert.Throws(() => Path.GetTempFileName()); // invalid path + if (OperatingSystem.IsWindows()) + { + Environment.SetEnvironmentVariable(tempEnvVar, "|||"); + Assert.Throws(() => Path.GetTempFileName()); // invalid path + } } finally {