exchange leak in DeviceCommissioner::EstablishPASEConnection

[msandstedt]

Problem

At this rev:

commit b53521c4ee34cc9d53d7524d446ccd9be0af3fa0 (origin/master, origin/HEAD)
Author: Michael Sandstedt <[email protected]>
Date:   Fri Jan 7 17:49:40 2022 -0600

    Cleanup CASESession and PASESession lifetime (#13357)

In this code:

    exchangeCtxt = mSystemState->ExchangeMgr()->NewContext(session.Value(), &device->GetPairing());
    VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);

    err = mIDAllocator.Allocate(keyID);
    SuccessOrExit(err);

    // TODO - Remove use of SetActive/IsActive from CommissioneeDeviceProxy
    device->SetActive(true);

    err = device->GetPairing().Pair(params.GetPeerAddress(), params.GetSetupPINCode(), keyID,
                                    Optional<ReliableMessageProtocolConfig>::Value(mMRPConfig), exchangeCtxt, this);

This was identified here: #13294 (comment)

device->GetPairing().Pair() will free the exchange on error, but only if it is called. If mIDAllocator.Allocate(keyID); fails, this code leaks the exchange.

A similar leak for CASE likely exists in OperationalDeviceProxy or CASESessionManager.

Proposed Solution

This would cause this code not to leak:

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
index a5d3e36e4..ce183bdfc 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -832,8 +832,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     session = mSystemState->SessionMgr()->CreateUnauthenticatedSession(params.GetPeerAddress(), device->GetMRPConfig());
     VerifyOrExit(session.HasValue(), err = CHIP_ERROR_NO_MEMORY);
 
-    exchangeCtxt = mSystemState->ExchangeMgr()->NewContext(session.Value(), &device->GetPairing());
-    VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);
 
     err = mIDAllocator.Allocate(keyID);
     SuccessOrExit(err);
@@ -841,6 +839,15 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     // TODO - Remove use of SetActive/IsActive from CommissioneeDeviceProxy
     device->SetActive(true);
 
+    // Allocate the exchange immediately before calling PASESession::Pair.
+    //
+    // PASESession::Pair takes ownership of the exchange and will free it on
+    // error, but can only do this if it is actually called.  Allocating the
+    // exchange context right before calling Pair ensures that if allocation
+    // succeeds, PASESession has taken ownership.
+    exchangeCtxt = mSystemState->ExchangeMgr()->NewContext(session.Value(), &device->GetPairing());
+    VerifyOrExit(exchangeCtxt != nullptr, err = CHIP_ERROR_INTERNAL);
+
     err = device->GetPairing().Pair(params.GetPeerAddress(), params.GetSetupPINCode(), keyID,
                                     Optional<ReliableMessageProtocolConfig>::Value(mMRPConfig), exchangeCtxt, this);
     SuccessOrExit(err);

Perhaps better though would be for PASESession just to allocate its own exchange context.

[msandstedt]

Confirmed this bug also exists for CASE:

CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddress & peerAddress,
                                        const ReliableMessageProtocolConfig & mrpConfig, OnCASEConnected onConnection,
                                        OnCASEConnectionFailure onFailure, void * context)
{
    // Create a UnauthenticatedSession for CASE pairing.
    // Don't use mSecureSession here, because mSecureSession is for encrypted communication.
    Optional<SessionHandle> session = mInitParams.sessionManager->CreateUnauthenticatedSession(peerAddress, mrpConfig);
    VerifyOrReturnError(session.HasValue(), CHIP_ERROR_NO_MEMORY);

    Messaging::ExchangeContext * exchange = mInitParams.exchangeMgr->NewContext(session.Value(), &mCASESession);
    VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);

    uint16_t keyID = 0;
    ReturnErrorOnFailure(mInitParams.idAllocator->Allocate(keyID));

    ReturnErrorOnFailure(mCASESession.EstablishSession(peerAddress, mInitParams.fabricInfo, peer.GetNodeId(), keyID, exchange, this,
                                                       mInitParams.mrpLocalConfig));

If mInitParams.idAllocator->Allocate(keyID) fails, the exchange will be leaked.