Fix leak of session objects on session close

SessionObjects were kept alive in allObjects as tombstones with
valid=false after their session closed, so every object ever created
lingered until C_Finalize, holding its Mutex*. Long running clients
saw unbounded memory growth dominated by mutex objects.

Add refcounting to OSObject and release the store's reference in
sessionClosed, allSessionsClosed, tokenLoggedOut, deleteObject,
clearStore, and ~SessionObjectStore. Also erase from allObjects in
these paths to avoid a double-free against the destructor.

Author: bukka

src/lib/object_store/OSObject.h

@@ -38,12 +38,27 @@
 #include "config.h"
 #include "OSAttribute.h"
 #include "cryptoki.h"
+#include <atomic>
 
 class OSObject
 {
 public:
-	// Destructor
-	virtual ~OSObject() { }
+	OSObject() : refCount(1) {}
+
+	virtual ~OSObject() = default;
+
+	void acquire() noexcept
+	{
+		refCount.fetch_add(1, std::memory_order_relaxed);
+	}
+
+	void release() noexcept
+	{
+		if (refCount.fetch_sub(1, std::memory_order_acq_rel) == 1)
+		{
+			delete this;
+		}
+	}
 
 	// Check if the specified attribute exists
 	virtual bool attributeExists(CK_ATTRIBUTE_TYPE type) = 0;
@@ -89,6 +104,8 @@ class OSObject
 	// Destroys the object (warning, any pointers to the object are no longer
 	// valid after this call because delete is called!)
 	virtual bool destroyObject() = 0;
+private:
+	std::atomic<int> refCount;
 };
 
 #endif // !_SOFTHSM_V2_OSOBJECT_H

src/lib/object_store/SessionObjectStore.cpp

@@ -59,12 +59,10 @@ SessionObjectStore::~SessionObjectStore()
 	std::set<SessionObject*> cleanUp = allObjects;
 	allObjects.clear();
 
-	for (std::set<SessionObject*>::iterator i = cleanUp.begin(); i != cleanUp.end(); i++)
+	for (auto* obj : cleanUp)
 	{
-		if ((*i) == NULL) continue;
-
-		SessionObject* that = *i;
-		delete that;
+		if (obj == NULL) continue;
+		obj->release();
 	}
 
 	MutexFactory::i()->recycleMutex(storeMutex);
