-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[QUIC] Cleaned up TlsSecret and added test. #93119
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAdded test for SSLKEYLOGFILE, fixed missing writing of the secret and removed direct reference from safe handle (now held by IDisposable).
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAdded test for SSLKEYLOGFILE, fixed missing writing of the secret and removed direct reference from safe handle (now held by IDisposable).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicTlsSecret.cs
Outdated
Show resolved
Hide resolved
{ | ||
NativeMemory.Clear(_tlsSecrets, (nuint)sizeof(QUIC_TLS_SECRETS)); | ||
NativeMemory.Free(_tlsSecrets); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is safe. It feels like we can free the memory while MsQuic still may be trying to write to that. That was one reason why originally the native memory was tight to the MsQuic handle lifecycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dispose is called when the connection handle gets released, not from QuicConnection.DisposeAsync
.
c131283
to
58851d5
Compare
58851d5
to
51a5be4
Compare
Added test for SSLKEYLOGFILE, fixed missing writing of the secret and removed direct reference from safe handle (now held by IDisposable).