Prevent double free in concurrent C_DestroyObject on shared token object#205
Merged
philljj merged 3 commits intoJul 15, 2026
Merged
Conversation
C_DestroyObject resolved a handle to a raw WP11_Object* under a released read lock and then unconditionally freed it. Token objects are shared across sessions, so two sessions destroying the same handle at once (a supported concurrent use under CKF_OS_LOCKING_OK) could both reach WP11_Object_Free on the same pointer, and the losing thread also dereferenced the freed object inside WP11_Session_RemoveObject. WP11_Session_RemoveObject now takes an onToken argument derived from the handle rather than the object, confirms the object is still linked using pointer identity under the token lock (never dereferencing a possibly freed object on the token path), and returns WP11_OBJECT_ALREADY_REMOVED when a concurrent caller already unlinked it. C_DestroyObject gates the free on that result: the loser returns CKR_OBJECT_HANDLE_INVALID without touching the object again. The successful-removal path is unchanged. Only the token lock is taken, since wp11_Session_Final already calls WP11_Session_RemoveObject while holding the slot lock. Add WP11_Object_HandleOnToken to derive onToken from a handle, update the remaining callers to pass onToken, and add tests/concurrent_destroy_object_test.c which races two sessions destroying one token object and checks exactly one winner per round with no double free. F-5252
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens C_DestroyObject against concurrent destruction of shared token objects (valid under CKF_OS_LOCKING_OK) by preventing a double-free / use-after-free when two sessions destroy the same handle at the same time.
Changes:
- Extend
WP11_Session_RemoveObject()to accept anonTokenargument and returnWP11_OBJECT_ALREADY_REMOVEDwhen a concurrent caller already unlinked the object. - Add
WP11_Object_HandleOnToken()and updateC_DestroyObjectto deriveonTokenfrom the handle and avoid freeing on the “already removed” path. - Add a new multithreaded regression test and wire it into the test build.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfpkcs11/internal.h |
Updates WP11_Session_RemoveObject API and introduces WP11_OBJECT_ALREADY_REMOVED and WP11_Object_HandleOnToken() declaration. |
src/internal.c |
Implements the new remove semantics under the token lock and adds WP11_Object_HandleOnToken(). |
src/crypto.c |
Updates callers for new signature and adjusts C_DestroyObject to avoid double-free on concurrent destroys. |
tests/include.am |
Adds the new concurrent destroy test target to the test build. |
tests/concurrent_destroy_object_test.c |
Adds a race-focused regression test for concurrent C_DestroyObject on a shared token object. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on the concurrent-destroy fix: - Perform the CKA_DESTROYABLE check inside WP11_Session_RemoveObject, under the token lock and only after the object is confirmed still linked (and therefore alive), via a new checkDestroyable argument. Previously C_DestroyObject dereferenced the object for WP11_Object_IsDestroyable before removal could confirm liveness, which could read freed memory when another thread destroyed the same handle first. - Look up session objects on the caller's own session list first, by pointer identity, and only fall back to object->session under NSS. This avoids dereferencing a possibly freed object on the common path; the remaining NSS cross-session fallback is the same out-of-contract case as a single-session concurrent misuse. - Return CKR_FUNCTION_FAILED from C_DestroyObject when the object was unlinked but persisting the token afterwards failed, instead of always reporting CKR_OK and masking the error. - Include <stdlib.h> in the regression test for atoi().
philljj
requested changes
Jul 14, 2026
philljj
left a comment
philljj
left a comment
Contributor
There was a problem hiding this comment.
Look good and tests good!
Just some suggestions to remove complexity and document changes a bit better.
Address review feedback on the concurrent C_DestroyObject fix. Rename the full removal routine to WP11_Session_RemoveObjectByHandle, which takes a handle-derived onToken and a checkDestroyable flag. This is the entry point for C_DestroyObject, where a concurrent destroy of the same shared token-object handle may free the object, so onToken must come from the handle rather than from a dereference of the object. Add a WP11_Session_RemoveObject(session, object) wrapper for cleanup paths that hold the only reference to a live object. It reads onToken from the object itself, which removes the risk of passing the wrong object's onToken, and drops the repeated (void)..., 0 boilerplate at the six cleanup call sites. Document both params and every return code (WP11_OBJECT_ALREADY_REMOVED, WP11_OBJECT_NOT_DESTROYABLE, negative store status, 0) in the function comment and header so callers know what to do in each case.
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
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.
C_DestroyObject resolved a handle to a raw WP11_Object* under a released read lock and then unconditionally freed it. Token objects are shared across sessions, so two sessions destroying the same handle at once (a supported concurrent use under CKF_OS_LOCKING_OK) could both reach WP11_Object_Free on the same pointer, and the losing thread also dereferenced the freed object inside WP11_Session_RemoveObject.
WP11_Session_RemoveObject now takes an onToken argument derived from the handle rather than the object, confirms the object is still linked using pointer identity under the token lock (never dereferencing a possibly freed object on the token path), and returns WP11_OBJECT_ALREADY_REMOVED when a concurrent caller already unlinked it. C_DestroyObject gates the free on that result: the loser returns CKR_OBJECT_HANDLE_INVALID without touching the object again. The successful-removal path is unchanged. Only the token lock is taken, since wp11_Session_Final already calls WP11_Session_RemoveObject while holding the slot lock.
Add WP11_Object_HandleOnToken to derive onToken from a handle, update the remaining callers to pass onToken, and add
tests/concurrent_destroy_object_test.c which races two sessions destroying one token object and checks exactly one winner per round with no double free.
F-5252