Skip to content

Fix leak of session objects on session close#875

Open
bukka wants to merge 1 commit into
softhsm:mainfrom
bukka:session-close-object-free
Open

Fix leak of session objects on session close#875
bukka wants to merge 1 commit into
softhsm:mainfrom
bukka:session-close-object-free

Conversation

@bukka
Copy link
Copy Markdown
Member

@bukka bukka commented May 17, 2026

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 have SessionObjectStore, HandleManager, and C_FindObjectsInit acquire/release across the boundaries where session-object pointers escape the store mutex. Releases happen outside the relevant lock to avoid widening critical sections. Also erase from allObjects in the store cleanup paths to avoid a double-free against the destructor.

Summary by CodeRabbit

  • Bug Fixes

    • Improved thread-safety and stability around object lifetime and session closure to prevent use-after-free and avoid releasing objects while locks are held.
    • More reliable object enumeration during searches and session/token operations for consistent results.
  • Refactor

    • Introduced explicit acquire/release ownership semantics and updated handle management to use typed references for safer cleanup and reduced races.
  • Tests

    • Updated tests to validate presence, validity, and lifecycle behavior under the new ownership model.

Review Change Stack

@bukka bukka requested a review from a team as a code owner May 17, 2026 17:19
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds intrusive atomic reference counting to OSObject; updates SessionObjectStore and HandleManager to acquire/release and defer releases until after unlocking; separates session/token collection in SoftHSM find; and updates tests to the new lifetime semantics.

Changes

Intrusive Reference Counting in Object Store

Layer / File(s) Summary
Atomic reference counting in OSObject
src/lib/object_store/OSObject.h
OSObject adds a default constructor initializing refCount to 1, public acquire() and release() methods manipulating a std::atomic<int>, and a defaulted virtual destructor; release() deletes this when count reaches zero.
Lock-deferred release in SessionObjectStore
src/lib/object_store/SessionObjectStore.cpp
Destructor, deleteObject(), sessionClosed(), allSessionsClosed(), tokenLoggedOut(), and clearStore() collect objects under storeMutex, erase them from containers, and call release() after unlocking; getObjects() now calls acquire() for returned objects.
Find initialization separates session/token collections
src/lib/SoftHSM.cpp
C_FindObjectsInit collects token and session objects into separate sets, uses an RAII releaser for session objects, then merges both sets into allObjects for existing matching and handle creation logic.
SessionObjectStore tests updated for refcounting
src/lib/object_store/test/SessionObjectStoreTests.cpp
testMultiSession now verifies object presence by CKA_ID attribute presence arrays and phases aligned with session closures instead of pointer-membership checks.

Handle Manager: typed pointers and safe lifetime

Layer / File(s) Summary
Handle/HandleManager API and headers
src/lib/handle_mgr/Handle.h, src/lib/handle_mgr/HandleManager.h, src/lib/handle_mgr/Makefile.am, src/lib/handle_mgr/Handle.cpp
Headers include OSObject.h and Session.h; public signatures and member types move from CK_VOID_PTR to typed OSObject */Session *; Makefile include path updated; Handle constructors initialize object/session to nullptr.
HandleManager implementation: acquire/release semantics
src/lib/handle_mgr/HandleManager.cpp
HandleManager stores typed pointers, calls object->acquire() when issuing session-object handles, and defers object->release() until after unlocking in destructor and all cleanup paths (destroyObject, sessionClosed, allSessionsClosed, tokenLoggedOut), also returning nullptr/CK_INVALID_HANDLE for missing lookups.
HandleManager tests with TestOSObject
src/lib/handle_mgr/test/HandleManagerTests.cpp
Adds TestOSObject subclass and refactors tests to use concrete OSObject* and Session* values, asserting handle stability, slot migration denial, logout/session-closure behavior, and explicitly releasing created OSObjects.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant SessionObjectStore
  participant storeMutex
  participant toRelease_set
  participant OSObject

  Caller->>SessionObjectStore: request removal/clear or getObjects()
  SessionObjectStore->>storeMutex: lock
  storeMutex-->>SessionObjectStore: locked
  SessionObjectStore->>SessionObjectStore: collect matching OSObject*
  SessionObjectStore->>toRelease_set: move collected OSObject*
  SessionObjectStore->>storeMutex: unlock
  storeMutex-->>SessionObjectStore: unlocked
  SessionObjectStore->>OSObject: call release() for each in toRelease_set
  OSObject->>OSObject: decrement refCount / delete when zero
