Fix resource leaks and session-pool transition races - #665
Merged
Conversation
mtrojnar
force-pushed
the
fix/resource-session-handling
branch
3 times, most recently
from
July 27, 2026 13:08
a56b4aa to
70532f2
Compare
The second i2d_ASN1_OBJECT() call can fail after ec_params has been allocated. That error path returned the pooled session but leaked the DER buffer. Route curve lookup, DER encoding, and allocation failures through one cleanup path that releases the session and frees ec_params.
When a private RSA object omits CKA_PUBLIC_EXPONENT, pkcs11_get_rsa() builds a temporary template to find the matching public key. Attributes allocated for that template were never freed. Clear the template immediately after the object lookup, its final use, so every lookup outcome releases the allocated attributes.
Key generation dropped slot->lock before switching to R/W mode and then unlocked it again unconditionally. Mode changes could also call C_CloseAllSessions() while another thread still used a checked-out handle. Serialize mode changes with transition_active under the slot lock. Track checked-out sessions explicitly, block normal acquisition for the entire drain, mode switch, relogin, and key-generation checkout, and close sessions only after every lease is returned. Use condition-variable broadcasts so transition and consumer waiters all recheck their predicates. Acquire and relogin the key-generation session without re-entering the normal gated path. Restore pool accounting on every error, and reject invalid releases defensively. Reset transition and pool accounting during slot reload so a child cannot inherit state owned by a vanished parent thread. Keeping all transition state under the existing slot lock avoids a separate mutex that could be inherited locked.
The get, put, and open helper names obscured that these functions manage libp11's per-slot pool rather than direct Cryptoki session ownership. Rename the helpers and every internal caller to describe pool acquisition, release, and mode changes explicitly. Keep PKCS11_open_session() unchanged to preserve the public API and ABI.
mtrojnar
force-pushed
the
fix/resource-session-handling
branch
from
July 27, 2026 13:11
70532f2 to
47a80f7
Compare
Add deterministic tests around a fake PKCS#11 module for transition waiter wakeups, acquisition gating during key-generation relogin, recovery after a failed relogin, and stale transition state after fork reload. Add a bounded SoftHSM stress test that runs signing, key generation, and pool mode changes concurrently. A watchdog turns deadlocks into explicit test failures. Run the focused unit helper through a shell wrapper that selects the configured OpenSSL. Select the same runtime paths for the stress helper after SoftHSM setup, so custom builds load the matching libcrypto and the locally built libp11 without affecting pkcs11-tool. Call OPENSSL_cleanup() at the end of the stress helper to make leak checks deterministic. Guard the terminal cleanup for OpenSSL 1.0.x and LibreSSL versions that do not provide it. Wire both tests into Automake, include their scripts in distributions, and skip the threaded helpers when pthread support is unavailable.
Store a referenced PKCS11_OBJECT_private directly in EVP_PKEY ex-data instead of following a PKCS11_KEY back-pointer. The back-pointer becomes stale when key enumeration reallocates the cached key array, causing concurrent signing and key generation to crash. Remove the now-obsolete back-pointer from the private object structure.
The EVP_PKEY cached on a private EdDSA or XDH object only borrows its legacy ex-data pointer. Returned EVP_PKEY objects acquire their owning object reference through the callback-backed generic ex-data installed by pkcs11_get_key(). Taking another reference while constructing each cached key therefore had no matching release. It kept the object, slot, and associated PIN data alive after ENGINE teardown. Remove those redundant references and document the ownership split.
The Ed448 key path registered pkcs11_ed25519_method_free() with atexit(), leaving the Ed448 EVP_PKEY method allocated and invoking cleanup for the wrong method family. Pair pkcs11_ed448_method_new() with pkcs11_ed448_method_free().
PKCS#11-backed EVP_PKEY objects retain engine-managed key and slot state. Several ENGINE tests called ENGINE_finish() before freeing those keys, so teardown could no longer release the object graph. Leak checks consequently reported the objects, slots, and PIN data. Free every EVP_PKEY before dropping the functional ENGINE reference. Centralize cleanup in the check/copy tests and track whether ENGINE_init() succeeded, so error paths release either the functional or structural reference as appropriate.
OSSL_PARAM_BLD_push_utf8_string() retains the supplied pointer until OSSL_PARAM_BLD_to_param() materializes the parameter array. The EC group-name buffer was scoped to the switch case, so its lifetime ended before the common conversion after the switch. ASan consequently reported a stack-use-after-scope. Move the buffer to function scope so it remains valid through parameter construction.
ERR_load_PKCS11_strings() registers both the P11 and CKR error tables when a context is created. Remove both registrations when the final context releases libp11's process-global state so OpenSSL can reclaim the associated hash entries. CKR reason strings are still registered on OpenSSL 3; only legacy function codes are ignored there.
Private RSA objects store a PKCS11_OBJECT_private pointer in RSA ex-data, and pkcs11_rsa_free_method() releases that reference. The matching pkcs11_object_ref() was incorrectly inside the OpenSSL 3 version guard. On OpenSSL 1.x, freeing the cached EVP_PKEY therefore released the cache's sole reference, and pkcs11_destroy_keys() then accessed the already freed object. Acquire the ex-data-owned reference on every supported OpenSSL version, matching the ownership used for private EC keys.
mtrojnar
force-pushed
the
fix/resource-session-handling
branch
from
July 28, 2026 15:53
7eefaf5 to
86e639c
Compare
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Pull Request Type
Related Issue
Issue number: N/A
Current Behavior
Several key and session-pool paths mishandle resources or synchronization:
pkcs11_ec_keygen()leaks the allocated DER-encoded EC parameters if the secondi2d_ASN1_OBJECT()call fails.pkcs11_get_rsa()leaks attributes in the temporary template used to find a matching public key when a private key does not exposeCKA_PUBLIC_EXPONENT.slot->locktwice after switching the pool to R/W mode, causing undefined mutex behavior. Its mode change, login, and session acquisition can also interleave with another transition.C_CloseAllSessions()even when other threads still hold checked-out sessions, invalidating handles used by concurrent operations.New Behavior
Error paths release their temporary allocations, and session-pool mode changes are serialized safely. A transition blocks new checkouts, waits for active users to return their sessions, and only then closes the Cryptoki sessions and changes mode.
Key generation keeps its mode change, login, and session acquisition serialized against other transitions. Condition-variable broadcasts wake both transition and consumer waiters on POSIX and Windows.
Internal helpers are named explicitly for pool acquisition, release, and mode changes. The public
PKCS11_open_session()API remains unchanged.Scope of Changes
C_CloseAllSessions().pthread_cond_broadcast()compatibility.pkcs11_session_pool_*names without changing the public API or ABI.Testing
Passed:
Both focused SoftHSM tests passed. The modified C translation units also compile cleanly with
-Wall -Wextra; the Windows pthread compatibility wrapper was checked with MinGW; and concurrent EC key generation passed a 10-round stress run.Additional Notes
make checkattempt reported 37 passes, 2 skips, and 9 test-environment failures because existingtests/output.*directories were not writable (Permission deniedwhile creating certificate files).License Declaration