Fix OperationalSessionSetup notifying success with inactive sessions (#33822)

When a success callback marks the session defunct for some reason, other
succcess callbacks should not be called. Implement a GroupedCallbackList to
make this logic possible.

This specific solution is based on the assumption that we don't want to change
the OperationalSessionSetup API which takes success and two variants of failure
callbacks as separate, client-provided Callback objects, with all callbacks
being optional to provide. We also don't want to introduce additional dynamic
allocation within OperationalSessionSetup e.g. to allocate a struct holding the
related callbacks.

The GroupedCallbackList class makes use of the existing prev/next pointers
within the client-allocated Callback objects to capture the grouping
relationship between them.

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

Author: ksperling-apple

src/app/OperationalSessionSetup.cpp

@@ -325,35 +325,14 @@ void OperationalSessionSetup::EnqueueConnectionCallbacks(Callback::Callback<OnDe
                                                          Callback::Callback<OnDeviceConnectionFailure> * onFailure,
                                                          Callback::Callback<OnSetupFailure> * onSetupFailure)
 {
-    if (onConnection != nullptr)
-    {
-        mConnectionSuccess.Enqueue(onConnection->Cancel());
-    }
-
-    if (onFailure != nullptr)
-    {
-        mConnectionFailure.Enqueue(onFailure->Cancel());
-    }
-
-    if (onSetupFailure != nullptr)
-    {
-        mSetupFailure.Enqueue(onSetupFailure->Cancel());
-    }
+    mCallbacks.Enqueue(onConnection, onFailure, onSetupFailure);
 }
 
 void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error, SessionEstablishmentStage stage,
                                                          ReleaseBehavior releaseBehavior)
 {
-    Cancelable failureReady, setupFailureReady, successReady;
-
-    //
-    // Dequeue both failure and success callback lists into temporary stack args before invoking either of them.
-    // We do this since we may not have a valid 'this' pointer anymore upon invoking any of those callbacks
-    // since the callee may destroy this object as part of that callback.
-    //
-    mConnectionFailure.DequeueAll(failureReady);
-    mSetupFailure.DequeueAll(setupFailureReady);
-    mConnectionSuccess.DequeueAll(successReady);
+    // We expect that we only have callbacks if we are not performing just address update.
+    VerifyOrDie(!mPerformingAddressUpdate || mCallbacks.IsEmpty());
 
 #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
     // Clear out mConnectionRetry, so that those cancelables are not holding
@@ -365,7 +344,8 @@ void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error, Sessi
 #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
 
     // Gather up state we will need for our notifications.
-    bool performingAddressUpdate                  = mPerformingAddressUpdate;
+    SuccessFailureCallbackList readyCallbacks;
+    readyCallbacks.EnqueueTakeAll(mCallbacks);
     auto * exchangeMgr                            = mInitParams.exchangeMgr;
     Optional<SessionHandle> optionalSessionHandle = mSecureSession.Get();
     ScopedNodeId peerId                           = mPeerId;
@@ -383,71 +363,57 @@ void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error, Sessi
     }
 
     // DO NOT touch any members of this object after this point.  It's dead.
-
-    NotifyConnectionCallbacks(failureReady, setupFailureReady, successReady, error, stage, peerId, performingAddressUpdate,
-                              exchangeMgr, optionalSessionHandle, requestedBusyDelay);
+    NotifyConnectionCallbacks(readyCallbacks, error, stage, peerId, exchangeMgr, optionalSessionHandle, requestedBusyDelay);
 }
 