Loading

🎯 4 (Complex) | ⏱️ ~45 minutes

  • softhsm/SoftHSMv2#851: Related changes touching HandleManager cleanup paths and object lifecycle handling; overlaps in session/token handle removal logic.

"I'm a rabbit in a patch of code,
I count the refs as they explode,
Locks unlock, releases spring,
Objects fall on gentle wing,
Tests dance — the store hums on the road."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 24.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix leak of session objects on session close' clearly and concisely summarizes the main change: addressing a memory leak where session objects persisted after their session closed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/object_store/SessionObjectStore.cpp`:
- Around line 143-146: getObjects(...) currently returns raw OSObject* pointers
copied under storeMutex but does not call acquire() on them, so subsequent
release() calls (e.g., in the code paths that drop references) can make callers
receive dangling pointers; fix by transferring real ownership: inside
getObjects() while holding storeMutex call acquire() on each OSObject* you plan
to return (or instead move returned pointers into a RAII holder that keeps an
owning reference) and require callers to call release() when done, and remove
the premature toRelease->release() patterns (or defer them until after callers
have released) so functions like getObjects(), any code paths that use
storeMutex, and the release() invocations near the referenced blocks all
preserve at least one live reference to each object until readers finish.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e0fb8be7-63de-48ab-b928-81ae28c6e293

📥 Commits

Reviewing files that changed from the base of the PR and between 02cebd5 and 119b27b.

📒 Files selected for processing (3)
  • src/lib/object_store/OSObject.h
  • src/lib/object_store/SessionObjectStore.cpp
  • src/lib/object_store/test/SessionObjectStoreTests.cpp

