Skip to content

Commit

Permalink
Set CASESessio
Browse files Browse the repository at this point in the history
The fix in project-chip#12794 means that the CASESession and PASESession objects do
not need to persist for ExchangeMessageDispatch::SendMessage to succeed
at the final ACK of session establishment.  Instead, SendMessage uses the
SessionEstablishmentExchangeDispatch global singleton.

Ths means we can address 13146 such that CASESession and PASESession may
actually be freed or reused when completion callbacks fire.  This will
only work, however, if these objects clear themselves as delegates for
their exchange contexts when discarding references to these.  This
commit does so.

This commit also reorders all calls to mDelegate->OnSessionEstablished and
mDelegate->OnSessionEstablishmentError to occur last in any given method
in case mDelegate frees or reuses the CASESession or PASESession objects
on execution of these completion callbacks.

With this, we can remove the behavior in the OperationalDeviceProxy which
that was defering release of the CASESession object until after an
iteration of the event loop.  Now when OnSessionEstablished fires, the
CASESession can be reused or discared immediately.
  • Loading branch information
msandstedt committed Jan 7, 2022
1 parent fb154cf commit 39ced66
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 17 deletions.
15 changes: 4 additions & 11 deletions src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -280,22 +280,15 @@ void OperationalDeviceProxy::Clear()
mInitParams = DeviceProxyInitParams();
}

void OperationalDeviceProxy::CloseCASESessionTask(System::Layer * layer, void * context)
void OperationalDeviceProxy::DeferCloseCASESession()
{
OperationalDeviceProxy * device = static_cast<OperationalDeviceProxy *>(context);
if (device->mCASEClient)
if (this->mCASEClient)
{
device->mInitParams.clientPool->Release(device->mCASEClient);
device->mCASEClient = nullptr;
this->mInitParams.clientPool->Release(this->mCASEClient);
this->mCASEClient = nullptr;
}
}

void OperationalDeviceProxy::DeferCloseCASESession()
{
// Defer the release for the pending Ack to be sent
mSystemLayer->ScheduleWork(CloseCASESessionTask, this);
}

void OperationalDeviceProxy::OnSessionReleased(const SessionHandle & session)
{
VerifyOrReturn(mSecureSession.Contains(session),
Expand Down
18 changes: 14 additions & 4 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,13 @@ void CASESession::OnResponseTimeout(ExchangeContext * ec)
VerifyOrReturn(mExchangeCtxt == ec, ChipLogError(SecureChannel, "CASESession::OnResponseTimeout exchange doesn't match"));
ChipLogError(SecureChannel, "CASESession timed out while waiting for a response from the peer. Current state was %" PRIu8,
mState);
mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
// Null out mExchangeCtxt so that Clear() doesn't try closing it. The
// exchange will handle that.
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;
Clear();
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
}

CHIP_ERROR CASESession::DeriveSecureSession(CryptoContext & session, CryptoContext::SessionRole role)
Expand Down Expand Up @@ -684,9 +686,11 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg)
mCASESessionEstablished = true;

// Forget our exchange, as no additional messages are expected from the peer
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;

// Call delegate to indicate session establishment is successful
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablished();

exit:
Expand Down Expand Up @@ -1118,9 +1122,11 @@ CHIP_ERROR CASESession::HandleSigma3(System::PacketBufferHandle && msg)
mCASESessionEstablished = true;

// Forget our exchange, as no additional messages are expected from the peer
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;

// Call delegate to indicate session establishment is successful
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablished();

exit:
Expand Down Expand Up @@ -1302,15 +1308,17 @@ void CASESession::OnSuccessStatusReport()
mCASESessionEstablished = true;

// Forget our exchange, as no additional messages are expected from the peer
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;

// Call delegate to indicate pairing completion
mDelegate->OnSessionEstablished();

mState = kInitialized;

// TODO: Set timestamp on the new session, to allow selecting a least-recently-used session for eviction
// on running out of session contexts.

// Call delegate to indicate pairing completion.
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablished();
}

CHIP_ERROR CASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode)
Expand Down Expand Up @@ -1524,8 +1532,10 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
{
// Null out mExchangeCtxt so that Clear() doesn't try closing it. The
// exchange will handle that.
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;
Clear();
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablishmentError(err);
}
return err;
Expand Down
10 changes: 9 additions & 1 deletion src/protocols/secure_channel/PASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -343,11 +343,13 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec)
ChipLogError(SecureChannel,
"PASESession timed out while waiting for a response from the peer. Expected message type was %" PRIu8,
to_underlying(mNextExpectedMsg));
mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
// Null out mExchangeCtxt so that Clear() doesn't try closing it. The
// exchange will handle that.
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;
Clear();
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT);
}

CHIP_ERROR PASESession::DeriveSecureSession(CryptoContext & session, CryptoContext::SessionRole role)
Expand Down Expand Up @@ -824,9 +826,11 @@ CHIP_ERROR PASESession::HandleMsg3(System::PacketBufferHandle && msg)
mPairingComplete = true;

// Forget our exchange, as no additional messages are expected from the peer
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;

// Call delegate to indicate pairing completion
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablished();

exit:
Expand All @@ -843,9 +847,11 @@ void PASESession::OnSuccessStatusReport()
mPairingComplete = true;

// Forget our exchange, as no additional messages are expected from the peer
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;

// Call delegate to indicate pairing completion
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablished();
}

Expand Down Expand Up @@ -938,9 +944,11 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl
{
// Null out mExchangeCtxt so that Clear() doesn't try closing it. The
// exchange will handle that.
mExchangeCtxt->SetDelegate(nullptr);
mExchangeCtxt = nullptr;
Clear();
ChipLogError(SecureChannel, "Failed during PASE session setup. %s", ErrorStr(err));
// Do this last in case the delegate frees us.
mDelegate->OnSessionEstablishmentError(err);
}
return err;
Expand Down
2 changes: 1 addition & 1 deletion third_party/pigweed/repo
Submodule repo updated 433 files

0 comments on commit 39ced66

Please sign in to comment.