Fix GCHandle leak in OSX SafeDeleteSslContext#128325
Conversation
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 dotnet#128136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Pull request overview
This PR updates the macOS SecureTransport PAL to ensure the GCHandle used as the SSLConnectionRef backreference is freed when the underlying SSLContext is released, rather than leaking for the lifetime of the process.
Changes:
- Store the
GCHandleallocated inSafeDeleteSslContext.SslSetConnectiononSafeSslHandle. - Free the stored
GCHandlefromSafeSslHandle.ReleaseHandle()to align its lifetime with the native SSL context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs | Allocates the connection GCHandle and transfers ownership to SafeSslHandle before calling into SslSetConnection. |
| src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ssl.cs | Extends SafeSslHandle to retain and free the connection GCHandle during ReleaseHandle(). |
| sslContext.SetConnectionGCHandle(handle); | ||
|
|
||
| Interop.AppleCrypto.SslSetConnection(sslContext, GCHandle.ToIntPtr(handle)); |
| // 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); |
There was a problem hiding this comment.
Is this meant to be a weak handle? Target of weak handles can become null, but the Read/Write methods do not expect it.
There was a problem hiding this comment.
Probably not. @rzikm are you aware if there is any historical reason for it to be Weak handle?
There was a problem hiding this comment.
no Idea, the handle was added in #55947, @MaximLipnin do you remember?
I am fine with just changing it into strong GC handle.
Note
This PR was authored with help from GitHub Copilot.
The
GCHandleallocated inSafeDeleteSslContext.SslSetConnectionwas never freed, leaking one GCHandle table slot per SSL context on the legacy macOS SecureTransport path.Freeing it in
SafeDeleteSslContext.Disposeraces with in-flight nativeRead/Writecallbacks (reliably reproduced as aDebug.Assertcrash inSslStreamSniTest.SslStream_ServerCallbackAndLocalCertificateSelectionSet_Throws). Instead, ownership of the handle is moved intoSafeSslHandleand freed from itsReleaseHandle, which only runs after the ref count hits zero and the nativeSSLContextis released, so no further callbacks can fire.Tested locally on osx-arm64:
System.Net.Security.Unit.Tests(124) andSystem.Net.Security.Testsfunctional (4964) all pass.Fixes #128136