From 251722477192a9a7db6d50530712d1f792a32c6b Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Mon, 8 May 2023 10:49:41 -0400
Subject: [PATCH] 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.
---
 src/app/OperationalSessionSetup.cpp | 18 ++++++++----------
 src/app/OperationalSessionSetup.h   | 14 +++-----------
 2 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp
index cded2b4f404fb5..849490b40ab8c8 100644
--- a/src/app/OperationalSessionSetup.cpp
+++ b/src/app/OperationalSessionSetup.cpp
@@ -396,12 +396,18 @@ void OperationalSessionSetup::OnSessionEstablished(const SessionHandle & session
                    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::CleanupCASEClient()
     }
 }
 
-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 5dbdff6ebb2f9c..c6208a00f4ba35 100644
--- a/src/app/OperationalSessionSetup.h
+++ b/src/app/OperationalSessionSetup.h
@@ -152,16 +152,13 @@ typedef void (*OnDeviceConnectionRetry)(void * context, const ScopedNodeId & pee
  * 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 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
     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 @@ class DLL_EXPORT OperationalSessionSetup : public SessionDelegate,
 
     Transport::PeerAddress mDeviceAddress = Transport::PeerAddress::UDP(Inet::IPAddress::Any);
 
-    SessionHolderWithDelegate mSecureSession;
+    SessionHolder mSecureSession;
 
     Callback::CallbackDeque mConnectionSuccess;
     Callback::CallbackDeque mConnectionFailure;