Fix resource leaks and session-pool transition races#665
Open
mtrojnar wants to merge 7 commits into
Open
Conversation
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 unconditionally unlocked it again. Besides invoking undefined mutex behavior, its mode switch, login, and session acquisition could interleave with another transition. A mode switch also called C_CloseAllSessions() while other threads could still be using handles checked out from the pool, invalidating active sessions. Serialize mode changes with a dedicated transition mutex. Block new checkouts while a transition drains the pool, wait for existing users to return their sessions, and only then close all sessions. Keep key generation's transition, login, and checkout serialized against other mode changes. Broadcast condition-variable wakeups during transitions, including on Windows, so both transition and consumer waiters can make progress.
The get, put, and open helper names obscured that these functions manage libp11's per-slot pool rather than direct Cryptoki session ownership. Name the helpers for acquiring and releasing pooled sessions and for changing the pool-wide access mode. This is a mechanical internal rename with no behavior change. Keep PKCS11_open_session() unchanged to preserve the public API and ABI.
A forked child can inherit a session-pool mode transition while the parent has a transition in progress. The transition owner thread does not exist in the child, so a child-side session checkout can wait forever for a condition-variable broadcast that can never happen. The dedicated transition mutex had the same fork-safety problem when inherited locked. Keep transition serialization under the existing slot lock instead of a separate transition_lock. transition_active now reserves ownership of a mode transition, while checkout_blocked only gates ordinary session checkouts during the pool drain before C_CloseAllSessions(). This keeps mode switches serialized without requiring owner-only acquire/login helper variants. Key generation reserves the transition while it switches to R/W mode, restores login, and checks out the R/W session, so another transition cannot interleave. Reset transition_active and checkout_blocked during slot reload after fork, because no transition owner can survive into the child.
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