Skip to content

Keep OSSL_LIB_CTX handles alive forever#129435

Open
vcsjones wants to merge 5 commits into
dotnet:mainfrom
vcsjones:oss-lib-ctx-forever
Open

Keep OSSL_LIB_CTX handles alive forever#129435
vcsjones wants to merge 5 commits into
dotnet:mainfrom
vcsjones:oss-lib-ctx-forever

Conversation

@vcsjones

Copy link
Copy Markdown
Member

Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers per-thread callbacks to mutate thread-local state. Other OpenSSL APIs, like random number generation, also have thread-local state. In the case of the random number generator, it registered a thread data destructor to clean up random state when the thread is shutting down. This, in turn, sees that the OSSL_LIB_CTX is associated with that thread.

If the OSSL_LIB_CTX is freed before the thread has shut down, the thread dtor will access invalid memory.

OpenSSL requires you to call OPENSSL_thread_stop_ex on all threads before calling OSSL_LIB_CTX_free to unhook the context from the thread.

This is generally not feasible for .NET.

As a solution, we are going to not free the context, and re-use it for each provider. Every provider loaded with SafeEvpPKeyHandle.OpenKeyFromProvider will create a context and load that provider into it. The context and provider will remain alive for the duration of the process.

The number of providers expected to be used in any application is generally expected to be low - likely two or three.

  1. The provider and context are now cached, and kept in a heap-backed buffer. There is no resizing strategy or pre-allocated buffer. Opening a new provider results in re-sizing the array. This strategy is likely fine because there will rarely ever be more than a few providers, as already noted. The resizing is managed by a mutex to synchronize access to the array. We could use something like pthread_rwlock_t if we wanted to increase complexity. Given that contention on this mutex would only happen from OpenKeyFromProvider, I expect it to be rare.

  2. Since the context and provider are now alive permanently, the "extra handle" bookkeeping gets simpler. We no longer need to pass it to destroy, and we no longer need to ref-count it. In fact we don't even need a struct at all anymore, the OSSL_LIB_CTX is the extra handle now.

  3. This reverts Add ActiveIssue for all named key tests to mitigate invalid memory access #129367 since the data dependency is resolved.

Fixes #129339

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR changes the OpenSSL provider-key implementation to cache OSSL_LIB_CTX / provider state for the lifetime of the process, simplifying “extra handle” lifetime management and re-enabling the OpenSSL named key test coverage that was previously disabled due to suspected invalid memory access.

Changes:

  • Introduces a per-provider cache of OSSL_LIB_CTX + OSSL_PROVIDER in the native OpenSSL PAL and stops freeing these contexts during normal key destruction.
  • Simplifies the EVP_PKEY native interop surface (EvpPkeyDestroy, UpRefEvpPkey) by removing the extra-handle refcounting scheme.
  • Removes ActiveIssue skips from OpenSSL named-key crypto and SslStream functional tests.
Show a summary per file
File Description
src/native/libs/System.Security.Cryptography.Native/pal_ssl.c Updates EVP_PKEY destroy call to the simplified signature.
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.h Removes EvpPKeyExtraHandle definition and updates function signatures.
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Adds a global provider/libctx cache and removes extra-handle refcounting.
src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.cs Re-enables named key tests by removing ActiveIssue attributes.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs Stops passing ExtraHandle into destroy; clarifies ExtraHandle semantics.
src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamOpenSslNamedKeysTests.manual.cs Re-enables SslStream TPM/provider tests by removing ActiveIssue attributes.
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.cs Updates P/Invoke signatures and adjusts failure cleanup accordingly.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 1

Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated
This updates the LoadKeyFromProvider to treat extraHandle as an inout. When it points to NULL, that means create a new "extra" handle, in this case an OSSL_LIB_CTX. When it doesn't point to NULL, that means use this existing value. This keeps the number of P/Invokes the same, as well as keeping extra handle opaque - the managed side does not know that it is an OSSL_LIB_CTX.
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c Outdated

@bartonjs bartonjs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dictionary is overkill, but easy to work with, so... eh, it's fine.

Copilot AI review requested due to automatic review settings June 15, 2026 22:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 3

Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c
Comment thread src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Arm/Arm64: Segfault in System.Security.Cryptography.Tests

4 participants