project-chip
Device leaks secure session allocated in OnSessionEstablished
The session allocated at
https://github.com/project-chip/connectedhomeip/blob/e175721458a56423c73872bc329f82fdb6482672/src/controller/CHIPDevice.cpp#L570-L571
is not freed by any of the following APIs
- Device::Reset
- DeviceController::ReleaseDevice
- DeviceCommissioner::UnpairDevice
The only API that frees it is
- Device::CloseSession
After pairing and unpairing 16 times the supply of PeerConnectionStates will be exhausted and further connections fail with CHIP_ERROR_NO_MEMORY (the limit is given by CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE).
It looks like there are other paths that could result in leaks even if this is fixed, e.g. SendMessage resets mState to ConnectionState::NotConnected without closing so if LoadSecureSessionParameters fails it would leak.
@andreilitvin @pan-apple @kghost @bzbarsky-apple Do you understand the intent here?
Also, it seems confusing that "handle" implies ownership in PacketBufferHandle but not with "SecureSessionHandle". We have some quite error prone patterns here..
It looks like that no one have taken the consideration of the situation that session can be closed.
The SecureSessionHandle is used broadly to grab a reference to a session, when the session is closed the handle become a dangling handle. It will break all running exchanges and applications which is holding the dangling handle.
There is part of the reason why I wrote Channel (src/channel), it introduced a ChannelHandle which behaves like a SessionHandle, but with extra notification using ChannelDelegate about the session closed event. It will abort all running exchanges , and notify all applications which is bond to the session, that the underlying session has been closed.
It looks like that no one have taken the consideration of the situation that session can be closed.
The
SecureSessionHandleis used broadly to grab a reference to a session, when the session is closed the handle become a dangling handle. It will break all running exchanges and applications which is holding the dangling handle.There is part of the reason why I wrote Channel (src/channel), it introduced a
ChannelHandlewhich behaves like aSessionHandle, but with extra notification usingChannelDelegateabout the session closed event. It will abort all running exchanges , and notify all applications which is bond to the session, that the underlying session has been closed.
Are we going to move to the channels?
It looks like that no one have taken the consideration of the situation that session can be closed.
The
SecureSessionHandleis used broadly to grab a reference to a session, when the session is closed the handle become a dangling handle. It will break all running exchanges and applications which is holding the dangling handle.There is part of the reason why I wrote Channel (src/channel), it introduced a
ChannelHandlewhich behaves like aSessionHandle, but with extra notification usingChannelDelegateabout the session closed event. It will abort all running exchanges , and notify all applications which is bond to the session, that the underlying session has been closed.
@Damian-Nordic implemented a manual close API and hooks for python.. but I do not think he realized that release was also not closing it.
Are we going to move to the channels?
I want to have a discussion with Boris and Pankaj about how to enable it, we are hitting more and more problems which can be solved by the channels.
I believe there are other leaks, e.g. in DeviceCommissioner. SecureSessionHandle first arrives via OnNewConnection but there are a number of instances where that function returns early without storing its argument. If we need to manually call ExpirePairing on every new handle then these are also leaks.
@pan-apple You are the author of these APIs. Can you please describe the design intent here?
Marking this for secondary because I'm concerned this could lead to crashes. Also because this may no longer have an owner if Pankaj is no longer on matter sdk.
This is still an issue, but the session eviction work @mrjerryjohns is doing should address it...
Issue Scrub: Assigning to @mspang to confirm if this is still an issue or not.