PoC TLS resume on Linux client #64369
feedback from review
- Loading branch information
commit 99240d4baeaefe53aaf1791c4075393f6dfd8aae
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ | |
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Net.Security; | ||
|
|
@@ -34,7 +33,7 @@ internal static partial class Ssl | |
| internal static unsafe partial void SslCtxSetAlpnSelectCb(SafeSslContextHandle ctx, delegate* unmanaged<IntPtr, byte**, byte*, byte*, uint, IntPtr, int> callback, IntPtr arg); | ||
|
|
||
| [GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslCtxSetCaching")] | ||
| internal static unsafe partial int SslCtxSetCaching(SafeSslContextHandle ctx, int mode, delegate* unmanaged<IntPtr, IntPtr, int> neewSessionCallback, delegate* unmanaged<IntPtr, IntPtr, void> removeSessionCallback); | ||
|
|
||
| internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, X509Certificate2[] chain) | ||
| { | ||
|
|
@@ -61,7 +60,8 @@ namespace Microsoft.Win32.SafeHandles | |
| { | ||
| internal sealed class SafeSslContextHandle : SafeHandle | ||
| { | ||
| // This is session cache keyed by SNI e.g. TargetHost | ||
| private Dictionary<string, IntPtr>? _sslSessions; | ||
| private GCHandle _gch; | ||
|
|
||
| public SafeSslContextHandle() | ||
|
|
@@ -81,60 +81,74 @@ public override bool IsInvalid | |
|
|
||
| protected override bool ReleaseHandle() | ||
| { | ||
| if (_sslSessions != null) | ||
| { | ||
| // The SSL_CTX is ref counted and may not immediately die when we call SslCtxDestroy() | ||
| // Since there is no relation between SafeSslContextHandle and SafeSslHandle `this` can be release | ||
| // while we still have SSL session using it. | ||
| Interop.Ssl.SslCtxSetData(handle, IntPtr.Zero); | ||
|
|
||
| lock (_sslSessions) | ||
| { | ||
| foreach (IntPtr session in _sslSessions.Values) | ||
| { | ||
| Interop.Ssl.SessionFree(session); | ||
| } | ||
|
|
||
| _sslSessions.Clear(); | ||
| } | ||
|
|
||
| Debug.Assert(_gch.IsAllocated); | ||
| _gch.Free(); | ||
| } | ||
|
|
||
| Interop.Ssl.SslCtxDestroy(handle); | ||
| SetHandle(IntPtr.Zero); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| internal void EnableSessionCache() | ||
| { | ||
| Debug.Assert(_sslSessions == null); | ||
|
|
||
| _sslSessions = new Dictionary<string, IntPtr>(); | ||
| _gch = GCHandle.Alloc(this); | ||
wfurt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Debug.Assert(_gch.IsAllocated); | ||
| // This is needed so we can find the handle from session in SessionRemove callback. | ||
| Interop.Ssl.SslCtxSetData(this, (IntPtr)_gch); | ||
| } | ||
|
|
||
| internal bool TryAddSession(IntPtr namePtr, IntPtr session) | ||
| { | ||
| Debug.Assert(_sslSessions != null && session != IntPtr.Zero); | ||
|
|
||
| if (_sslSessions == null || namePtr == IntPtr.Zero) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| string? targetName = Marshal.PtrToStringAnsi(namePtr); | ||
| Debug.Assert(targetName != null); | ||
|
|
||
| if (!string.IsNullOrEmpty(targetName)) | ||
| { | ||
| // We do this only for lookup in RemoveSession. | ||
| // Since this is part of chache manipulation and no function impact it is done here. | ||
| // This will use strdup() so it is safe to pass in raw pointer. | ||
| Interop.Ssl.SessionSetHostname(session, namePtr); | ||
|
|
||
| lock (_sslSessions) | ||
|
Member
There was a problem hiding this comment. The locking around _sslSessions makes sense, since you're manipulating state depending on how the dictionary performed. But, since you're already locking it, it feels like you want a non-Concurrent dictionary.
Member
Author
There was a problem hiding this comment. agreed. I was also thinking about grabbing extra reference on the session. That would allow me to use ConcurrentDictionary without locking as the session would never be released in the middle.
Member
There was a problem hiding this comment.
Something like ? (Upref inside has a race condition with the cleanup in ReleaseHandle) That would get a little weird since in the cleanup you'd need to call free twice, I think? The fact that we wrote ConcurrentDictionary suggests that it gives better perf (on average) than manual locking... but if the code to interact with it is doing memory/lifetime management and it becomes unreadable with the gymnastics... then locking is better for maintainability. (If it's clean code and more performant, than by all means use upref+ConcurrentDictionary)
Member
Author
There was a problem hiding this comment. yes. and then call free twice. I'm inclined to good with the lock and better maintainability as the perf does not depend on this. This happens one in while - not even for each SSL session. I started with ConcurrentDictionary but you are right - we don't need it at this point. |
||
| { | ||
| if (!_sslSessions.TryAdd(targetName, session)) | ||
| { | ||
| if (_sslSessions.Remove(targetName, out IntPtr oldSession)) | ||
| { | ||
| Interop.Ssl.SessionFree(oldSession); | ||
| } | ||
|
|
||
| bool added = _sslSessions.TryAdd(targetName, session); | ||
| Debug.Assert(added); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -144,21 +158,33 @@ public bool TryAddSession(IntPtr serverName, IntPtr session) | |
| return false; | ||
| } | ||
|
|
||
| internal void RemoveSession(IntPtr namePtr, IntPtr session) | ||
| { | ||
| Debug.Assert(_sslSessions != null); | ||
|
|
||
| string? targetName = Marshal.PtrToStringAnsi(namePtr); | ||
| Debug.Assert(targetName != null); | ||
|
|
||
| if (_sslSessions != null && targetName != null) | ||
| { | ||
| IntPtr oldSession; | ||
| bool removed; | ||
| lock (_sslSessions) | ||
| { | ||
| removed = _sslSessions.Remove(targetName, out oldSession); | ||
| } | ||
|
|
||
| if (removed) | ||
| { | ||
| // It seems like we may be called more than once. Since we grabbed only one refference | ||
| // when added to Dictionary, we will also drop exactly one when removed. | ||
| Interop.Ssl.SessionFree(oldSession); | ||
| } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| internal bool TrySetSession(SafeSslHandle sslHandle, string name) | ||
| { | ||
| Debug.Assert(_sslSessions != null); | ||
|
|
||
|
|
@@ -169,6 +195,7 @@ public bool TrySetSession(SafeSslHandle sslHandle, string name) | |
|
|
||
| // even if we don't have matching session, we can get new one and we need | ||
| // way how to link SSL back to `this`. | ||
| Debug.Assert(Interop.Ssl.SslGetData(sslHandle) == IntPtr.Zero); | ||
| Interop.Ssl.SslSetData(sslHandle, (IntPtr)_gch); | ||
wfurt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| lock (_sslSessions) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -889,26 +889,4 @@ int local_EVP_PKEY_public_check(EVP_PKEY_CTX* ctx) | |
| return -1; | ||
| } | ||
| } | ||
| #endif | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.