-void OperationalSessionSetup::NotifyConnectionCallbacks(Cancelable & failureReady, Cancelable & setupFailureReady,
-                                                        Cancelable & successReady, CHIP_ERROR error,
+void OperationalSessionSetup::NotifyConnectionCallbacks(SuccessFailureCallbackList & ready, CHIP_ERROR error,
                                                         SessionEstablishmentStage stage, const ScopedNodeId & peerId,
-                                                        bool performingAddressUpdate, Messaging::ExchangeManager * exchangeMgr,
+                                                        Messaging::ExchangeManager * exchangeMgr,
                                                         const Optional<SessionHandle> & optionalSessionHandle,
                                                         System::Clock::Milliseconds16 requestedBusyDelay)
 {
-    //
-    // If we encountered no error, go ahead and call all success callbacks. Otherwise,
-    // call the failure callbacks.
-    //
-    while (failureReady.mNext != &failureReady)
+    Callback::Callback<OnDeviceConnected> * onConnected;
+    Callback::Callback<OnDeviceConnectionFailure> * onConnectionFailure;
+    Callback::Callback<OnSetupFailure> * onSetupFailure;
+    while (ready.Take(onConnected, onConnectionFailure, onSetupFailure))
     {
-        // We expect that we only have callbacks if we are not performing just address update.
-        VerifyOrDie(!performingAddressUpdate);
-        Callback::Callback<OnDeviceConnectionFailure> * cb =
-            Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(failureReady.mNext);
-
-        cb->Cancel();
-
-        if (error != CHIP_NO_ERROR)
+        if (error == CHIP_NO_ERROR)
         {
-            cb->mCall(cb->mContext, peerId, error);
+            VerifyOrDie(exchangeMgr);
+            VerifyOrDie(optionalSessionHandle.Value()->AsSecureSession()->IsActiveSession());
+            if (onConnected != nullptr)
+            {
+                onConnected->mCall(onConnected->mContext, *exchangeMgr, optionalSessionHandle.Value());
+
+                // That sucessful call might have made the session inactive.  If it did, then we should
+                // not call any more success callbacks, since we do not in fact have an active session
+                // for them, and if they try to put the session in a holder that will fail, and then
+                // trying to use the holder as if it has a session will crash.
+                if (!optionalSessionHandle.Value()->AsSecureSession()->IsActiveSession())
+                {
+                    ChipLogError(Discovery, "Success callback for connection to " ChipLogFormatScopedNodeId " tore down session",
+                                 ChipLogValueScopedNodeId(peerId));
+                    error = CHIP_ERROR_CONNECTION_ABORTED;
+                }
+            }
         }
-    }
-
-    while (setupFailureReady.mNext != &setupFailureReady)
-    {
-        // We expect that we only have callbacks if we are not performing just address update.
-        VerifyOrDie(!performingAddressUpdate);
-        Callback::Callback<OnSetupFailure> * cb = Callback::Callback<OnSetupFailure>::FromCancelable(setupFailureReady.mNext);
-
-        cb->Cancel();
-
-        if (error != CHIP_NO_ERROR)
+        else // error
         {
-            // Initialize the ConnnectionFailureInfo object
-            ConnnectionFailureInfo failureInfo(peerId, error, stage);
-#if CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP
-            if (error == CHIP_ERROR_BUSY)
+            if (onConnectionFailure != nullptr)
             {
-                failureInfo.requestedBusyDelay.Emplace(requestedBusyDelay);
+                onConnectionFailure->mCall(onConnectionFailure->mContext, peerId, error);
             }
+            if (onSetupFailure != nullptr)
+            {
+                ConnnectionFailureInfo failureInfo(peerId, error, stage);
+#if CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP
+                if (error == CHIP_ERROR_BUSY)
+                {
+                    failureInfo.requestedBusyDelay.Emplace(requestedBusyDelay);
+                }
 #endif // CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP
-            cb->mCall(cb->mContext, failureInfo);
-        }
-    }
-
-    while (successReady.mNext != &successReady)
-    {
-        // We expect that we only have callbacks if we are not performing just address update.
-        VerifyOrDie(!performingAddressUpdate);
-        Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(successReady.mNext);
-
-        cb->Cancel();
-        if (error == CHIP_NO_ERROR)
-        {
-            VerifyOrDie(exchangeMgr);
-            // We know that we for sure have the SessionHandle in the successful case.
-            cb->mCall(cb->mContext, *exchangeMgr, optionalSessionHandle.Value());
+                onSetupFailure->mCall(onSetupFailure->mContext, failureInfo);
+            }
         }
     }
 }

src/app/OperationalSessionSetup.h

@@ -33,6 +33,7 @@
 #include <app/util/basic-types.h>
 #include <credentials/GroupDataProvider.h>
 #include <lib/address_resolve/AddressResolve.h>
+#include <lib/core/GroupedCallbackList.h>
 #include <messaging/ExchangeContext.h>
 #include <messaging/ExchangeDelegate.h>
 #include <messaging/ExchangeMgr.h>
@@ -309,9 +310,8 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate,
 
     SessionHolder mSecureSession;
 
-    Callback::CallbackDeque mConnectionSuccess;
-    Callback::CallbackDeque mConnectionFailure;
-    Callback::CallbackDeque mSetupFailure;
+    typedef Callback::GroupedCallbackList<OnDeviceConnected, OnDeviceConnectionFailure, OnSetupFailure> SuccessFailureCallbackList;
+    SuccessFailureCallbackList mCallbacks;
 
     OperationalSessionReleaseDelegate * mReleaseDelegate;
 
@@ -402,10 +402,8 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate,
      * notifications. This happens after the object has been released, if it's
      * being released.
      */
-    static void NotifyConnectionCallbacks(Callback::Cancelable & failureReady, Callback::Cancelable & setupFailureReady,
-                                          Callback::Cancelable & successReady, CHIP_ERROR error, SessionEstablishmentStage stage,
-                                          const ScopedNodeId & peerId, bool performingAddressUpdate,
-                                          Messaging::ExchangeManager * exchangeMgr,
+    static void NotifyConnectionCallbacks(SuccessFailureCallbackList & ready, CHIP_ERROR error, SessionEstablishmentStage stage,
+                                          const ScopedNodeId & peerId, Messaging::ExchangeManager * exchangeMgr,
                                           const Optional<SessionHandle> & optionalSessionHandle,
                                           // requestedBusyDelay will be 0 if not
                                           // CHIP_CONFIG_ENABLE_BUSY_HANDLING_FOR_OPERATIONAL_SESSION_SETUP,

src/lib/core/BUILD.gn

@@ -157,6 +157,7 @@ static_library("core") {
     "CHIPKeyIds.h",
     "CHIPPersistentStorageDelegate.h",
     "ClusterEnums.h",
+    "GroupedCallbackList.h",
     "OTAImageHeader.cpp",
     "OTAImageHeader.h",
     "PeerId.h",