Skip to content

Commit

Permalink
Remove the OnSessionReleased callback from OperationalSessionSetup.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple committed May 5, 2023
1 parent 70cbd5f commit 1093560
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 21 deletions.
18 changes: 8 additions & 10 deletions src/app/OperationalSessionSetup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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())
Expand Down
14 changes: 3 additions & 11 deletions src/app/OperationalSessionSetup.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 1093560

Please sign in to comment.