Skip to content

make sure OpenSSL is initialized before Tls13Supported code runs #62037

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

make sure OpenSSL is initialized before Tls13Supported code runs #62037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions src/libraries/Common/src/Interop/Unix/System.Net.Security.Native/Interop.Initialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,42 @@ static Ssl()
{
SslInitializer.Initialize();
}

// access for static properties that may be called before static constructor.
internal static void EsureInitialized()
{
SslInitializer.Initialize();
}
}

internal static class SslInitializer
{
#if !SYSNETSECURITY_NO_OPENSSL
private static readonly object _initLock = new object();
private static bool _initialized;

#pragma warning disable CA1810
static SslInitializer()
{
CryptoInitializer.Initialize();

//Call ssl specific initializer
Ssl.EnsureLibSslInitialized();
if (Interop.Crypto.ErrPeekLastError() != 0)
lock (_initLock)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even just making a nested type will fix it... e.g. Interop.Ssl.Tls13Supported => Interop.Ssl.Capabilities.Tls13Supported

This makes sense to me.

{
// It is going to be wrapped in a type load exception but will have the error information
throw Interop.Crypto.CreateOpenSslCryptographicException();
if (!_initialized)
{
CryptoInitializer.Initialize();

//Call ssl specific initializer
Ssl.EnsureLibSslInitialized();
if (Interop.Crypto.ErrPeekLastError() != 0)
{
// It is going to be wrapped in a type load exception but will have the error information
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

_initialized = true;
}
}
}
#pragma warning restore CA1810
#endif

internal static void Initialize()
Expand Down
10 changes: 8 additions & 2 deletions src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,14 @@ internal static byte[] ConvertAlpnProtocolListToByteArray(List<SslApplicationPro
}

[GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_Tls13Supported")]
private static partial int Tls13SupportedImpl();
internal static readonly bool Tls13Supported = Tls13SupportedImpl() != 0;
private static partial int Tls13SupportedNative();
internal static readonly bool Tls13Supported = Tls13SupportedImpl();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


internal static bool Tls13SupportedImpl()
{
EsureInitialized();
return Tls13SupportedNative() != 0;
}

internal static SafeSharedX509NameStackHandle SslGetClientCAList(SafeSslHandle ssl)
{
Expand Down