Skip to content

Commit

Permalink
Fix OperationalSessionSetup notifying success with inactive sessions (p…
Browse files Browse the repository at this point in the history 
…roject-chip#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 <[email protected]>
  • Loading branch information
@ksperling-apple @bzbarsky-apple
ksperling-apple and bzbarsky-apple authored Jun 13, 2024
1 parent 0ae8a02 commit b081572
Show file tree
Hide file tree
Showing 6 changed files with 522 additions and 84 deletions.
120 changes: 43 additions & 77 deletions src/app/OperationalSessionSetup.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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);
}
}
}
}
Expand Down
12 changes: 5 additions & 7 deletions src/app/OperationalSessionSetup.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/lib/core/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ static_library("core") {
"CHIPKeyIds.h",
"CHIPPersistentStorageDelegate.h",
"ClusterEnums.h",
"GroupedCallbackList.h",
"OTAImageHeader.cpp",
"OTAImageHeader.h",
"PeerId.h",
Expand Down
Loading

0 comments on commit b081572

Please sign in to comment.