From e9a8cb892756ae0f3790d62e7482975200c34868 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 13 Jul 2021 21:59:25 +0200 Subject: [PATCH 1/5] Add IDisposable interface to GCHandle --- .../Runtime/InteropServices/GCHandle.cs | 32 +++++++++++++--- .../Runtime/InteropServices/GCHandleTests.cs | 37 +++++++++++++++++++ .../System.Runtime/ref/System.Runtime.cs | 3 +- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs index 579633db55cafc..a02e1afe086efb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs @@ -4,7 +4,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; -using System.Threading; using Internal.Runtime.CompilerServices; namespace System.Runtime.InteropServices @@ -23,7 +22,7 @@ namespace System.Runtime.InteropServices /// Pinned - same as Normal, but allows the address of the actual object to be taken. /// [StructLayout(LayoutKind.Sequential)] - public partial struct GCHandle + public partial struct GCHandle : IDisposable { // The actual integer handle value that the EE uses internally. private IntPtr _handle; @@ -67,13 +66,34 @@ private GCHandle(object? value, GCHandleType type) /// A new GC handle that protects the object. public static GCHandle Alloc(object? value, GCHandleType type) => new GCHandle(value, type); + /// + public void Dispose() + { + IntPtr handle = _handle; + + if ((nint)handle != 0) + { + InternalFree(GetHandleValue(handle)); + + _handle = IntPtr.Zero; + } + } + /// Frees a GC handle. public void Free() { - // Free the handle if it hasn't already been freed. - IntPtr handle = Interlocked.Exchange(ref _handle, IntPtr.Zero); - ThrowIfInvalid(handle); - InternalFree(GetHandleValue(handle)); + IntPtr handle = _handle; + + if ((nint)handle != 0) + { + InternalFree(GetHandleValue(handle)); + + _handle = IntPtr.Zero; + } + else + { + ThrowHelper.ThrowInvalidOperationException_HandleIsNotInitialized(); + } } // Target property - allows getting / updating of the handle's referent. diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs index bf5ec1713cdbc6..87d05f5985e6eb 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs @@ -119,6 +119,43 @@ public void Free_NotInitialized_ThrowsInvalidOperationException() Assert.Throws(() => handle.Free()); } + [Fact] + public void Dispose_NotInitialized_DoesntThrow() + { + var handle = new GCHandle(); + handle.Dispose(); + } + + [Fact] + public void Dispose_WorksAsExpected() + { + object target = new object(); + GCHandle handle = GCHandle.Alloc(target); + + try + { + Assert.Equal(target, handle.Target); + Assert.True(handle.IsAllocated); + + handle.Dispose(); + + Assert.False(handle.IsAllocated); + + // Repeated Dispose() calls are just a no-op + handle.Dispose(); + handle.Dispose(); + handle.Dispose(); + handle.Dispose(); + + Assert.False(handle.IsAllocated); + } + finally + { + handle.Dispose(); + Assert.False(handle.IsAllocated); + } + } + public static IEnumerable Equals_TestData() { GCHandle handle = GCHandle.Alloc(new object()); diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index 9e1a56a42a65ad..ade42f0f0220ea 100644 --- a/src/libraries/System.Runtime/ref/System.Runtime.cs +++ b/src/libraries/System.Runtime/ref/System.Runtime.cs @@ -13284,7 +13284,7 @@ public sealed partial class FieldOffsetAttribute : System.Attribute public FieldOffsetAttribute(int offset) { } public int Value { get { throw null; } } } - public partial struct GCHandle + public partial struct GCHandle : System.IDisposable { private int _dummyPrimitive; public bool IsAllocated { get { throw null; } } @@ -13292,6 +13292,7 @@ public partial struct GCHandle public System.IntPtr AddrOfPinnedObject() { throw null; } public static System.Runtime.InteropServices.GCHandle Alloc(object? value) { throw null; } public static System.Runtime.InteropServices.GCHandle Alloc(object? value, System.Runtime.InteropServices.GCHandleType type) { throw null; } + public void Dispose() { } public override bool Equals([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] object? o) { throw null; } public void Free() { } public static System.Runtime.InteropServices.GCHandle FromIntPtr(System.IntPtr value) { throw null; } From db295c4a8d4bd7c4d8f34e7848c3284da4ee6fda Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 13 Jul 2021 22:06:15 +0200 Subject: [PATCH 2/5] Zero internal GC handle field before disposal --- .../src/System/Runtime/InteropServices/GCHandle.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs index a02e1afe086efb..69385533d2f3d5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs @@ -73,9 +73,9 @@ public void Dispose() if ((nint)handle != 0) { - InternalFree(GetHandleValue(handle)); - _handle = IntPtr.Zero; + + InternalFree(GetHandleValue(handle)); } } @@ -86,9 +86,9 @@ public void Free() if ((nint)handle != 0) { - InternalFree(GetHandleValue(handle)); - _handle = IntPtr.Zero; + + InternalFree(GetHandleValue(handle)); } else { From bb5f30bc5d25d5c2db6362ce7cdcbfd2b4ee0953 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 13 Jul 2021 22:22:38 +0200 Subject: [PATCH 3/5] Apply code style feedbacks to GCHandle.Free() --- .../src/System/Runtime/InteropServices/GCHandle.cs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs index 69385533d2f3d5..1af61ae2dcf0fa 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.cs @@ -83,17 +83,9 @@ public void Dispose() public void Free() { IntPtr handle = _handle; - - if ((nint)handle != 0) - { - _handle = IntPtr.Zero; - - InternalFree(GetHandleValue(handle)); - } - else - { - ThrowHelper.ThrowInvalidOperationException_HandleIsNotInitialized(); - } + ThrowIfInvalid(handle); + _handle = IntPtr.Zero; + InternalFree(GetHandleValue(handle)); } // Target property - allows getting / updating of the handle's referent. From 5e6525e7a1c201280568fa0861ae36f1ce2c8471 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 13 Jul 2021 22:52:39 +0200 Subject: [PATCH 4/5] Migrate applicable patterns to GCHandle.Dispose() --- .../CrossLoaderAllocatorHashHelpers.cs | 3 +-- .../Interop.OpenSsl.cs | 5 +---- .../Interop.Ssl.cs | 5 +---- .../System/Net/Windows/HttpListener.Windows.cs | 15 +++------------ .../Net/Windows/HttpListenerResponse.Windows.cs | 5 +---- .../Net/Windows/WebSockets/WebSocketBuffer.cs | 10 ++-------- .../Net/Sockets/SocketAsyncEventArgs.Windows.cs | 5 +---- .../src/System/Net/Sockets/SocketPal.Windows.cs | 5 +---- .../src/System/Buffers/MemoryHandle.cs | 5 +---- .../src/System/Threading/Overlapped.cs | 8 ++------ 10 files changed, 14 insertions(+), 52 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs index 9bb4628a3be64f..e2fd4b5d6bb5aa 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/CrossLoaderAllocatorHashHelpers.cs @@ -17,8 +17,7 @@ internal sealed class LAHashDependentHashTracker ~LAHashDependentHashTracker() { - if (_dependentHandle.IsAllocated) - _dependentHandle.Free(); + _dependentHandle.Dispose(); } } diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 9dd310326b0ad3..fa9fd638e1ad34 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -212,10 +212,7 @@ internal static SafeSslHandle AllocateSslContext(SslProtocols protocols, SafeX50 } catch { - if (alpnHandle.IsAllocated) - { - alpnHandle.Free(); - } + alpnHandle.Dispose(); throw; } diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs index 29154b77da6325..4669b0d85efd84 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs @@ -326,10 +326,7 @@ protected override void Dispose(bool disposing) _writeBio?.Dispose(); } - if (AlpnHandle.IsAllocated) - { - AlpnHandle.Free(); - } + AlpnHandle.Dispose(); base.Dispose(disposing); } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs index 63c96497909f3a..2c186b9a5da197 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListener.Windows.cs @@ -1664,22 +1664,13 @@ private static void SendError(HttpListenerSession session, ulong requestId, Http } finally { - if (headersArrayHandle.IsAllocated) - { - headersArrayHandle.Free(); - } - if (wwwAuthenticateHandle.IsAllocated) - { - wwwAuthenticateHandle.Free(); - } + headersArrayHandle.Dispose(); + wwwAuthenticateHandle.Dispose(); if (challengeHandles != null) { for (int i = 0; i < challengeHandles.Length; i++) { - if (challengeHandles[i].IsAllocated) - { - challengeHandles[i].Free(); - } + challengeHandles[i].Dispose(); } } } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs index 0c4b36af8efc75..a5553e6fc20977 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/HttpListenerResponse.Windows.cs @@ -584,10 +584,7 @@ private void FreePinnedHeaders(List? pinnedHeaders) { foreach (GCHandle gcHandle in pinnedHeaders) { - if (gcHandle.IsAllocated) - { - gcHandle.Free(); - } + gcHandle.Dispose(); } } } diff --git a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs index 712b0d7f39d665..95b696112848d0 100644 --- a/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs +++ b/src/libraries/System.Net.HttpListener/src/System/Net/Windows/WebSockets/WebSocketBuffer.cs @@ -281,10 +281,7 @@ internal void ReleasePinnedSendBuffer() return; } - if (_pinnedSendBufferHandle.IsAllocated) - { - _pinnedSendBufferHandle.Free(); - } + _pinnedSendBufferHandle.Dispose(); _pinnedSendBuffer = ArraySegment.Empty; } @@ -620,10 +617,7 @@ private bool IsNativeBuffer(IntPtr pBuffer, uint bufferSize) private void CleanUp() { - if (_gcHandle.IsAllocated) - { - _gcHandle.Free(); - } + _gcHandle.Dispose(); ReleasePinnedSendBuffer(); } diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs index b22efcee8d9047..1814b82120b2cc 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs @@ -896,10 +896,7 @@ private void PinSocketAddressBuffer() } // Unpin any existing. - if (_socketAddressGCHandle.IsAllocated) - { - _socketAddressGCHandle.Free(); - } + _socketAddressGCHandle.Dispose(); // Pin down the new one. _socketAddressGCHandle = GCHandle.Alloc(_socketAddress!.Buffer, GCHandleType.Pinned); diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs index e157c687fd4a1b..08279514c8fd25 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs @@ -256,10 +256,7 @@ public static SocketError Send(SafeSocketHandle handle, IList { for (int i = 0; i < count; ++i) { - if (objectsToPin[i].IsAllocated) - { - objectsToPin[i].Free(); - } + objectsToPin[i].Dispose(); } if (!useStack) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Buffers/MemoryHandle.cs b/src/libraries/System.Private.CoreLib/src/System/Buffers/MemoryHandle.cs index 2ac65aa35a1264..f510529cb6d84b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Buffers/MemoryHandle.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Buffers/MemoryHandle.cs @@ -39,10 +39,7 @@ public MemoryHandle(void* pointer, GCHandle handle = default, IPinnable? pinnabl /// public void Dispose() { - if (_handle.IsAllocated) - { - _handle.Free(); - } + _handle.Dispose(); if (_pinnable != null) { diff --git a/src/mono/System.Private.CoreLib/src/System/Threading/Overlapped.cs b/src/mono/System.Private.CoreLib/src/System/Threading/Overlapped.cs index 117e75cef56f63..a46d72d01abfac 100644 --- a/src/mono/System.Private.CoreLib/src/System/Threading/Overlapped.cs +++ b/src/mono/System.Private.CoreLib/src/System/Threading/Overlapped.cs @@ -175,10 +175,7 @@ private void FreeNativeOverlapped() { for (int i = 0; i < _pinnedData.Length; i++) { - if (_pinnedData[i].IsAllocated) - { - _pinnedData[i].Free(); - } + _pinnedData[i].Dispose(); } _pinnedData = null; } @@ -186,8 +183,7 @@ private void FreeNativeOverlapped() if (_pNativeOverlapped != null) { GCHandle handle = *(GCHandle*)(_pNativeOverlapped + 1); - if (handle.IsAllocated) - handle.Free(); + handle.Dispose(); Marshal.FreeHGlobal((IntPtr)_pNativeOverlapped); //CORERT: Interop.MemFree((IntPtr)_pNativeOverlapped); From cf6976d948ced2819523f91c04e415135e58b88b Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 16 Jul 2021 09:50:21 -0400 Subject: [PATCH 5/5] Fix GCHandle tests to not double-dispose --- .../Runtime/InteropServices/GCHandleTests.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs index 87d05f5985e6eb..a2a559c458c443 100644 --- a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs +++ b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/GCHandleTests.cs @@ -158,18 +158,21 @@ public void Dispose_WorksAsExpected() public static IEnumerable Equals_TestData() { - GCHandle handle = GCHandle.Alloc(new object()); - yield return new object[] { handle, handle, true }; - yield return new object[] { GCHandle.Alloc(new object()), GCHandle.Alloc(new object()), false }; + // xunit disposes of any IDisposables passed as row values in a member data. + // Avoid GCHandle being disposed of automatically by wrapping it. - yield return new object[] { GCHandle.Alloc(new object()), new object(), false }; - yield return new object[] { GCHandle.Alloc(new object()), null, false }; + GCHandle handle = GCHandle.Alloc(new object()); + yield return new object[] { new KeyValuePair(handle, handle), true }; + yield return new object[] { new KeyValuePair(GCHandle.Alloc(new object()), GCHandle.Alloc(new object())), false }; + yield return new object[] { new KeyValuePair(GCHandle.Alloc(new object()), new object()), false }; + yield return new object[] { new KeyValuePair(GCHandle.Alloc(new object()), null), false }; } [Theory] [MemberData(nameof(Equals_TestData))] - public void Equals_Object_ReturnsExpected(GCHandle handle, object other, bool expected) + public void Equals_Object_ReturnsExpected(KeyValuePair pair, bool expected) { + (GCHandle handle, object other) = pair; try { Assert.Equal(expected, handle.Equals(other));