make sure OpenSSL is initialized before Tls13Supported code runs #62037
Conversation
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsThis is regression caused by #46640. With 6.0, it will be initialized when Ssl us used but some properties like CiphersSuitePolicy can be used before in order to prepare everything needed. @bartonjs suggested different fix on #61891. It works as expected but I did not like the fact that it removed the In similar note, Comments on the change agreed that fixes #61891.
|
| //Call ssl specific initializer | ||
| Ssl.EnsureLibSslInitialized(); | ||
| if (Interop.Crypto.ErrPeekLastError() != 0) | ||
| lock (_initLock) |
There was a problem hiding this comment.
None of this locking is necessary, or really even useful. It can run in parallel across (today) 4 different assemblies. Even after I'm done collapsing things it'll still be two. So the called code is (supposed to be) defensive against concurrency.
There was a problem hiding this comment.
I know it can run. But that will make it visible to strace or run with LD_DEBUG, would it not?
What I'm trying to argue is that we can do unnecessary file system operation and system calls.
There was a problem hiding this comment.
The library load will only ever happen once, the P/Invoke into EnsureLibSslInitialized will only happen once per assembly (cctor guarantee), and the cascaded EnsureOpenSslInitialized will only happen once per assembly. If you do the change as currently structured but remove the locking then there might be two calls to EnsureLibSslInitialized (one from the cctor, one from the extra explicit call).
Restructuring things to move the field off this class will keep things simple looking while staying with the current initialize-count. Even just making a nested type will fix it... e.g. Interop.Ssl.Tls13Supported => Interop.Ssl.Capabilities.Tls13Supported.
There was a problem hiding this comment.
Even just making a nested type will fix it... e.g. Interop.Ssl.Tls13Supported => Interop.Ssl.Capabilities.Tls13Supported
This makes sense to me.
| private static partial int Tls13SupportedImpl(); | ||
| internal static readonly bool Tls13Supported = Tls13SupportedImpl() != 0; | ||
| private static partial int Tls13SupportedNative(); | ||
| internal static readonly bool Tls13Supported = Tls13SupportedImpl(); |
There was a problem hiding this comment.
Rather than all this work, if the readonly-ness is still important, I'd suggest just moving the static field to a different type (just somewhere in System.Net.Security instead of in Common/src/Interop)
There was a problem hiding this comment.
I don't have any benchmarks. But it seems like doing something once is better than possibly insure some repeatedly.
Of course, crypt is CPU expensive so the benefit may come more from code generation/elimination IMHO.
ghost
locked as resolved and limited conversation to collaborators
This is regression caused by #46640.
Tls13Supportedcan be called before static constructor runs. It was OK in 5.0 since the OpenSSL library was initialized viaWith 6.0, it will be initialized when Ssl us used but some properties like CiphersSuitePolicy can be used before in order to prepare everything needed.
@bartonjs suggested different fix on #61891. It works as expected but I did not like the fact that it removed the
readonlyattribute onTls13Supported. I don't know if the impact is measurable but it feels like it would better to do little bit more one time instead of loosing some optimization on every SslStream.In similar note, Comments on the change agreed that
InitializeOpenSSLShimis idempotent. While that is true, loading symbols and library is externally visible and non-trivial amount of system calls and other operations. When somebody debugs loading, seeing the operation multiple times would not make it easier. I added simple lock to make the loading exactly once like we did in 5.0. Since this is static method, the extra cost should be negligible IMHO.fixes #61891.