Bug 1508776 - Remove unneeded refcounting from SFTKSession r=mt,kjacobs
authorJ.C. Jones <jjones@mozilla.com>
Fri, 27 Sep 2019 21:24:52 +0000 (2019-09-27)
changeset 15314 5619cbbca3db5f583a888181ec024069a46087aa
parent 15313 be9c48ad76cb8fc5b4cba27f7524f4e32bf3ecc9
child 15315 c2036484971341696ae2d9f84b1fc48f3344019d
push id3519
push userjjones@mozilla.com
push dateFri, 27 Sep 2019 22:03:15 +0000 (2019-09-27)
reviewersmt, kjacobs
bugs1508776
Bug 1508776 - Remove unneeded refcounting from SFTKSession r=mt,kjacobs SFTKSession objects are only ever actually destroyed at PK11 session closure, as the session is always the final holder -- and asserting refCount == 1 shows that to be true. Because of that, NSC_CloseSession can just call `sftk_DestroySession` directly and leave `sftk_FreeSession` as a no-op to be removed in the future. Differential Revision: https://phabricator.services.mozilla.com/D47010
lib/softoken/pkcs11.c
lib/softoken/pkcs11i.h
lib/softoken/pkcs11u.c
--- a/lib/softoken/pkcs11.c
+++ b/lib/softoken/pkcs11.c
@@ -2800,18 +2800,19 @@ sftk_CloseAllSessions(SFTKSlot *slot, PR
                 --slot->sessionCount;
                 SKIP_AFTER_FORK(PZ_Unlock(slot->slotLock));
                 if (session->info.flags & CKF_RW_SESSION) {
                     (void)PR_ATOMIC_DECREMENT(&slot->rwSessionCount);
                 }
             } else {
                 SKIP_AFTER_FORK(PZ_Unlock(lock));
             }
