Skip to content

Commit 5d5af62

Browse files
liveansCopilotjkotasMihaZupan
authored
Fix GCHandle leak in OSX SafeDeleteSslContext (#128325)
> [!NOTE] > This PR was authored with help from GitHub Copilot. The `GCHandle` allocated in `SafeDeleteSslContext.SslSetConnection` was never freed, leaking one GCHandle table slot per SSL context on the legacy macOS SecureTransport path. Freeing it in `SafeDeleteSslContext.Dispose` races with in-flight native `Read`/`Write` callbacks (reliably reproduced as a `Debug.Assert` crash in `SslStreamSniTest.SslStream_ServerCallbackAndLocalCertificateSelectionSet_Throws`). Instead, ownership of the handle is moved into `SafeSslHandle` and freed from its `ReleaseHandle`, which only runs after the ref count hits zero and the native `SSLContext` is released, so no further callbacks can fire. Tested locally on osx-arm64: `System.Net.Security.Unit.Tests` (124) and `System.Net.Security.Tests` functional (4964) all pass. Fixes #128136 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
1 parent 822530f commit 5d5af62

2 files changed

Lines changed: 20 additions & 6 deletions

File tree

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,13 @@ namespace System.Net
460460
{
461461
internal sealed class SafeSslHandle : SafeHandle
462462
{
463+
// Backreference used by AppleCryptoNative_SslSetConnection so native
464+
// Read/Write callbacks can resolve the owning SafeDeleteSslContext.
465+
// Owned here so the lifetime is tied to ReleaseHandle, which only
466+
// runs once all outstanding P/Invokes (and therefore any in-flight
467+
// callbacks) have completed.
468+
private GCHandle<SafeDeleteSslContext> _connectionGCHandle;
469+
463470
public SafeSslHandle()
464471
: base(IntPtr.Zero, ownsHandle: true)
465472
{
@@ -470,10 +477,18 @@ internal SafeSslHandle(IntPtr invalidHandleValue, bool ownsHandle)
470477
{
471478
}
472479

480+
internal void SetConnectionGCHandle(GCHandle<SafeDeleteSslContext> handle)
481+
{
482+
Debug.Assert(!_connectionGCHandle.IsAllocated, "Connection GCHandle already set");
483+
_connectionGCHandle = handle;
484+
}
485+
473486
protected override bool ReleaseHandle()
474487
{
475488
Interop.CoreFoundation.CFRelease(handle);
476489
SetHandle(IntPtr.Zero);
490+
_connectionGCHandle.Dispose();
491+
477492
return true;
478493
}
479494

src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -191,9 +191,10 @@ private static SafeSslHandle CreateSslContext(SslAuthenticationOptions sslAuthen
191191

192192
private void SslSetConnection(SafeSslHandle sslContext)
193193
{
194-
GCHandle handle = GCHandle.Alloc(this, GCHandleType.Weak);
194+
var handle = new GCHandle<SafeDeleteSslContext>(this);
195+
sslContext.SetConnectionGCHandle(handle);
195196

196-
Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle));
197+
Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle<SafeDeleteSslContext>.ToIntPtr(handle));
197198
}
198199

199200
public override bool IsInvalid => _sslContext?.IsInvalid ?? true;
@@ -220,8 +221,7 @@ protected override void Dispose(bool disposing)
220221
[UnmanagedCallersOnly]
221222
private static unsafe int WriteToConnection(IntPtr connection, byte* data, void** dataLength)
222223
{
223-
SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target;
224-
Debug.Assert(context != null);
224+
SafeDeleteSslContext context = GCHandle<SafeDeleteSslContext>.FromIntPtr(connection).Target;
225225

226226
// We don't pool these buffers and we can't because there's a race between their us in the native
227227
// read/write callbacks and being disposed when the SafeHandle is disposed. This race is benign currently,
@@ -255,8 +255,7 @@ private static unsafe int WriteToConnection(IntPtr connection, byte* data, void*
255255
[UnmanagedCallersOnly]
256256
private static unsafe int ReadFromConnection(IntPtr connection, byte* data, void** dataLength)
257257
{
258-
SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target;
259-
Debug.Assert(context != null);
258+
SafeDeleteSslContext context = GCHandle<SafeDeleteSslContext>.FromIntPtr(connection).Target;
260259

261260
try
262261
{

0 commit comments

Comments
 (0)