Split WP11_Session_RemoveObject into by-handle and owned-object entries

LinuxJedi · LinuxJedi · commit d5c00fa57d16 · 2026-07-15T13:55:09.000+01:00
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.
diff --git a/src/crypto.c b/src/crypto.c
@@ -1278,9 +1278,7 @@ static CK_RV AddRSAPrivateKeyObject(WP11_Session* session,
 err_out:
     if (rv != CKR_OK) {
         if (*phKey != CK_INVALID_HANDLE) {
-            /* ignore return value, logged in function */
-            (void)WP11_Session_RemoveObject(session, privKeyObject,
-                                            WP11_Object_OnToken(privKeyObject), 0);
+            WP11_Session_RemoveObject(session, privKeyObject);
             *phKey = CK_INVALID_HANDLE;
         }
         if (privKeyObject != NULL) {
@@ -1476,9 +1474,7 @@ CK_RV C_CreateObject(CK_SESSION_HANDLE hSession, CK_ATTRIBUTE_PTR pTemplate,
     }
     rv = AddObject(session, object, pTemplate, ulCount, phObject);
     if (rv != CKR_OK) {
-        /* ignore return value, logged in function */
-        (void)WP11_Session_RemoveObject(session, object,
-                                        WP11_Object_OnToken(object), 0);
+        WP11_Session_RemoveObject(session, object);
         WP11_Object_Free(object);
     }
 
@@ -1697,8 +1693,8 @@ CK_RV C_DestroyObject(CK_SESSION_HANDLE hSession,
     }
     /* Derive onToken from the handle, not the object: it stays valid even if a
      * concurrent destroy of the same handle frees the object out from under
-     * us, and it lets WP11_Session_RemoveObject unlink token objects using
-     * pointer identity alone. */
+     * us, and it lets WP11_Session_RemoveObjectByHandle unlink token objects
+     * using pointer identity alone. */
     onToken = WP11_Object_HandleOnToken(hObject);
 
     /* Only require R/W session for token objects */
@@ -1709,11 +1705,11 @@ CK_RV C_DestroyObject(CK_SESSION_HANDLE hSession,
     }
 
     /* Remove and reject-if-not-destroyable in one locked step. The
-     * CKA_DESTROYABLE check is performed inside WP11_Session_RemoveObject, once
-     * the object is confirmed still linked (and therefore alive), so it cannot
-     * race a concurrent free of the same handle. */
-    ret = WP11_Session_RemoveObject(session, obj, onToken,
-                                    1 /* checkDestroyable */);
+     * CKA_DESTROYABLE check is performed inside WP11_Session_RemoveObjectByHandle,
+     * once the object is confirmed still linked (and therefore alive), so it
+     * cannot race a concurrent free of the same handle. */
+    ret = WP11_Session_RemoveObjectByHandle(session, obj, onToken,
+                                            1 /* checkDestroyable */);
     if (ret == WP11_OBJECT_ALREADY_REMOVED) {
         /* Another thread destroyed this object first (a concurrent
          * C_DestroyObject on the same shared token-object handle). That thread
@@ -7936,14 +7932,12 @@ CK_RV C_GenerateKeyPair(CK_SESSION_HANDLE hSession,
 
     if (rv != CKR_OK && pub != NULL) {
         if (*phPublicKey != CK_INVALID_HANDLE)
-            (void)WP11_Session_RemoveObject(session, pub,
-                                            WP11_Object_OnToken(pub), 0);
+            WP11_Session_RemoveObject(session, pub);
         WP11_Object_Free(pub);
     }
     if (rv != CKR_OK && priv != NULL) {
         if (*phPrivateKey != CK_INVALID_HANDLE)
-            (void)WP11_Session_RemoveObject(session, priv,
-                                            WP11_Object_OnToken(priv), 0);
+            WP11_Session_RemoveObject(session, priv);
         WP11_Object_Free(priv);
     }
 
@@ -8440,9 +8434,7 @@ CK_RV C_UnwrapKey(CK_SESSION_HANDLE hSession,
             }
             if (rv != CKR_OK) {
                 if (*phKey != CK_INVALID_HANDLE) {
-                    /* ignore return value, logged in function */
-                    (void)WP11_Session_RemoveObject(session, keyObj,
-                                                    WP11_Object_OnToken(keyObj), 0);
+                    WP11_Session_RemoveObject(session, keyObj);
                     *phKey = CK_INVALID_HANDLE;
                 }
                 if (keyObj != NULL) {
diff --git a/src/internal.c b/src/internal.c
@@ -922,7 +922,7 @@ static void wp11_Session_Final(WP11_Session* session)
         /* Free objects in session. */
         while ((obj = session->object) != NULL) {
             /* ignore return value, logged in function */
-            (void)WP11_Session_RemoveObject(session, obj, obj->onToken, 0);
+            WP11_Session_RemoveObject(session, obj);
             WP11_Object_Free(obj);
         }
         session->inUse = 0;
@@ -8815,15 +8815,37 @@ int WP11_Session_AddObject(WP11_Session* session, int onToken,
 }
 
 /**
- * Remove object to the session or token.
+ * Remove an object from its session or token list, addressing it by a
+ * caller-supplied onToken flag rather than by dereferencing the object.
  *
- * @param  session  [in]  Session object.
- * @param  object   [in]  Key Object object.
- * @return  -ve on failure.
- *          0 on success.
+ * Membership is established by pointer identity alone, so *object is never
+ * dereferenced until it is confirmed still linked (and therefore still alive).
+ * onToken must be derived from the handle (see WP11_Object_HandleOnToken), not
+ * from *object, so it stays valid even after a concurrent caller frees the
+ * object. This is the entry point for C_DestroyObject, where a second thread may
+ * be destroying the same shared token-object handle at the same time.
+ *
+ * Callers that hold the only reference to a live object should use the simpler
+ * WP11_Session_RemoveObject() wrapper instead: it derives onToken from the
+ * object itself and so cannot mismatch onToken against the wrong object.
+ *
+ * @param  session          [in]  Session performing the removal.
+ * @param  object           [in]  Object to remove, matched by pointer identity.
+ * @param  onToken          [in]  Non-zero if the handle names a token object.
+ *                                Derive from the handle, never from *object.
+ * @param  checkDestroyable [in]  Non-zero to honor CKA_DESTROYABLE and refuse to
+ *                                remove an object marked not destroyable.
+ * @return  WP11_OBJECT_ALREADY_REMOVED if the object was no longer linked, i.e.
+ *          a concurrent caller already removed it; the caller must not free it.
+ *          WP11_OBJECT_NOT_DESTROYABLE if checkDestroyable is set and the object
+ *          has CKA_DESTROYABLE = CK_FALSE; the object is left in place and must
+ *          not be freed.
+ *          -ve on a store failure after the object was unlinked.
+ *          0 on success; the caller now owns the object and must free it.
  */
-int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object,
-                              int onToken, int checkDestroyable)
+int WP11_Session_RemoveObjectByHandle(WP11_Session* session,
+                                      WP11_Object* object, int onToken,
+                                      int checkDestroyable)
 {
     int ret = 0;
     int found = 0;
@@ -8957,6 +8979,25 @@ int WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object,
     return ret;
 }
 
+/**
+ * Remove a live, exclusively-owned object from its session or token list.
+ *
+ * Thin wrapper over WP11_Session_RemoveObjectByHandle() for cleanup paths that
+ * hold the only reference to the object (object-creation rollback, keygen
+ * failure). onToken is read from the object itself, which is safe because no
+ * other thread can be removing it, and supplying onToken this way removes any
+ * risk of passing the wrong object's onToken. CKA_DESTROYABLE is not enforced
+ * (internal cleanup is not a user destroy) and any store error is logged in the
+ * callee, so the return value is intentionally discarded.
+ *
+ * @param  session  [in]  Session performing the removal.
+ * @param  object   [in]  Object to remove; must be live and owned by the caller.
+ */
+void WP11_Session_RemoveObject(WP11_Session* session, WP11_Object* object)
+{
+    (void)WP11_Session_RemoveObjectByHandle(session, object, object->onToken, 0);
+}
+
 /**
  * Drop any session reference to an object that is about to be freed.
  *
@@ -10291,13 +10332,13 @@ int WP11_Object_Find(WP11_Session* session, CK_OBJECT_HANDLE objHandle,
         /* The NSS cross-session walk below needs the slot lock so a concurrent
          * remove-session can't reclaim a session node. Acquire it before the
          * token lock to match the global lock order (slot then token) used by
-         * WP11_Session_RemoveObject / WP11_Slot_CloseSessions; acquiring them
-         * in the opposite order would risk an AB-BA deadlock. */
+         * WP11_Session_RemoveObjectByHandle / WP11_Slot_CloseSessions; acquiring
+         * them in the opposite order would risk an AB-BA deadlock. */
         WP11_Lock_LockRO(&session->slot->lock);
 #endif
         /* Hold the token lock across the session->object walk so a concurrent
-         * WP11_Session_RemoveObject (which holds the same lock around the
-         * unlink) can't free a node we're stepping through. */
+         * WP11_Session_RemoveObjectByHandle (which holds the same lock around
+         * the unlink) can't free a node we're stepping through. */
         WP11_Lock_LockRO(&session->slot->token.lock);
         obj = session->object;
         while (obj != NULL) {
diff --git a/tests/concurrent_destroy_object_test.c b/tests/concurrent_destroy_object_test.c
@@ -27,7 +27,7 @@
  * CKF_OS_LOCKING_OK). The buggy code resolved the handle to a raw WP11_Object*
  * under a released read lock, so both callers reached WP11_Object_Free on the
  * same pointer - a double free - and the loser also dereferenced the freed
- * object inside WP11_Session_RemoveObject.
+ * object inside WP11_Session_RemoveObjectByHandle.
  *
  * Each round below creates one token object and has two threads (each on its
  * own session) destroy it simultaneously. The fix must guarantee that exactly
diff --git a/wolfpkcs11/internal.h b/wolfpkcs11/internal.h
@@ -441,19 +441,21 @@ WP11_LOCAL int WP11_Session_SetMldsaParams(WP11_Session* session, CK_VOID_PTR pa
                                            CK_ULONG paramsLen);
 WP11_LOCAL int WP11_Session_AddObject(WP11_Session* session, int onToken,
                            WP11_Object* object);
-/* Returned by WP11_Session_RemoveObject when the object was no longer linked,
- * i.e. a concurrent caller (e.g. a second C_DestroyObject on the same shared
- * token-object handle) already removed it. A positive value, distinct from the
- * 0 / negative store-status codes returned on a successful removal. The caller
- * must not free the object a second time. */
+/* Returned by WP11_Session_RemoveObjectByHandle when the object was no longer
+ * linked, i.e. a concurrent caller (e.g. a second C_DestroyObject on the same
+ * shared token-object handle) already removed it. A positive value, distinct
+ * from the 0 / negative store-status codes returned on a successful removal. The
+ * caller must not free the object a second time. */
 #define WP11_OBJECT_ALREADY_REMOVED 1
-/* Returned by WP11_Session_RemoveObject (only when checkDestroyable is set) when
- * the object is still linked but has CKA_DESTROYABLE = CK_FALSE. The object is
- * left in place and must not be freed. */
+/* Returned by WP11_Session_RemoveObjectByHandle (only when checkDestroyable is
+ * set) when the object is still linked but has CKA_DESTROYABLE = CK_FALSE. The
+ * object is left in place and must not be freed. */
 #define WP11_OBJECT_NOT_DESTROYABLE 2
-WP11_LOCAL int WP11_Session_RemoveObject(WP11_Session* session,
+WP11_LOCAL int WP11_Session_RemoveObjectByHandle(WP11_Session* session,
                               WP11_Object* object, int onToken,
                               int checkDestroyable);
+WP11_LOCAL void WP11_Session_RemoveObject(WP11_Session* session,
+                              WP11_Object* object);
 WP11_LOCAL int WP11_Object_HandleOnToken(CK_OBJECT_HANDLE handle);
 WP11_LOCAL void WP11_Slot_ClearActiveObject(WP11_Slot* slot,
                                             WP11_Object* object);

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@`
`27`	`27`	`* CKF_OS_LOCKING_OK). The buggy code resolved the handle to a raw WP11_Object*`
`28`	`28`	`* under a released read lock, so both callers reached WP11_Object_Free on the`
`29`	`29`	`* same pointer - a double free - and the loser also dereferenced the freed`
`30`		`- * object inside WP11_Session_RemoveObject.`
	`30`	`+ * object inside WP11_Session_RemoveObjectByHandle.`
`31`	`31`	`*`
`32`	`32`	`* Each round below creates one token object and has two threads (each on its`
`33`	`33`	`* own session) destroy it simultaneously. The fix must guarantee that exactly`