@@ -129,90 +127,109 @@ SessionObject* SessionObjectStore::createObject(CK_SLOT_ID slotID, CK_SESSION_HA
 // Delete an object
 bool SessionObjectStore::deleteObject(SessionObject* object)
 {
-	MutexLocker lock(storeMutex);
-
-	if (objects.find(object) == objects.end())
+	SessionObject* toRelease = NULL;
 	{
-		ERROR_MSG("Cannot delete non-existent object 0x%08X", object);
-
-		return false;
+		MutexLocker lock(storeMutex);
+		if (objects.find(object) == objects.end())
+		{
+			ERROR_MSG("Cannot delete non-existent object 0x%08X", object);
+			return false;
+		}
+		object->invalidate();
+		objects.erase(object);
+		allObjects.erase(object);
+		toRelease = object;
+	}
+	if (toRelease)
+	{
+		toRelease->release();
 	}
-
-	// Invalidate the object instance
-	object->invalidate();
-
-	objects.erase(object);
-
 	return true;
 }
 
-// Indicate that a session has been closed; invalidates all objects
-// associated with this session
+// Indicate that a session has been closed - remove all its objects
 void SessionObjectStore::sessionClosed(CK_SESSION_HANDLE hSession)
 {
-	MutexLocker lock(storeMutex);
-
-	std::set<SessionObject*> checkObjects = objects;
-
-	for (std::set<SessionObject*>::iterator i = checkObjects.begin(); i != checkObjects.end(); i++)
+	std::set<SessionObject*> toRelease;
 	{
-        if ((*i)->removeOnSessionClose(hSession))
-		{
-			// Since the object remains in the allObjects set, any pointers to it will
-			// remain valid but it will no longer be returned when the set of objects
-			// is requested
-			objects.erase(*i);
+		MutexLocker lock(storeMutex);
+		for (auto it = objects.begin(); it != objects.end(); ) {
+			if ((*it)->removeOnSessionClose(hSession))
+			{
+				toRelease.insert(*it);
+				allObjects.erase(*it);
+				it = objects.erase(it);
+			}
+			else
+			{
+				++it;
+			}
 		}
-    }
+	}
+	for (auto* obj : toRelease)
+	{
+		obj->release();
+	}
 }
 
 void SessionObjectStore::allSessionsClosed(CK_SLOT_ID slotID)
 {
-	MutexLocker lock(storeMutex);
-
-	std::set<SessionObject*> checkObjects = objects;
-
-	for (std::set<SessionObject*>::iterator i = checkObjects.begin(); i != checkObjects.end(); i++)
-	{
-		if ((*i)->removeOnAllSessionsClose(slotID))
+		std::set<SessionObject*> toRelease;
 		{
-			// Since the object remains in the allObjects set, any pointers to it will
-			// remain valid but it will no longer be returned when the set of objects
-			// is requested
-			objects.erase(*i);
+			MutexLocker lock(storeMutex);
+			for (auto it = objects.begin(); it != objects.end(); ) {
+				if ((*it)->removeOnAllSessionsClose(slotID))
+				{
+					toRelease.insert(*it);
+					allObjects.erase(*it);
+					it = objects.erase(it);
+				}
+				else
+				{
+					++it;
+				}
+			}
 		}
+	for (auto* obj : toRelease)
+	{
+		obj->release();
 	}
 }
 
 void SessionObjectStore::tokenLoggedOut(CK_SLOT_ID slotID)
 {
-	MutexLocker lock(storeMutex);
-
-	std::set<SessionObject*> checkObjects = objects;
-
-	for (std::set<SessionObject*>::iterator i = checkObjects.begin(); i != checkObjects.end(); i++)
+	std::set<SessionObject*> toRelease;
 	{
-		if ((*i)->removeOnTokenLogout(slotID))
-		{
-			// Since the object remains in the allObjects set, any pointers to it will
-			// remain valid but it will no longer be returned when the set of objects
-			// is requested
-			objects.erase(*i);
+		MutexLocker lock(storeMutex);
+		for (auto it = objects.begin(); it != objects.end(); ) {
+			if ((*it)->removeOnTokenLogout(slotID)) {
+				toRelease.insert(*it);
+				allObjects.erase(*it);
+				it = objects.erase(it);
+			}
+			else
+			{
+				++it;
+			}
 		}
 	}
+	for (auto* obj : toRelease)
+	{
+		obj->release();
+	}
 }
 
 // Clear the whole store
 void SessionObjectStore::clearStore()
 {
-	MutexLocker lock(storeMutex);
-
-	objects.clear();
-	std::set<SessionObject*> clearObjects = allObjects;
-	allObjects.clear();
-
-	for (std::set<SessionObject*>::iterator i = clearObjects.begin(); i != clearObjects.end(); i++)
+	std::set<SessionObject*> clearObjects;
+	{
+		MutexLocker lock(storeMutex);
+		objects.clear();
+		clearObjects.swap(allObjects);
+	}
+	for (auto* obj : clearObjects)
 	{
-		delete *i;
+		obj->release();
 	}
 }

src/lib/object_store/test/SessionObjectStoreTests.cpp

@@ -181,115 +181,99 @@ void SessionObjectStoreTests::testMultiSession()
 	// Check that the store contains 3 objects
 	CPPUNIT_ASSERT(store->getObjectCount() == 3);
 
-	// Check that all three objects are distinct and present
+	// Check that all objects are distinct and present
 	std::set<OSObject*> objects;
-        store->getObjects(objects);
-	bool present1[3] = { false, false, false };
-
-	for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); i++)
+	store->getObjects(objects);
+	bool present[5] = { false, false, false, false, false };
+	for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); ++i)
 	{
-		ByteString retrievedId;
-
 		CPPUNIT_ASSERT((*i)->isValid());
 		CPPUNIT_ASSERT((*i)->attributeExists(CKA_ID));
-
 		CPPUNIT_ASSERT((*i)->getAttribute(CKA_ID).isByteStringAttribute());
-
-		for (int j = 0; j < 3; j++)
+		for (int j = 0; j < 5; ++j)
 		{
 			if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[j])
 			{
-				present1[j] = true;
+				present[j] = true;
 			}
 		}
 	}
+	CPPUNIT_ASSERT(present[0] && present[1] && present[2]);
+	CPPUNIT_ASSERT(!present[3] && !present[4]);
 
-	for (int j = 0; j < 3; j++)
-	{
-		CPPUNIT_ASSERT(present1[j] == true);
-	}
-
-	// Now indicate that the second session has been closed
+	// Close session 2 -> obj2 is destroyed. Its pointer is now stale.
 	store->sessionClosed(2);
+	obj2 = NULL;   // hygiene: prevent accidental use-after-free in the test
 
-	// Verify that it was indeed removed
 	CPPUNIT_ASSERT(store->getObjectCount() == 2);
 