Comment thread src/lib/object_store/SessionObjectStore.cpp
Comment on lines +244 to +249
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])
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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])
{
for (auto* obj : objects)
{
for (int j = 0; j < 5; ++j)
{
if (obj->getAttribute(CKA_ID).getByteStringValue() == id[j])
{

@bukka bukka force-pushed the session-close-object-free branch from 119b27b to 0143301 Compare May 23, 2026 16:36
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/object_store/test/SessionObjectStoreTests.cpp (1)

82-153: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Same leak issue in testCreateDeleteObjects.

Same pattern: getObjects() acquires references at lines 83 and 128, but objects.clear() at line 127 and delete testStore at line 153 happen without releasing the acquired objects.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/test/SessionObjectStoreTests.cpp` around lines 82 - 153,
getObjects() returns owned OSObject* references that must be released; fix
testCreateDeleteObjects by iterating the 'objects' set and releasing each
OSObject* (call the appropriate destructor/release/unref method for OSObject)
before calling objects.clear() and again ensure any remaining references are
released before delete testStore; locate the getObjects(...) usage and replace
the two spots around objects.clear() and before delete testStore with a loop
that releases each pointer (cast as needed, e.g., SessionObject*) then clears
the set.
🧹 Nitpick comments (1)
src/lib/object_store/SessionObjectStore.cpp (1)

174-196: 💤 Low value

Inconsistent indentation inside function body.

Lines 176–191 have an extra level of indentation compared to the surrounding code and the other similar functions in this file.

🔧 Suggested fix
 void SessionObjectStore::allSessionsClosed(CK_SLOT_ID slotID)
 {
-		std::set<SessionObject*> toRelease;
+	std::set<SessionObject*> toRelease;
+	{
+		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;
+			}
+		}
+	}
-		{
-			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)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/SessionObjectStore.cpp` around lines 174 - 196, The
function SessionObjectStore::allSessionsClosed has an extra indentation level
for the inner block and loops; fix by normalizing indentation to match the rest
of the file: remove the extra leading tabs/spaces so the MutexLocker
lock(storeMutex) block, the for loop iterating objects (and its else branch),
the closing brace, and the final for (auto* obj : toRelease) loop align with
surrounding function scope; keep use of variables and calls unchanged
(toRelease, storeMutex, objects, allObjects, removeOnAllSessionsClose,
obj->release).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/object_store/test/SessionObjectStoreTests.cpp`:
- Around line 184-278: The test leaks references because
store->getObjects(objects) acquires each OSObject (via acquire()) but the test
clears the set or deletes the store without calling release(); before each
objects.clear() (the calls after the first population and before the subsequent
getObjects calls) and before delete store you must iterate the
std::set<OSObject*> (objects) and call release() on each pointer (or use an RAII
wrapper) to balance the acquire, e.g., add a loop that calls (*it)->release()
for each element prior to objects.clear() and again to release the final set
before delete store so all acquired references from getObjects() are freed.

---

Outside diff comments:
In `@src/lib/object_store/test/SessionObjectStoreTests.cpp`:
- Around line 82-153: getObjects() returns owned OSObject* references that must
be released; fix testCreateDeleteObjects by iterating the 'objects' set and
releasing each OSObject* (call the appropriate destructor/release/unref method
for OSObject) before calling objects.clear() and again ensure any remaining
references are released before delete testStore; locate the getObjects(...)
usage and replace the two spots around objects.clear() and before delete
testStore with a loop that releases each pointer (cast as needed, e.g.,
SessionObject*) then clears the set.

---

Nitpick comments:
In `@src/lib/object_store/SessionObjectStore.cpp`:
- Around line 174-196: The function SessionObjectStore::allSessionsClosed has an
extra indentation level for the inner block and loops; fix by normalizing
indentation to match the rest of the file: remove the extra leading tabs/spaces
so the MutexLocker lock(storeMutex) block, the for loop iterating objects (and
its else branch), the closing brace, and the final for (auto* obj : toRelease)
loop align with surrounding function scope; keep use of variables and calls
unchanged (toRelease, storeMutex, objects, allObjects, removeOnAllSessionsClose,
obj->release).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e151609b-0488-4413-a17f-a4739904dac4

📥 Commits

Reviewing files that changed from the base of the PR and between 119b27b and 0143301.

📒 Files selected for processing (10)
  • src/lib/SoftHSM.cpp
  • src/lib/handle_mgr/Handle.cpp
  • src/lib/handle_mgr/Handle.h
  • src/lib/handle_mgr/HandleManager.cpp
  • src/lib/handle_mgr/HandleManager.h
  • src/lib/handle_mgr/Makefile.am
  • src/lib/handle_mgr/test/HandleManagerTests.cpp
  • src/lib/object_store/OSObject.h
  • src/lib/object_store/SessionObjectStore.cpp
  • src/lib/object_store/test/SessionObjectStoreTests.cpp
✅ Files skipped from review due to trivial changes (2)
  • src/lib/handle_mgr/Handle.cpp
  • src/lib/handle_mgr/Makefile.am

Comment thread src/lib/object_store/test/SessionObjectStoreTests.cpp
@bukka bukka force-pushed the session-close-object-free branch 2 times, most recently from 865b73b to 726d740 Compare May 23, 2026 16:55
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/object_store/SessionObjectStore.cpp (1)

55-69: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Destructor accesses objects and allObjects without holding storeMutex.

If any other thread could still be calling getObjects(), sessionClosed(), etc., while this destructor runs, there is a data race on the containers. The other cleanup methods all hold storeMutex before touching these sets.

If the usage contract guarantees single-threaded destruction (no concurrent access during teardown), this is fine, but documenting that assumption or adding a lock for consistency would prevent subtle future bugs.

🛡️ Proposed fix to lock during destruction
 SessionObjectStore::~SessionObjectStore()
 {
-	// Clean up
-	objects.clear();
-	std::set<SessionObject*> cleanUp = allObjects;
-	allObjects.clear();
+	std::set<SessionObject*> cleanUp;
+	{
+		MutexLocker lock(storeMutex);
+		objects.clear();
+		cleanUp.swap(allObjects);
+	}
 
 	for (auto* obj : cleanUp)
 	{
 		if (obj == NULL) continue;
 		obj->release();
 	}
 
 	MutexFactory::i()->recycleMutex(storeMutex);
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/SessionObjectStore.cpp` around lines 55 - 69, The
destructor accesses the containers objects and allObjects without holding the
storeMutex, causing a potential data race with methods like getObjects() and
sessionClosed(); fix by acquiring storeMutex at the start of
SessionObjectStore::~SessionObjectStore (use the same mutex type/locking API
used elsewhere, e.g. a scoped lock/lock_guard for storeMutex), perform
objects.clear(), swap/move allObjects into a local set and iterate/releases
while still holding or immediately after moving under the lock, then release the
lock before calling MutexFactory::i()->recycleMutex(storeMutex) so you don't
recycle a locked mutex; ensure you reference the existing symbols
SessionObjectStore::~SessionObjectStore, objects, allObjects, storeMutex, and
MutexFactory::i()->recycleMutex in the change.
♻️ Duplicate comments (1)
src/lib/object_store/test/SessionObjectStoreTests.cpp (1)

210-212: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Release acquired object refs before clearing/reloading the set.

getObjects() acquires references, but at Line 210, Line 241, and Line 269 the set is cleared without releasing them first; the final fetched set is also not released before delete store (Line 278). This leaks refs in the test and can hide lifecycle regressions.

Suggested minimal fix
 	std::set<OSObject*> objects;
+	auto releaseObjects = [&objects]() {
+		for (std::set<OSObject*>::iterator i = objects.begin(); i != objects.end(); ++i)
+		{
+			(*i)->release();
+		}
+		objects.clear();
+	};
 	store->getObjects(objects);
@@
-	objects.clear();
+	releaseObjects();
 	store->getObjects(objects);
@@
-	objects.clear();
+	releaseObjects();
 	store->getObjects(objects);
@@
-	objects.clear();
+	releaseObjects();
 	store->getObjects(objects);
@@
+	releaseObjects();
 	delete store;

Also applies to: 241-243, 269-271

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/test/SessionObjectStoreTests.cpp` around lines 210 -
212, Before each objects.clear() and before delete store, iterate the 'objects'
vector returned by store->getObjects(objects) and release each acquired
reference using the object's release API (e.g., objects[i]->release() or
unref()) or the store's corresponding release method (e.g.,
store->releaseObject(objects[i])) so refs are dropped; update the spots around
objects.clear() (the occurrences after store->getObjects(objects) at the
locations shown) and before the final delete store to explicitly release all
entries and then clear the vector.
🧹 Nitpick comments (1)
src/lib/object_store/SessionObjectStore.cpp (1)

174-196: 💤 Low value

Fix inconsistent indentation.

Lines 176–191 have an extra indentation level compared to lines 192–195. This appears to be a merge or editing artifact.

🧹 Proposed fix
 void SessionObjectStore::allSessionsClosed(CK_SLOT_ID slotID)
 {
-		std::set<SessionObject*> toRelease;
-		{
-			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;
-				}
-			}
+	std::set<SessionObject*> toRelease;
+	{
+		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();
 	}
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/SessionObjectStore.cpp` around lines 174 - 196, Fix the
inconsistent indentation in SessionObjectStore::allSessionsClosed by aligning
the inner block and following loop to the same indentation level as the
surrounding function body; specifically normalize indentation for the scope that
locks storeMutex and the subsequent for-loop that calls obj->release(), ensuring
the blocks around variables toRelease, storeMutex, objects, allObjects,
removeOnAllSessionsClose, and release are consistently indented to match the
function's style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/handle_mgr/HandleManager.cpp`:
- Around line 243-244: sessionClosed currently calls allSessionsClosed(slotID,
true) while still holding handlesMutex which causes actual handle releases (the
toRelease loop in allSessionsClosed) to run under the outer lock; change
allSessionsClosed to only collect handles that need release and either return
the collected toRelease set or accept an output/set reference parameter, and
update sessionClosed to call allSessionsClosed while locked but defer iterating
and releasing items until after it unlocks handlesMutex; ensure any early-return
paths and the skip-relock logic in allSessionsClosed are adjusted so no release
loop executes under the handlesMutex lock.

---

Outside diff comments:
In `@src/lib/object_store/SessionObjectStore.cpp`:
- Around line 55-69: The destructor accesses the containers objects and
allObjects without holding the storeMutex, causing a potential data race with
methods like getObjects() and sessionClosed(); fix by acquiring storeMutex at
the start of SessionObjectStore::~SessionObjectStore (use the same mutex
type/locking API used elsewhere, e.g. a scoped lock/lock_guard for storeMutex),
perform objects.clear(), swap/move allObjects into a local set and
iterate/releases while still holding or immediately after moving under the lock,
then release the lock before calling MutexFactory::i()->recycleMutex(storeMutex)
so you don't recycle a locked mutex; ensure you reference the existing symbols
SessionObjectStore::~SessionObjectStore, objects, allObjects, storeMutex, and
MutexFactory::i()->recycleMutex in the change.

---

Duplicate comments:
In `@src/lib/object_store/test/SessionObjectStoreTests.cpp`:
- Around line 210-212: Before each objects.clear() and before delete store,
iterate the 'objects' vector returned by store->getObjects(objects) and release
each acquired reference using the object's release API (e.g.,
objects[i]->release() or unref()) or the store's corresponding release method
(e.g., store->releaseObject(objects[i])) so refs are dropped; update the spots
around objects.clear() (the occurrences after store->getObjects(objects) at the
locations shown) and before the final delete store to explicitly release all
entries and then clear the vector.

---

Nitpick comments:
In `@src/lib/object_store/SessionObjectStore.cpp`:
- Around line 174-196: Fix the inconsistent indentation in
SessionObjectStore::allSessionsClosed by aligning the inner block and following
loop to the same indentation level as the surrounding function body;
specifically normalize indentation for the scope that locks storeMutex and the
subsequent for-loop that calls obj->release(), ensuring the blocks around
variables toRelease, storeMutex, objects, allObjects, removeOnAllSessionsClose,
and release are consistently indented to match the function's style.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f6d7100b-8bd0-4e58-a697-e946922e4639

📥 Commits

Reviewing files that changed from the base of the PR and between 0143301 and 865b73b.

📒 Files selected for processing (10)
  • src/lib/SoftHSM.cpp
  • src/lib/handle_mgr/Handle.cpp
  • src/lib/handle_mgr/Handle.h
  • src/lib/handle_mgr/HandleManager.cpp
  • src/lib/handle_mgr/HandleManager.h
  • src/lib/handle_mgr/Makefile.am
  • src/lib/handle_mgr/test/HandleManagerTests.cpp
  • src/lib/object_store/OSObject.h
  • src/lib/object_store/SessionObjectStore.cpp
  • src/lib/object_store/test/SessionObjectStoreTests.cpp

Comment thread src/lib/handle_mgr/HandleManager.cpp
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/object_store/SessionObjectStore.cpp (1)

174-196: 💤 Low value

Inconsistent indentation.

The function body has extra leading whitespace on lines 176-191 compared to the rest of the file and the release loop at lines 192-195.

🔧 Suggested fix
 void SessionObjectStore::allSessionsClosed(CK_SLOT_ID slotID)
 {
-		std::set<SessionObject*> toRelease;
-		{
-			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;
-				}
-			}
-		}
+	std::set<SessionObject*> toRelease;
+	{
+		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();
 	}
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/object_store/SessionObjectStore.cpp` around lines 174 - 196, The
function SessionObjectStore::allSessionsClosed has inconsistent leading
indentation inside the scope that builds toRelease (extra whitespace before the
block and its inner lines); fix it by normalizing the indentation to match the
surrounding file style so the MutexLocker block and its for-loop align with the
function's opening brace and the subsequent release loop, keeping the same
indentation for variables toRelease, storeMutex, objects, allObjects and calls
to removeOnAllSessionsClose and release.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/object_store/SessionObjectStore.cpp`:
- Around line 174-196: The function SessionObjectStore::allSessionsClosed has
inconsistent leading indentation inside the scope that builds toRelease (extra
whitespace before the block and its inner lines); fix it by normalizing the
indentation to match the surrounding file style so the MutexLocker block and its
for-loop align with the function's opening brace and the subsequent release
loop, keeping the same indentation for variables toRelease, storeMutex, objects,
allObjects and calls to removeOnAllSessionsClose and release.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ac05cb3f-98e2-4745-916d-2ba1d97bc209

📥 Commits

Reviewing files that changed from the base of the PR and between 865b73b and 726d740.

📒 Files selected for processing (10)
  • src/lib/SoftHSM.cpp
  • src/lib/handle_mgr/Handle.cpp
  • src/lib/handle_mgr/Handle.h
  • src/lib/handle_mgr/HandleManager.cpp
  • src/lib/handle_mgr/HandleManager.h
  • src/lib/handle_mgr/Makefile.am
  • src/lib/handle_mgr/test/HandleManagerTests.cpp
  • src/lib/object_store/OSObject.h
  • src/lib/object_store/SessionObjectStore.cpp
  • src/lib/object_store/test/SessionObjectStoreTests.cpp
✅ Files skipped from review due to trivial changes (2)
  • src/lib/handle_mgr/Makefile.am
  • src/lib/handle_mgr/Handle.cpp

@bukka bukka force-pushed the session-close-object-free branch from 726d740 to a3b3c08 Compare May 23, 2026 17:07
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 have SessionObjectStore, HandleManager,
and C_FindObjectsInit acquire/release across the boundaries where
session-object pointers escape the store mutex. Releases happen outside
the relevant lock to avoid widening critical sections. Also erase from
allObjects in the store cleanup paths to avoid a double-free against
the destructor.
@bukka bukka force-pushed the session-close-object-free branch from a3b3c08 to ff53bac Compare May 23, 2026 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants