Fix leak of session objects on session close#875
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 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. ChangesIntrusive Reference Counting in Object Store
Handle Manager: typed pointers and safe lifetime
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
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (3)
src/lib/object_store/OSObject.hsrc/lib/object_store/SessionObjectStore.cppsrc/lib/object_store/test/SessionObjectStoreTests.cpp
| 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]) | ||
| { |
There was a problem hiding this comment.
| 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]) | |
| { |
119b27b to
0143301
Compare
There was a problem hiding this comment.
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 winSame leak issue in testCreateDeleteObjects.
Same pattern:
getObjects()acquires references at lines 83 and 128, butobjects.clear()at line 127 anddelete testStoreat 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 valueInconsistent 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
📒 Files selected for processing (10)
src/lib/SoftHSM.cppsrc/lib/handle_mgr/Handle.cppsrc/lib/handle_mgr/Handle.hsrc/lib/handle_mgr/HandleManager.cppsrc/lib/handle_mgr/HandleManager.hsrc/lib/handle_mgr/Makefile.amsrc/lib/handle_mgr/test/HandleManagerTests.cppsrc/lib/object_store/OSObject.hsrc/lib/object_store/SessionObjectStore.cppsrc/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
865b73b to
726d740
Compare
There was a problem hiding this comment.
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 winDestructor accesses
objectsandallObjectswithout holdingstoreMutex.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 holdstoreMutexbefore 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 winRelease 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 beforedelete 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 valueFix 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
📒 Files selected for processing (10)
src/lib/SoftHSM.cppsrc/lib/handle_mgr/Handle.cppsrc/lib/handle_mgr/Handle.hsrc/lib/handle_mgr/HandleManager.cppsrc/lib/handle_mgr/HandleManager.hsrc/lib/handle_mgr/Makefile.amsrc/lib/handle_mgr/test/HandleManagerTests.cppsrc/lib/object_store/OSObject.hsrc/lib/object_store/SessionObjectStore.cppsrc/lib/object_store/test/SessionObjectStoreTests.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/object_store/SessionObjectStore.cpp (1)
174-196: 💤 Low valueInconsistent 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
📒 Files selected for processing (10)
src/lib/SoftHSM.cppsrc/lib/handle_mgr/Handle.cppsrc/lib/handle_mgr/Handle.hsrc/lib/handle_mgr/HandleManager.cppsrc/lib/handle_mgr/HandleManager.hsrc/lib/handle_mgr/Makefile.amsrc/lib/handle_mgr/test/HandleManagerTests.cppsrc/lib/object_store/OSObject.hsrc/lib/object_store/SessionObjectStore.cppsrc/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
726d740 to
a3b3c08
Compare
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.
a3b3c08 to
ff53bac
Compare
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
Refactor
Tests