Fix race condition in Environment.GetEnvironmentVariable (#42608)

diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/CLRConfig.cs b/src/coreclr/src/System.Private.CoreLib/src/System/CLRConfig.cs
index 9ac34e4..cb2a032 100644 (file)
--- a/src/coreclr/src/System.Private.CoreLib/src/System/CLRConfig.cs
+++ b/src/coreclr/src/System.Private.CoreLib/src/System/CLRConfig.cs
@@ -28,7 +28,7 @@ namespace System
             // abstractions where reasonably possible.
 
             Span<char> buffer = stackalloc char[32];
-            int length = Interop.Kernel32.GetEnvironmentVariable(environmentName, buffer);
+            uint length = Interop.Kernel32.GetEnvironmentVariable(environmentName, ref buffer.GetPinnableReference(), (uint)buffer.Length);
             switch (length)
             {
                 case 1:

diff --git a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentVariable.cs b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentVariable.cs
index 069b7dd..74e2a02 100644 (file)
--- a/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentVariable.cs
+++ b/src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetEnvironmentVariable.cs
@@ -8,15 +8,7 @@ internal static partial class Interop
 {
     internal static partial class Kernel32
     {
-        internal static unsafe int GetEnvironmentVariable(string lpName, Span<char> buffer)
-        {
-            fixed (char* bufferPtr = &MemoryMarshal.GetReference(buffer))
-            {
-                return GetEnvironmentVariable(lpName, bufferPtr, buffer.Length);
-            }
-        }
-
         [DllImport(Libraries.Kernel32, EntryPoint = "GetEnvironmentVariableW", SetLastError = true, CharSet = CharSet.Unicode, ExactSpelling = true)]
-        private static extern unsafe int GetEnvironmentVariable(string lpName, char* lpBuffer, int nSize);
+        internal static extern unsafe uint GetEnvironmentVariable(string lpName, ref char lpBuffer, uint nSize);
     }
 }

diff --git a/src/libraries/Common/src/System/Text/ValueStringBuilder.cs b/src/libraries/Common/src/System/Text/ValueStringBuilder.cs
index fbd8950..f56ca2d 100644 (file)
--- a/src/libraries/Common/src/System/Text/ValueStringBuilder.cs
+++ b/src/libraries/Common/src/System/Text/ValueStringBuilder.cs
@@ -44,7 +44,11 @@ namespace System.Text
 
         public void EnsureCapacity(int capacity)
         {
-            if (capacity > _chars.Length)
+            // This is not expected to be called this with negative capacity
+            Debug.Assert(capacity >= 0);
+
+            // If the caller has a bug and calls this with negative capacity, make sure to call Grow to throw an exception.
+            if ((uint)capacity > (uint)_chars.Length)
                 Grow(capacity - _pos);
         }
 
@@ -283,7 +287,8 @@ namespace System.Text
             Debug.Assert(additionalCapacityBeyondPos > 0);
             Debug.Assert(_pos > _chars.Length - additionalCapacityBeyondPos, "Grow called incorrectly, no resize is needed.");
 
-            char[] poolArray = ArrayPool<char>.Shared.Rent(Math.Max(_pos + additionalCapacityBeyondPos, _chars.Length * 2));
+            // Make sure to let Rent throw an exception if the caller has a bug and the desired capacity is negative
+            char[] poolArray = ArrayPool<char>.Shared.Rent((int)Math.Max((uint)(_pos + additionalCapacityBeyondPos), (uint)_chars.Length * 2));
 
             _chars.Slice(0, _pos).CopyTo(poolArray);
 

diff --git a/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs
index e074d2f..15f027c 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Environment.Variables.Windows.cs
@@ -1,10 +1,10 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Buffers;
 using System.Collections;
 using System.Diagnostics;
 using System.Runtime.InteropServices;
+using System.Text;
 
 namespace System
 {
@@ -12,36 +12,22 @@ namespace System
     {
         private static string? GetEnvironmentVariableCore(string variable)
         {
-            Span<char> buffer = stackalloc char[128]; // a somewhat reasonable default size
-            int requiredSize = Interop.Kernel32.GetEnvironmentVariable(variable, buffer);
+            var builder = new ValueStringBuilder(stackalloc char[128]);
 
-            if (requiredSize == 0 && Marshal.GetLastWin32Error() == Interop.Errors.ERROR_ENVVAR_NOT_FOUND)
+            uint length;
+            while ((length = Interop.Kernel32.GetEnvironmentVariable(variable, ref builder.GetPinnableReference(), (uint)builder.Capacity)) > builder.Capacity)
             {
-                return null;
+                builder.EnsureCapacity((int)length);
             }
 
-            if (requiredSize <= buffer.Length)
+            if (length == 0 && Marshal.GetLastWin32Error() == Interop.Errors.ERROR_ENVVAR_NOT_FOUND)
             {
-                return new string(buffer.Slice(0, requiredSize));
+                builder.Dispose();
+                return null;
             }
 
-            char[] chars = ArrayPool<char>.Shared.Rent(requiredSize);
-            try
-            {
-                buffer = chars;
-                requiredSize = Interop.Kernel32.GetEnvironmentVariable(variable, buffer);
-                if ((requiredSize == 0 && Marshal.GetLastWin32Error() == Interop.Errors.ERROR_ENVVAR_NOT_FOUND) ||
-                    requiredSize > buffer.Length)
-                {
-                    return null;
-                }
-
-                return new string(buffer.Slice(0, requiredSize));
-            }
-            finally
-            {
-                ArrayPool<char>.Shared.Return(chars);
-            }
+            builder.Length = (int)length;
+            return builder.ToString();
         }
 
         private static void SetEnvironmentVariableCore(string variable, string? value)