From 71937bc8f9889f878d0906a992b1ea8ccd3fa996 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ahmet=20=C4=B0brahim=20Aksoy?= Date: Mon, 18 May 2026 15:42:19 +0300 Subject: [PATCH 1/5] Fix GCHandle leak in OSX SafeDeleteSslContext The GCHandle allocated by SslSetConnection so native Read/Write callbacks can resolve back to the owning SafeDeleteSslContext was never freed, leaking one GCHandle table slot per SSL context created on macOS (legacy SecureTransport path). Move ownership of the GCHandle to SafeSslHandle and free it from its ReleaseHandle override. ReleaseHandle is only invoked after the SafeHandle ref count reaches zero - i.e. once all outstanding P/Invokes (and therefore any in-flight native Read/Write callbacks) have completed - so this is the earliest point at which it is safe to drop the backreference. Freeing the handle in SafeDeleteSslContext.Dispose directly races with callbacks that are still in flight on another thread. Fixes #128136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop.Ssl.cs | 18 ++++++++++++++++++ .../Security/Pal.OSX/SafeDeleteSslContext.cs | 6 ++++++ 2 files changed, 24 insertions(+) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs index 4d6fb38093c4c2..4879e7e756c9e0 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs @@ -460,6 +460,13 @@ namespace System.Net { internal sealed class SafeSslHandle : SafeHandle { + // Backreference used by AppleCryptoNative_SslSetConnection so native + // Read/Write callbacks can resolve the owning SafeDeleteSslContext. + // Owned here so the lifetime is tied to ReleaseHandle, which only + // runs once all outstanding P/Invokes (and therefore any in-flight + // callbacks) have completed. + private GCHandle _connectionGCHandle; + public SafeSslHandle() : base(IntPtr.Zero, ownsHandle: true) { @@ -470,10 +477,21 @@ internal SafeSslHandle(IntPtr invalidHandleValue, bool ownsHandle) { } + internal void SetConnectionGCHandle(GCHandle handle) + { + Debug.Assert(!_connectionGCHandle.IsAllocated, "Connection GCHandle already set"); + _connectionGCHandle = handle; + } + protected override bool ReleaseHandle() { Interop.CoreFoundation.CFRelease(handle); SetHandle(IntPtr.Zero); + if (_connectionGCHandle.IsAllocated) + { + _connectionGCHandle.Free(); + } + return true; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index c94808ea519827..a6475d1a069406 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -191,7 +191,13 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions sslAuthen private void SslSetConnection(SafeSslHandle sslContext) { + // The GCHandle is owned by the SafeSslHandle and freed in its + // ReleaseHandle override, which only runs after all outstanding + // P/Invokes (and therefore any in-flight Read/Write callbacks) + // have completed. Freeing it here in Dispose would race with + // callbacks that are still in flight on another thread. GCHandle handle = GCHandle.Alloc(this, GCHandleType.Weak); + sslContext.SetConnectionGCHandle(handle); Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); } From 25c4910850a733caa192bce1c3497900ecf4f51a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ahmet=20=C4=B0brahim=20Aksoy?= Date: Thu, 21 May 2026 17:44:16 +0300 Subject: [PATCH 2/5] Use Normal GCHandle and harden Read/Write callbacks Address review feedback on the GCHandle leak fix: * Promote the connection GCHandle from Weak to Normal. The handle's lifetime is already bounded by SafeSslHandle.ReleaseHandle (no risk of a runaway leak), and Normal correctly expresses that the managed SafeDeleteSslContext must stay rooted while the native SSLContext can call back into us. This mirrors the existing SafeDeleteNwContext pattern and removes the contradiction between Weak's nullable Target contract and the assumptions of the Read/Write callbacks. * Replace Debug.Assert(context != null) in WriteToConnection / ReadFromConnection with explicit null checks that return ReadErr / WritErr. The branch should be unreachable in practice, but the cheap guard documents intent and makes the callbacks robust to future lifetime changes instead of dereferencing null in release builds. Tested on osx-arm64: System.Net.Security unit tests (124) and functional tests (4964) all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index a6475d1a069406..9009b39fcef143 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -192,11 +192,12 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions sslAuthen private void SslSetConnection(SafeSslHandle sslContext) { // The GCHandle is owned by the SafeSslHandle and freed in its - // ReleaseHandle override, which only runs after all outstanding - // P/Invokes (and therefore any in-flight Read/Write callbacks) - // have completed. Freeing it here in Dispose would race with - // callbacks that are still in flight on another thread. - GCHandle handle = GCHandle.Alloc(this, GCHandleType.Weak); + // ReleaseHandle override, which only runs once the ref count drops + // to zero (i.e. after all outstanding P/Invokes and any in-flight + // native Read/Write callbacks have completed). A strong (Normal) + // handle is used so that this SafeDeleteSslContext stays rooted + // for as long as the native SSLContext can call back into us. + GCHandle handle = GCHandle.Alloc(this, GCHandleType.Normal); sslContext.SetConnectionGCHandle(handle); Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); @@ -227,7 +228,12 @@ protected override void Dispose(bool disposing) private static unsafe int WriteToConnection(IntPtr connection, byte* data, void** dataLength) { SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; - Debug.Assert(context != null); + if (context is null) + { + // The managed context has already been collected. Fail the + // I/O cleanly so SecureTransport tears down the handshake. + return OSStatus.WritErr; + } // We don't pool these buffers and we can't because there's a race between their us in the native // read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently, @@ -262,7 +268,12 @@ private static unsafe int WriteToConnection(IntPtr connection, byte* data, void* private static unsafe int ReadFromConnection(IntPtr connection, byte* data, void** dataLength) { SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; - Debug.Assert(context != null); + if (context is null) + { + // The managed context has already been collected. Fail the + // I/O cleanly so SecureTransport tears down the handshake. + return OSStatus.ReadErr; + } try { From 7a3094913ec221677102e96afee06c6aa7e78159 Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Thu, 21 May 2026 18:12:49 +0300 Subject: [PATCH 3/5] Update src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs Co-authored-by: Jan Kotas --- .../src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 9009b39fcef143..f6afa7b9ed44e3 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -194,10 +194,8 @@ private void SslSetConnection(SafeSslHandle sslContext) // The GCHandle is owned by the SafeSslHandle and freed in its // ReleaseHandle override, which only runs once the ref count drops // to zero (i.e. after all outstanding P/Invokes and any in-flight - // native Read/Write callbacks have completed). A strong (Normal) - // handle is used so that this SafeDeleteSslContext stays rooted - // for as long as the native SSLContext can call back into us. - GCHandle handle = GCHandle.Alloc(this, GCHandleType.Normal); + // native Read/Write callbacks have completed). + GCHandle handle = GCHandle.Alloc(this); sslContext.SetConnectionGCHandle(handle); Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); From 744bfe00600465ffd54a7efb04cad31061696216 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ahmet=20=C4=B0brahim=20Aksoy?= Date: Thu, 21 May 2026 18:24:34 +0300 Subject: [PATCH 4/5] Switch to GCHandle and simplify Address review feedback from @jkotas: * Use the strongly-typed GCHandle so Target returns the right type directly without a cast or nullable annotation. * Replace GCHandle.Free/IsAllocated with GCHandle.Dispose, which is a no-op on a default value. * Remove the null checks added to WriteToConnection / ReadFromConnection. With a strong GC handle, Target cannot be null, so the guard was dead code that obscured intent. Tested on osx-arm64: System.Net.Security unit tests (124) and functional tests (4964) all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Interop.Ssl.cs | 9 +++----- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 23 ++++--------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs index 4879e7e756c9e0..3ee769a26c4fae 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs @@ -465,7 +465,7 @@ internal sealed class SafeSslHandle : SafeHandle // Owned here so the lifetime is tied to ReleaseHandle, which only // runs once all outstanding P/Invokes (and therefore any in-flight // callbacks) have completed. - private GCHandle _connectionGCHandle; + private GCHandle _connectionGCHandle; public SafeSslHandle() : base(IntPtr.Zero, ownsHandle: true) @@ -477,7 +477,7 @@ internal SafeSslHandle(IntPtr invalidHandleValue, bool ownsHandle) { } - internal void SetConnectionGCHandle(GCHandle handle) + internal void SetConnectionGCHandle(GCHandle handle) { Debug.Assert(!_connectionGCHandle.IsAllocated, "Connection GCHandle already set"); _connectionGCHandle = handle; @@ -487,10 +487,7 @@ protected override bool ReleaseHandle() { Interop.CoreFoundation.CFRelease(handle); SetHandle(IntPtr.Zero); - if (_connectionGCHandle.IsAllocated) - { - _connectionGCHandle.Free(); - } + _connectionGCHandle.Dispose(); return true; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index f6afa7b9ed44e3..27531adc03d987 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -191,14 +191,11 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions sslAuthen private void SslSetConnection(SafeSslHandle sslContext) { - // The GCHandle is owned by the SafeSslHandle and freed in its - // ReleaseHandle override, which only runs once the ref count drops - // to zero (i.e. after all outstanding P/Invokes and any in-flight // native Read/Write callbacks have completed). - GCHandle handle = GCHandle.Alloc(this); + GCHandle handle = new GCHandle(this); sslContext.SetConnectionGCHandle(handle); - Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); + Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); } public override bool IsInvalid => _sslContext?.IsInvalid ?? true; @@ -225,13 +222,7 @@ protected override void Dispose(bool disposing) [UnmanagedCallersOnly] private static unsafe int WriteToConnection(IntPtr connection, byte* data, void** dataLength) { - SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; - if (context is null) - { - // The managed context has already been collected. Fail the - // I/O cleanly so SecureTransport tears down the handshake. - return OSStatus.WritErr; - } + SafeDeleteSslContext context = GCHandle.FromIntPtr(connection).Target; // We don't pool these buffers and we can't because there's a race between their us in the native // read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently, @@ -265,13 +256,7 @@ private static unsafe int WriteToConnection(IntPtr connection, byte* data, void* [UnmanagedCallersOnly] private static unsafe int ReadFromConnection(IntPtr connection, byte* data, void** dataLength) { - SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target; - if (context is null) - { - // The managed context has already been collected. Fail the - // I/O cleanly so SecureTransport tears down the handshake. - return OSStatus.ReadErr; - } + SafeDeleteSslContext context = GCHandle.FromIntPtr(connection).Target; try { From 000e571c569351227f2160206793a2aa0409984a Mon Sep 17 00:00:00 2001 From: Ahmet Ibrahim Aksoy Date: Mon, 25 May 2026 21:49:31 +0300 Subject: [PATCH 5/5] Update src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs Co-authored-by: Miha Zupan --- .../src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 27531adc03d987..a26029c9f6849f 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -191,8 +191,7 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions sslAuthen private void SslSetConnection(SafeSslHandle sslContext) { - // native Read/Write callbacks have completed). - GCHandle handle = new GCHandle(this); + var handle = new GCHandle(this); sslContext.SetConnectionGCHandle(handle); Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle));