Skip to content

Commit

Permalink
Fix possible exchange leak in session establishment (#14416)
Browse files Browse the repository at this point in the history
The CASESession and PASESession objects take ownership of their exchange
contexts once passed, and will free and / or close them as appropriate.
However, for this object lifecycle management to occur, exchange contexts
must be passed to these objects in the first place.

The current session establishment calls do not do this if early-out
failures occur between allocation of the exchange contexts and
calls to the session establishment methods.

This commit fixes this problem by allocating exchanges immediately
before the calls to the session establishment methods.  This removes
the possibility of early-out failures causing leaks.

Fixes #13422
  • Loading branch information
msandstedt authored and pull[bot] committed Sep 29, 2023
1 parent 917010b commit 1716081
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 6 deletions.
12 changes: 9 additions & 3 deletions src/app/CASEClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,18 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
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));

// Allocate the exchange immediately before calling CASESession::EstablishSession.
//
// CASESession::EstablishSession 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 EstablishSession
// ensures that if allocation succeeds, CASESession has taken ownership.
Messaging::ExchangeContext * exchange = mInitParams.exchangeMgr->NewContext(session.Value(), &mCASESession);
VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);

ReturnErrorOnFailure(mCASESession.EstablishSession(peerAddress, mInitParams.fabricInfo, peer.GetNodeId(), keyID, exchange, this,
mInitParams.mrpLocalConfig));
mConnectionSuccessCallback = onConnection;
Expand Down
12 changes: 9 additions & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -878,15 +878,21 @@ 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);

// 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);
Expand Down

0 comments on commit 1716081

Please sign in to comment.