-            if (session)
-                sftk_FreeSession(session);
+            if (session) {
+                sftk_DestroySession(session);
+            }
         } while (session != NULL);
     }
     return CKR_OK;
 }
 
 /*
  * shut down the databases.
  * we get the slot lock (which also protects slot->certDB and slot->keyDB)
@@ -4039,18 +4040,16 @@ NSC_CloseSession(CK_SESSION_HANDLE hSess
     sessionFound = PR_FALSE;
 
     /* lock */
     lock = SFTK_SESSION_LOCK(slot, hSession);
     PZ_Lock(lock);
     if (sftkqueue_is_queued(session, hSession, slot->head, slot->sessHashSize)) {
         sessionFound = PR_TRUE;
         sftkqueue_delete(session, hSession, slot->head, slot->sessHashSize);
-        session->refCount--; /* can't go to zero while we hold the reference */
-        PORT_Assert(session->refCount > 0);
     }
     PZ_Unlock(lock);
 
     if (sessionFound) {
         SFTKDBHandle *handle;
         handle = sftk_getKeyDB(slot);
         PZ_Lock(slot->slotLock);
         if (--slot->sessionCount == 0) {
@@ -4061,19 +4060,20 @@ NSC_CloseSession(CK_SESSION_HANDLE hSess
         }
         PZ_Unlock(slot->slotLock);
         if (handle) {
             sftk_freeDB(handle);
         }
         if (session->info.flags & CKF_RW_SESSION) {
             (void)PR_ATOMIC_DECREMENT(&slot->rwSessionCount);
         }
-    }
-
-    sftk_FreeSession(session);
+        sftk_DestroySession(session);
+        session = NULL;
+    }
+
     return CKR_OK;
 }
 
 /* NSC_CloseAllSessions closes all sessions with a token. */
 CK_RV
 NSC_CloseAllSessions(CK_SLOT_ID slotID)
 {
     SFTKSlot *slot;
--- a/lib/softoken/pkcs11i.h
+++ b/lib/softoken/pkcs11i.h
@@ -280,17 +280,16 @@ struct SFTKSessionContextStr {
 
 /*
  * Sessions (have objects)
  */
 struct SFTKSessionStr {
     SFTKSession *next;
     SFTKSession *prev;
     CK_SESSION_HANDLE handle;
-    int refCount;
     PZLock *objectLock;
     int objectIDCount;
     CK_SESSION_INFO info;
     CK_NOTIFY notify;
     CK_VOID_PTR appData;
     SFTKSlot *slot;
     SFTKSearchResults *search;
     SFTKSessionContext *enc_context;
@@ -678,16 +677,17 @@ extern void sftk_FreeObjectList(SFTKObje
 extern void sftk_FreeSearch(SFTKSearchResults *search);
 extern CK_RV sftk_handleObject(SFTKObject *object, SFTKSession *session);
 
 extern SFTKSlot *sftk_SlotFromID(CK_SLOT_ID slotID, PRBool all);
 extern SFTKSlot *sftk_SlotFromSessionHandle(CK_SESSION_HANDLE handle);
 extern CK_SLOT_ID sftk_SlotIDFromSessionHandle(CK_SESSION_HANDLE handle);
 extern SFTKSession *sftk_SessionFromHandle(CK_SESSION_HANDLE handle);
 extern void sftk_FreeSession(SFTKSession *session);
+extern void sftk_DestroySession(SFTKSession *session);
 extern SFTKSession *sftk_NewSession(CK_SLOT_ID slotID, CK_NOTIFY notify,
                                     CK_VOID_PTR pApplication, CK_FLAGS flags);
 extern void sftk_update_state(SFTKSlot *slot, SFTKSession *session);
 extern void sftk_update_all_states(SFTKSlot *slot);
 extern void sftk_FreeContext(SFTKSessionContext *context);
 extern void sftk_InitFreeLists(void);
 extern void sftk_CleanupFreeLists(void);
 
--- a/lib/softoken/pkcs11u.c
+++ b/lib/softoken/pkcs11u.c
@@ -1808,17 +1808,16 @@ sftk_NewSession(CK_SLOT_ID slotID, CK_NO
     if (slot == NULL)
         return NULL;
 
     session = (SFTKSession *)PORT_Alloc(sizeof(SFTKSession));
     if (session == NULL)
         return NULL;
 
     session->next = session->prev = NULL;
-    session->refCount = 1;
     session->enc_context = NULL;
     session->hash_context = NULL;
     session->sign_context = NULL;
     session->search = NULL;
     session->objectIDCount = 1;
     session->objectLock = PZ_NewLock(nssILockObject);
     if (session->objectLock == NULL) {
         PORT_Free(session);
@@ -1832,21 +1831,20 @@ sftk_NewSession(CK_SLOT_ID slotID, CK_NO
     session->info.flags = flags;
     session->info.slotID = slotID;
     session->info.ulDeviceError = 0;
     sftk_update_state(slot, session);
     return session;
 }
 
 /* free all the data associated with a session. */
-static void
+void
 sftk_DestroySession(SFTKSession *session)
 {
     SFTKObjectList *op, *next;
-    PORT_Assert(session->refCount == 0);
 
     /* clean out the attributes */
     /* since no one is referencing us, it's safe to walk the chain
      * without a lock */
     for (op = session->objects[0]; op != NULL; op = next) {
         next = op->next;
         /* paranoia */
         op->next = op->prev = NULL;
@@ -1880,41 +1878,30 @@ sftk_SessionFromHandle(CK_SESSION_HANDLE
     PZLock *lock;
 
     if (!slot)
         return NULL;
     lock = SFTK_SESSION_LOCK(slot, handle);
 
     PZ_Lock(lock);
     sftkqueue_find(session, handle, slot->head, slot->sessHashSize);
-    if (session)
-        session->refCount++;
     PZ_Unlock(lock);
 
     return (session);
 }
 
 /*
- * release a reference to a session handle
+ * release a reference to a session handle. This method of using SFTKSessions
+ * is deprecated, but the pattern should be retained until a future effort
+ * to refactor all SFTKSession users at once is completed.
  */
 void
 sftk_FreeSession(SFTKSession *session)
 {
-    PRBool destroy = PR_FALSE;
-    SFTKSlot *slot = sftk_SlotFromSession(session);
-    PZLock *lock = SFTK_SESSION_LOCK(slot, session->handle);
-
-    PZ_Lock(lock);
-    if (session->refCount == 1)
-        destroy = PR_TRUE;
-    session->refCount--;
-    PZ_Unlock(lock);
-
-    if (destroy)
-        sftk_DestroySession(session);
+    return;
 }
 
 void
 sftk_addHandle(SFTKSearchResults *search, CK_OBJECT_HANDLE handle)
 {
     if (search->handles == NULL) {
         return;
     }