Remove the OnSessionReleased callback from OperationalSessionSetup. (#26395)
Since OperationalSessionSetup is ephemeral, it only holds on to a session while
it's notifying its listeners, after which it will delete itself.
Right now it was deleting itself from OnSessionReleased, but that means it could
end up with a double-delete... and also, it's already notifying listeners if it
has a session, so there is no point, or ability, to notify them again on session
release.
The changes here:
1. Take out the OnSessionReleased that couldn't do anything except lead to
use-after-free.
2. Fix a bug on OnSessionEstablished where if we got a session that's not usable
we leaked and left our listeners hanging instead of just notifying our
listeners with error.
diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp
index cded2b4..849490b 100644
--- a/src/app/OperationalSessionSetup.cpp
+++ b/src/app/OperationalSessionSetup.cpp
@@ -396,12 +396,18 @@
ChipLogError(Discovery, "OnSessionEstablished was called while we were not connecting"));
if (!mSecureSession.Grab(session))
- return; // Got an invalid session, do not change any state
+ {
+ // Got an invalid session, just dispatch an error. We have to do this
+ // so we don't leak.
+ DequeueConnectionCallbacks(CHIP_ERROR_INCORRECT_STATE);
+
+ // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
+ return;
+ }
MoveToState(State::SecureConnected);
DequeueConnectionCallbacks(CHIP_NO_ERROR);
- // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks.
}
void OperationalSessionSetup::CleanupCASEClient()
@@ -413,14 +419,6 @@
}
}
-void OperationalSessionSetup::OnSessionReleased()
-{
- // This is unlikely to be called since within the same call that we get SessionHandle we
- // then call DequeueConnectionCallbacks which releases `this`. If this is called, and we
- // we have any callbacks we will just send an error.
- DequeueConnectionCallbacks(CHIP_ERROR_INCORRECT_STATE);
-}
-
OperationalSessionSetup::~OperationalSessionSetup()
{
if (mAddressLookupHandle.IsActive())
diff --git a/src/app/OperationalSessionSetup.h b/src/app/OperationalSessionSetup.h
index 5dbdff6..c6208a0 100644
--- a/src/app/OperationalSessionSetup.h
+++ b/src/app/OperationalSessionSetup.h
@@ -152,16 +152,13 @@
* It is possible to determine which of the two purposes the OperationalSessionSetup is for by calling
* IsForAddressUpdate().
*/
-class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
- public SessionEstablishmentDelegate,
- public AddressResolve::NodeListener
+class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate, public AddressResolve::NodeListener
{
public:
~OperationalSessionSetup() override;
OperationalSessionSetup(const CASEClientInitParams & params, CASEClientPoolDelegate * clientPool, ScopedNodeId peerId,
- OperationalSessionReleaseDelegate * releaseDelegate) :
- mSecureSession(*this)
+ OperationalSessionReleaseDelegate * releaseDelegate)
{
mInitParams = params;
if (params.Validate() != CHIP_NO_ERROR || clientPool == nullptr || releaseDelegate == nullptr)
@@ -201,11 +198,6 @@
void OnSessionEstablished(const SessionHandle & session) override;
void OnSessionEstablishmentError(CHIP_ERROR error) override;
- //////////// SessionDelegate Implementation ///////////////
-
- // Called when a connection is closing. The object releases all resources associated with the connection.
- void OnSessionReleased() override;
-
ScopedNodeId GetPeerId() const { return mPeerId; }
static Transport::PeerAddress ToPeerAddress(const Dnssd::ResolvedNodeData & nodeData)
@@ -268,7 +260,7 @@
Transport::PeerAddress mDeviceAddress = Transport::PeerAddress::UDP(Inet::IPAddress::Any);
- SessionHolderWithDelegate mSecureSession;
+ SessionHolder mSecureSession;
Callback::CallbackDeque mConnectionSuccess;
Callback::CallbackDeque mConnectionFailure;