-        objects.clear();
+	objects.clear();
 	store->getObjects(objects);
-	bool present3[2] = { false, false };
-
-	for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); i++)
+	memset(present, 0, sizeof(present));
+	for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); ++i)
 	{
-		ByteString retrievedId;
-
-		CPPUNIT_ASSERT((*i)->isValid());
-		CPPUNIT_ASSERT((*i)->attributeExists(CKA_ID));
-
-		CPPUNIT_ASSERT((*i)->getAttribute(CKA_ID).isByteStringAttribute());
-
-		if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[0])
+		for (int j = 0; j < 5; ++j)
 		{
-			present3[0] = true;
-		}
-		if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[2])
-		{
-			present3[1] = true;
+			if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[j])
+			{
+				present[j] = true;
+			}
 		}
 	}
+	CPPUNIT_ASSERT(present[0] && !present[1] && present[2]);
 
-	for (int j = 0; j < 2; j++)
-	{
-		CPPUNIT_ASSERT(present3[j] == true);
-	}
-
-	// Create two more objects for session 7
+	// Create two more in session 7, with distinct IDs so we can identify them by attribute
 	SessionObject* obj4 = store->createObject(1, 7);
 	CPPUNIT_ASSERT(obj4 != NULL);
+	obj4->setAttribute(CKA_ID, idAtt[3]);
 	SessionObject* obj5 = store->createObject(1, 7);
 	CPPUNIT_ASSERT(obj5 != NULL);
+	obj5->setAttribute(CKA_ID, idAtt[4]);
 
 	CPPUNIT_ASSERT(store->getObjectCount() == 4);
 
-	// Close session 1
+	// Close session 1 -> obj1 destroyed.
 	store->sessionClosed(1);
+	obj1 = NULL;
 
 	CPPUNIT_ASSERT(store->getObjectCount() == 3);
 
-        objects.clear();
+	objects.clear();
 	store->getObjects(objects);
+	memset(present, 0, sizeof(present));
+	for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); ++i)
+	{
+		for (int j = 0; j < 5; ++j)
+		{
+			if ((*i)->getAttribute(CKA_ID).getByteStringValue() == id[j])
+			{
+				present[j] = true;
+			}
+		}
+	}
+	CPPUNIT_ASSERT(!present[0] && !present[1]);
+	CPPUNIT_ASSERT(present[2] && present[3] && present[4]);
 
-	CPPUNIT_ASSERT(objects.find(obj1) == objects.end());
-	CPPUNIT_ASSERT(objects.find(obj2) == objects.end());
-	CPPUNIT_ASSERT(objects.find(obj3) != objects.end());
-	CPPUNIT_ASSERT(objects.find(obj4) != objects.end());
-	CPPUNIT_ASSERT(objects.find(obj5) != objects.end());
-
-	CPPUNIT_ASSERT(!obj1->isValid());
-	CPPUNIT_ASSERT(!obj2->isValid());
+	// The objects whose sessions are still open should still be usable
 	CPPUNIT_ASSERT(obj3->isValid());
 	CPPUNIT_ASSERT(obj4->isValid());
 	CPPUNIT_ASSERT(obj5->isValid());
 
-	// Close session 7
+	// Close session 7 -> obj4, obj5 destroyed
 	store->sessionClosed(7);
+	obj4 = NULL;
+	obj5 = NULL;
 
 	CPPUNIT_ASSERT(store->getObjectCount() == 1);
 
-        objects.clear();
+	objects.clear();
 	store->getObjects(objects);
+	CPPUNIT_ASSERT(objects.size() == 1);
 
-	CPPUNIT_ASSERT(objects.find(obj1) == objects.end());
-	CPPUNIT_ASSERT(objects.find(obj2) == objects.end());
-	CPPUNIT_ASSERT(objects.find(obj3) != objects.end());
-	CPPUNIT_ASSERT(objects.find(obj4) == objects.end());
-	CPPUNIT_ASSERT(objects.find(obj5) == objects.end());
-
-	CPPUNIT_ASSERT(!obj1->isValid());
-	CPPUNIT_ASSERT(!obj2->isValid());
+	std::set<OSObject*>::iterator it = objects.begin();
+	CPPUNIT_ASSERT((*it)->isValid());
+	CPPUNIT_ASSERT((*it)->getAttribute(CKA_ID).getByteStringValue() == id[2]);
 	CPPUNIT_ASSERT(obj3->isValid());
-	CPPUNIT_ASSERT(!obj4->isValid());
-	CPPUNIT_ASSERT(!obj5->isValid());
 
 	delete store;
 }