fix pooled array leak (#88810)
authorDan Moseley <danmose@microsoft.com>
Tue, 18 Jul 2023 02:52:06 +0000 (21:52 -0500)
committerGitHub <noreply@github.com>
Tue, 18 Jul 2023 02:52:06 +0000 (22:52 -0400)
* ntlm

* test base

* qpack

* FileSys

* JSON

* interp tests

* fix NTAuthentication leak

* More in JSON

* more tests

* ntlmserver disposable

* more tests

* more tests

* tar tests

* feedback

17 files changed:
src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs
src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs
src/libraries/Common/tests/Tests/System/Net/MultiArrayBufferTests.cs
src/libraries/Common/tests/Tests/System/Net/aspnetcore/Http3/QPackDecoderTest.cs
src/libraries/Common/tests/Tests/System/Text/ValueStringBuilderTests.cs
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectory.File.Tests.cs
src/libraries/System.Formats.Tar/tests/TarFile/TarFile.ExtractToDirectoryAsync.File.Tests.cs
src/libraries/System.Memory/tests/MemoryPool/MemoryPool.cs
src/libraries/System.Net.Http.Json/tests/UnitTests/TranscodingWriteStreamTests.cs
src/libraries/System.Net.Http/tests/FunctionalTests/NtAuthTests.FakeServer.cs
src/libraries/System.Net.Mail/tests/Functional/LoopbackSmtpServer.cs
src/libraries/System.Net.Security/src/System/Net/NTAuthentication.Managed.cs
src/libraries/System.Net.Security/tests/UnitTests/NegotiateAuthenticationTests.cs
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemName.cs
src/libraries/System.Runtime/tests/System/Runtime/CompilerServices/DefaultInterpolatedStringHandlerTests.cs
src/libraries/System.Security.Cryptography/tests/ZeroMemoryTests.cs
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/RealWorldContextTests.cs

index 2561be4..cb7a3a7 100644 (file)
@@ -24,7 +24,7 @@ namespace System.Net.Security
     // and responses for unit test purposes. The validation checks the
     // structure of the messages, their integrity and use of specified
     // features (eg. MIC).
-    internal class FakeNtlmServer
+    internal class FakeNtlmServer : IDisposable
     {
         public FakeNtlmServer(NetworkCredential expectedCredential)
         {
@@ -142,6 +142,14 @@ namespace System.Net.Security
             UntrustedSPN = 4,
         }
 
+        public void Dispose()
+        {
+            _clientSeal?.Dispose();
+            _clientSeal = null;
+            _serverSeal?.Dispose();
+            _serverSeal = null;
+        }
+
         private static ReadOnlySpan<byte> GetField(ReadOnlySpan<byte> payload, int fieldOffset)
         {
             uint offset = BinaryPrimitives.ReadUInt32LittleEndian(payload.Slice(fieldOffset + 4));
@@ -396,6 +404,9 @@ namespace System.Net.Security
 
         public void ResetKeys()
         {
+            _clientSeal?.Dispose();
+            _serverSeal?.Dispose();
+
             _clientSeal = new RC4(_clientSealingKey);
             _serverSeal = new RC4(_serverSealingKey);
         }
index 0629483..a40bbce 100644 (file)
@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Buffers;
+using System.ComponentModel;
 using System.Diagnostics;
 using System.Runtime.CompilerServices;
 using System.Text;
@@ -201,17 +202,24 @@ namespace System.IO
 
                 if (!handle.IsInvalid)
                 {
-                    const int InitialBufferSize = 4096;
-                    char[]? buffer = ArrayPool<char>.Shared.Rent(InitialBufferSize);
-                    uint result = GetFinalPathNameByHandle(handle, buffer);
+                    char[] buffer = new char[4096];
+                    uint result;
+                    fixed (char* bufPtr = buffer)
+                    {
+                        result = Interop.Kernel32.GetFinalPathNameByHandle(handle, bufPtr, (uint)buffer.Length, Interop.Kernel32.FILE_NAME_NORMALIZED);
+                    }
+
+                    if (result == 0)
+                    {
+                        throw new Win32Exception();
+                    }
+
+                    Debug.Assert(result <= buffer.Length);
 
                     // Remove extended prefix
                     int skip = PathInternal.IsExtended(buffer) ? 4 : 0;
 
-                    return new string(
-                        buffer,
-                        skip,
-                        (int)result - skip);
+                    return new string(buffer, skip, (int)result - skip);
                 }
             }
             catch { }
@@ -219,14 +227,6 @@ namespace System.IO
             return TestDirectory;
         }
 
-        private unsafe uint GetFinalPathNameByHandle(SafeFileHandle handle, char[] buffer)
-        {
-            fixed (char* bufPtr = buffer)
-            {
-                return Interop.Kernel32.GetFinalPathNameByHandle(handle, bufPtr, (uint)buffer.Length, Interop.Kernel32.FILE_NAME_NORMALIZED);
-            }
-        }
-
         protected string CreateTestDirectory(params string[] paths)
         {
             string dir = Path.Combine(paths);
index 1459e9c..3ea1ae4 100644 (file)
@@ -16,7 +16,7 @@ namespace Tests.System.Net
         [Fact]
         public void BasicTest()
         {
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             Assert.True(buffer.IsEmpty);
             Assert.True(buffer.ActiveMemory.IsEmpty);
@@ -98,7 +98,7 @@ namespace Tests.System.Net
         {
             const int Size = 64 * 1024 + 1;
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < Size; i++)
             {
@@ -124,7 +124,7 @@ namespace Tests.System.Net
             const int ByteCount = 7;
             const int RepeatCount = 8 * 1024;       // enough to ensure we cross several block boundaries
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < RepeatCount; i++)
             {
@@ -156,7 +156,7 @@ namespace Tests.System.Net
             const int ByteCount = 7;
             const int RepeatCount = 8 * 1024;       // enough to ensure we cross several block boundaries
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < RepeatCount; i++)
             {
@@ -188,7 +188,7 @@ namespace Tests.System.Net
             const int ByteCount = 7;
             const int RepeatCount = 8 * 1024;       // enough to ensure we cross several block boundaries
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < RepeatCount; i++)
             {
@@ -221,7 +221,7 @@ namespace Tests.System.Net
 
             const int RepeatCount = 8 * 1024;       // enough to ensure we cross several block boundaries
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < RepeatCount; i++)
             {
@@ -250,7 +250,7 @@ namespace Tests.System.Net
 
             const int RepeatCount = 13;
 
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             for (int i = 0; i < RepeatCount; i++)
             {
@@ -291,7 +291,7 @@ namespace Tests.System.Net
         [Fact]
         public void EnsureAvailableSpaceTest()
         {
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             Assert.Equal(0, buffer.ActiveMemory.Length);
             Assert.Equal(0, buffer.AvailableMemory.Length);
@@ -423,7 +423,7 @@ namespace Tests.System.Net
         [Fact]
         public void EnsureAvailableSpaceUpToLimitTest()
         {
-            MultiArrayBuffer buffer = new MultiArrayBuffer(0);
+            using MultiArrayBuffer buffer = new MultiArrayBuffer(0);
 
             Assert.Equal(0, buffer.ActiveMemory.Length);
             Assert.Equal(0, buffer.AvailableMemory.Length);
index 931ecbc..643bfdd 100644 (file)
@@ -15,7 +15,7 @@ using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
 
 namespace System.Net.Http.Unit.Tests.QPack
 {
-    public class QPackDecoderTests
+    public class QPackDecoderTests : IDisposable
     {
         private const int MaxHeaderFieldSize = 8192;
 
@@ -64,6 +64,11 @@ namespace System.Net.Http.Unit.Tests.QPack
             _decoder = new QPackDecoder(MaxHeaderFieldSize);
         }
 
+        public void Dispose()
+        {
+            _decoder.Dispose();
+        }
+
         [Fact]
         public void DecodesIndexedHeaderField_StaticTableWithValue()
         {
@@ -318,7 +323,7 @@ namespace System.Net.Http.Unit.Tests.QPack
 
         private static void TestDecode(byte[] encoded, KeyValuePair<string, string>[] expectedValues, bool expectDynamicTableEntry, int? bytesAtATime)
         {
-            QPackDecoder decoder = new QPackDecoder(MaxHeaderFieldSize);
+            using QPackDecoder decoder = new QPackDecoder(MaxHeaderFieldSize);
             TestHttpHeadersHandler handler = new TestHttpHeadersHandler();
 
             // Read past header
index 20d65e5..728749a 100644 (file)
@@ -180,6 +180,7 @@ namespace System.Text.Tests
 
             Assert.NotEqual(0, sb.Length);
             Assert.Equal(sb.Length, vsb.Length);
+            Assert.Equal(sb.ToString(), vsb.ToString());
         }
 
         [Fact]
@@ -275,6 +276,7 @@ namespace System.Text.Tests
             Assert.Equal('b', vsb[3]);
             vsb[3] = 'c';
             Assert.Equal('c', vsb[3]);
+            vsb.Dispose();
         }
 
         [Fact]
@@ -297,6 +299,7 @@ namespace System.Text.Tests
             builder.EnsureCapacity(33);
 
             Assert.Equal(64, builder.Capacity);
+            builder.Dispose();
         }
 
         [Fact]
@@ -309,6 +312,7 @@ namespace System.Text.Tests
             builder.EnsureCapacity(16);
 
             Assert.Equal(64, builder.Capacity);
+            builder.Dispose();
         }
     }
 }
index 53eb65d..9ffbcc0 100644 (file)
@@ -340,7 +340,7 @@ namespace System.Formats.Tar.Tests
             AssertFileModeEquals(filePath, TestPermission1);
         }
 
-        [Fact]
+        [ConditionalFact(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))]
         public void LinkBeforeTarget()
         {
             using TempDirectory source = new TempDirectory();
index 62c3f74..42b0eef 100644 (file)
@@ -362,7 +362,7 @@ namespace System.Formats.Tar.Tests
             AssertFileModeEquals(filePath, TestPermission1);
         }
 
-        [Fact]
+        [ConditionalFact(typeof(MountHelper), nameof(MountHelper.CanCreateSymbolicLinks))]
         public async Task LinkBeforeTargetAsync()
         {
             using TempDirectory source = new TempDirectory();
index 25c2822..e040a36 100644 (file)
@@ -90,7 +90,7 @@ namespace System.MemoryTests
         public static void MemoryPoolPinBadOffset(int elementIndex)
         {
             MemoryPool<int> pool = MemoryPool<int>.Shared;
-            IMemoryOwner<int> block = pool.Rent(10);
+            using IMemoryOwner<int> block = pool.Rent(10);
             Memory<int> memory = block.Memory;
             Span<int> sp = memory.Span;
             Assert.Equal(memory.Length, sp.Length);
@@ -101,7 +101,7 @@ namespace System.MemoryTests
         public static void MemoryPoolPinOffsetAtEnd()
         {
             MemoryPool<int> pool = MemoryPool<int>.Shared;
-            IMemoryOwner<int> block = pool.Rent(10);
+            using IMemoryOwner<int> block = pool.Rent(10);
             Memory<int> memory = block.Memory;
             Span<int> sp = memory.Span;
             Assert.Equal(memory.Length, sp.Length);
@@ -122,7 +122,7 @@ namespace System.MemoryTests
         public static void MemoryPoolPinBadOffsetTooLarge()
         {
             MemoryPool<int> pool = MemoryPool<int>.Shared;
-            IMemoryOwner<int> block = pool.Rent(10);
+            using IMemoryOwner<int> block = pool.Rent(10);
             Memory<int> memory = block.Memory;
             Span<int> sp = memory.Span;
             Assert.Equal(memory.Length, sp.Length);
index f9e7126..b5b32f6 100644 (file)
@@ -67,7 +67,7 @@ namespace System.Net.Http.Json.Functional.Tests
             var model = new TestModel { Message = message };
             var stream = new MemoryStream();
 
-            var transcodingStream = new TranscodingWriteStream(stream, targetEncoding);
+            using var transcodingStream = new TranscodingWriteStream(stream, targetEncoding);
             await JsonSerializer.SerializeAsync(transcodingStream, model, model.GetType());
             // The transcoding streams use Encoders and Decoders that have internal buffers. We need to flush these
             // when there is no more data to be written. Stream.FlushAsync isn't suitable since it's
index 881555d..5ec6a53 100644 (file)
@@ -107,6 +107,8 @@ namespace System.Net.Http.Functional.Tests
             }
             while (!isAuthenticated);
 
+            fakeNtlmServer?.Dispose();
+
             await connection.SendResponseAsync(HttpStatusCode.OK);
         }
 
index 72d26da..6b5d4ab 100644 (file)
@@ -156,7 +156,7 @@ namespace Systen.Net.Mail.Tests
                         else if (parts[1].Equals("GSSAPI", StringComparison.OrdinalIgnoreCase))
                         {
                             Debug.Assert(ExpectedGssapiCredential != null);
-                            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(ExpectedGssapiCredential) { ForceNegotiateVersion = true };
+                            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(ExpectedGssapiCredential) { ForceNegotiateVersion = true };
                             FakeNegotiateServer fakeNegotiateServer = new FakeNegotiateServer(fakeNtlmServer);
 
                             try
index 4ba654d..50bc657 100644 (file)
@@ -773,6 +773,10 @@ namespace System.Net
 
         private void ResetKeys()
         {
+            // Release buffers to pool
+            _clientSeal?.Dispose();
+            _serverSeal?.Dispose();
+
             _clientSeal = new RC4(_clientSealingKey);
             _serverSeal = new RC4(_serverSealingKey);
         }
index eb2b3c4..ee909c5 100644 (file)
@@ -41,7 +41,7 @@ namespace System.Net.Security.Tests
         [ConditionalFact(nameof(IsNtlmAvailable))]
         public void RemoteIdentity_ThrowsOnDisposed()
         {
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
             NegotiateAuthentication negotiateAuthentication = new NegotiateAuthentication(
                 new NegotiateAuthenticationClientOptions
                 {
@@ -98,7 +98,7 @@ namespace System.Net.Security.Tests
         {
             // Mirrors the NTLMv2 example in the NTLM specification:
             NetworkCredential credential = new NetworkCredential("User", "Password", "Domain");
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(credential);
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(credential);
             fakeNtlmServer.SendTimestamp = false;
             fakeNtlmServer.TargetIsServer = true;
             fakeNtlmServer.PreferUnicode = false;
@@ -151,7 +151,7 @@ namespace System.Net.Security.Tests
         [ConditionalFact(nameof(IsNtlmAvailable))]
         public void NtlmCorrectExchangeTest()
         {
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
             NegotiateAuthentication ntAuth = new NegotiateAuthentication(
                 new NegotiateAuthenticationClientOptions
                 {
@@ -175,7 +175,7 @@ namespace System.Net.Security.Tests
         [ConditionalFact(nameof(IsNtlmAvailable))]
         public void NtlmIncorrectExchangeTest()
         {
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
             NegotiateAuthentication ntAuth = new NegotiateAuthentication(
                 new NegotiateAuthenticationClientOptions
                 {
@@ -194,7 +194,7 @@ namespace System.Net.Security.Tests
         [ActiveIssue("https://github.com/dotnet/runtime/issues/65678", TestPlatforms.OSX | TestPlatforms.iOS | TestPlatforms.MacCatalyst)]
         public void NtlmSignatureTest()
         {
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight);
             NegotiateAuthentication ntAuth = new NegotiateAuthentication(
                 new NegotiateAuthenticationClientOptions
                 {
@@ -250,7 +250,7 @@ namespace System.Net.Security.Tests
         public void NegotiateCorrectExchangeTest(bool requestMIC, bool requestConfidentiality)
         {
             // Older versions of gss-ntlmssp on Linux generate MIC at incorrect offset unless ForceNegotiateVersion is specified
-            FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight) { ForceNegotiateVersion = true };
+            using FakeNtlmServer fakeNtlmServer = new FakeNtlmServer(s_testCredentialRight) { ForceNegotiateVersion = true };
             FakeNegotiateServer fakeNegotiateServer = new FakeNegotiateServer(fakeNtlmServer) { RequestMIC = requestMIC };
             NegotiateAuthentication ntAuth = new NegotiateAuthentication(
                 new NegotiateAuthenticationClientOptions
index fa95554..0de76f2 100644 (file)
@@ -51,7 +51,13 @@ namespace System.IO.Enumeration
                 }
             }
 
-            return modified ? sb.ToString() : expression;
+            if (!modified)
+            {
+                sb.Dispose();
+                return expression;
+            }
+
+            return sb.ToString();
         }
 
         /// <summary>Verifies whether the given Win32 expression matches the given name. Supports the following wildcards: '*', '?', '&lt;', '&gt;', '"'. The backslash character '\' escapes.</summary>
index 11740f8..80f30c2 100644 (file)
@@ -18,17 +18,17 @@ namespace System.Runtime.CompilerServices.Tests
         [InlineData(-16, 1)]
         public void LengthAndHoleArguments_Valid(int literalLength, int formattedCount)
         {
-            new DefaultInterpolatedStringHandler(literalLength, formattedCount);
+            new DefaultInterpolatedStringHandler(literalLength, formattedCount).ToStringAndClear();
 
             Span<char> scratch1 = stackalloc char[1];
             foreach (IFormatProvider provider in new IFormatProvider[] { null, new ConcatFormatter(), CultureInfo.InvariantCulture, CultureInfo.CurrentCulture, new CultureInfo("en-US"), new CultureInfo("fr-FR") })
             {
-                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider);
+                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider).ToStringAndClear();
 
-                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, default);
-                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, scratch1);
-                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, Array.Empty<char>());
-                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, new char[256]);
+                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, default).ToStringAndClear();
+                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, scratch1).ToStringAndClear();
+                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, Array.Empty<char>()).ToStringAndClear();
+                new DefaultInterpolatedStringHandler(literalLength, formattedCount, provider, new char[256]).ToStringAndClear();
             }
         }
 
@@ -244,6 +244,8 @@ namespace System.Runtime.CompilerServices.Tests
                 handler.AppendFormatted(tss, 1, "X2");
                 Assert.Same(provider, tss.ToStringState.LastProvider);
             }
+
+            handler.ToStringAndClear();
         }
 
         [Fact]
@@ -357,6 +359,8 @@ namespace System.Runtime.CompilerServices.Tests
 
                 handler.AppendFormatted(t, 1, "X2");
                 Assert.Same(provider, ((IHasToStringState)t).ToStringState.LastProvider);
+
+                handler.ToStringAndClear();
             }
 
             Test(new FormattableInt32Wrapper(42));
index 5055743..5bc0a0e 100644 (file)
@@ -57,6 +57,8 @@ namespace System.Security.Cryptography.Tests
             {
                 Assert.Equal(0, testSpan[i]);
             }
+
+            ArrayPool<byte>.Shared.Return(rented, clearArray: false);
         }
 
         [Fact]
index 420df16..948dcfe 100644 (file)
@@ -118,12 +118,12 @@ namespace System.Text.Json.SourceGeneration.Tests
         [InlineData("{ \"key\" : \"value\" }")]
         public void RoundtripJsonDocument(string json)
         {
-            JsonDocument jsonDocument = JsonDocument.Parse(json);
+            using JsonDocument jsonDocument = JsonDocument.Parse(json);
 
             string actualJson = JsonSerializer.Serialize(jsonDocument, DefaultContext.JsonDocument);
             JsonTestHelper.AssertJsonEqual(json, actualJson);
 
-            JsonDocument actualJsonDocument = JsonSerializer.Deserialize(actualJson, DefaultContext.JsonDocument);
+            using JsonDocument actualJsonDocument = JsonSerializer.Deserialize(actualJson, DefaultContext.JsonDocument);
             JsonTestHelper.AssertJsonEqual(jsonDocument.RootElement, actualJsonDocument.RootElement);
         }
 
@@ -135,7 +135,8 @@ namespace System.Text.Json.SourceGeneration.Tests
         [InlineData("{ \"key\" : \"value\" }")]
         public void RoundtripJsonElement(string json)
         {
-            JsonElement jsonElement = JsonDocument.Parse(json).RootElement;
+            using JsonDocument jsonDocument = JsonDocument.Parse(json);
+            JsonElement jsonElement = jsonDocument.RootElement;
 
             string actualJson = JsonSerializer.Serialize(jsonElement, DefaultContext.JsonElement);
             JsonTestHelper.AssertJsonEqual(json, actualJson);