From ba265d1bc769bc82ca2f2724470cd66d860af135 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 30 Jun 2021 11:29:45 -0400 Subject: [PATCH] Make exchanges handle closing on message receipt more automatically. The idea is that the cases that want to keep the exchange open should do so explicitly and in all other cases the exchange should close. This helps avoid exchange leaks and makes it much clearer when an exchange is being kept open. --- examples/shell/shell_common/cmd_send.cpp | 1 - src/app/CommandHandler.cpp | 2 + src/app/CommandSender.cpp | 5 +-- src/app/InteractionModelEngine.cpp | 1 - src/app/ReadClient.cpp | 4 +- src/app/ReadHandler.cpp | 10 +++++ src/app/WriteClient.cpp | 10 ++--- src/app/WriteHandler.cpp | 2 + src/app/server/Server.cpp | 1 - src/controller/CHIPDevice.cpp | 1 - src/controller/CHIPDeviceController.cpp | 6 --- src/messaging/ExchangeContext.cpp | 38 ++++++++++++++++++- src/messaging/ExchangeContext.h | 6 +++ src/messaging/ExchangeDelegate.h | 13 ++++++- src/messaging/ExchangeMgr.cpp | 34 +++++------------ src/messaging/ReliableMessageContext.h | 6 +++ src/messaging/tests/TestExchangeMgr.cpp | 1 - .../tests/TestReliableMessageProtocol.cpp | 6 ++- src/protocols/echo/EchoClient.cpp | 12 +----- src/protocols/secure_channel/CASESession.cpp | 13 ++++--- .../secure_channel/MessageCounterManager.cpp | 2 - src/protocols/secure_channel/PASESession.cpp | 13 ++++--- .../tests/TestMessageCounterManager.cpp | 1 - .../secure_channel/tests/TestPASESession.cpp | 1 - 24 files changed, 111 insertions(+), 78 deletions(-) diff --git a/examples/shell/shell_common/cmd_send.cpp b/examples/shell/shell_common/cmd_send.cpp index 45bdeb6ef68df7..821437593970ad 100644 --- a/examples/shell/shell_common/cmd_send.cpp +++ b/examples/shell/shell_common/cmd_send.cpp @@ -110,7 +110,6 @@ class MockAppDelegate : public Messaging::ExchangeDelegate streamer_printf(sout, "Response received: len=%u time=%.3fms\n", buffer->DataLength(), static_cast(transitTime) / 1000); - gExchangeCtx->Close(); gExchangeCtx = nullptr; return CHIP_NO_ERROR; } diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 6379cb2ca177b3..6002f1c61c30b9 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -71,6 +71,8 @@ CHIP_ERROR CommandHandler::SendCommandResponse() MoveToState(CommandState::Sending); exit: + // Keep Shutdown() from double-closing our exchange. + mpExchangeCtx = nullptr; Shutdown(); ChipLogFunctError(err); return err; diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index ebd67421110960..8764ef842818a6 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -92,9 +92,8 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha exit: ChipLogFunctError(err); - // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received. - // This needs to be done before the Reset() call, because Reset() aborts mpExchangeCtx if its not null. - mpExchangeCtx->Close(); + // Null out mpExchangeCtx, so our Shutdown() call below won't try to abort + // it and fail to send an ack for the message we just received. mpExchangeCtx = nullptr; if (mpDelegate != nullptr) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 2291764920733b..9bb7e1fdeed73b 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -190,7 +190,6 @@ CHIP_ERROR InteractionModelEngine::OnUnknownMsgType(Messaging::ExchangeContext * // err = SendStatusReport(ec, kChipProfile_Common, kStatus_UnsupportedMessage); // SuccessOrExit(err); - apExchangeContext->Close(); apExchangeContext = nullptr; ChipLogFunctError(err); diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 1bf4532e47f9fb..75ce150326f695 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -217,8 +217,8 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange exit: ChipLogFunctError(err); - // Close the exchange cleanly so that the ExchangeManager will send an ack for the message we just received. - mpExchangeCtx->Close(); + // Null out mpExchangeCtx, so our Shutdown() call below won't try to abort + // it and fail to send an ack for the message we just received. mpExchangeCtx = nullptr; MoveToState(ClientState::Initialized); diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 34898f22057cdd..f64d1a48b1c422 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -82,6 +82,8 @@ CHIP_ERROR ReadHandler::OnReadRequest(Messaging::ExchangeContext * apExchangeCon if (err != CHIP_NO_ERROR) { ChipLogFunctError(err); + // Keep Shutdown() from double-closing our exchange. + mpExchangeCtx = nullptr; Shutdown(); } @@ -153,6 +155,14 @@ CHIP_ERROR ReadHandler::ProcessReadRequest(System::PacketBufferHandle && aPayloa MoveToState(HandlerState::Reportable); err = InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun(); + SuccessOrExit(err); + + // mpExchangeCtx can be null here due to + // https://github.com/project-chip/connectedhomeip/issues/8031 + if (mpExchangeCtx) + { + mpExchangeCtx->WillSendMessage(); + } exit: ChipLogFunctError(err); diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index 67769d4237b1d2..7f01f63fcba902 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -294,20 +294,16 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang VerifyOrDie(apExchangeContext == mpExchangeCtx); + // We are done with this exchange, and it will be closing itself. + mpExchangeCtx = nullptr; + // Verify that the message is an Invoke Command Response. // If not, close the exchange and free the payload. if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteResponse)) { - apExchangeContext->Close(); - mpExchangeCtx = nullptr; ExitNow(); } - // Close the current exchange after receiving the response since the response message marks the - // end of conversation represented by the exchange. We should create an new exchange for a new - // conversation defined in Interaction Model protocol. - ClearExistingExchangeContext(); - err = ProcessWriteResponseMessage(std::move(aPayload)); exit: diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 6aadad83676f7b..f4eb1d342529d1 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -74,6 +74,8 @@ CHIP_ERROR WriteHandler::OnWriteRequest(Messaging::ExchangeContext * apExchangeC exit: ChipLogFunctError(err); + // Keep Shutdown() from double-closing our exchange. + mpExchangeCtx = nullptr; Shutdown(); return err; } diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index bee77faa3fb6f0..d643c2c1fb412e 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -387,7 +387,6 @@ class ServerCallback : public ExchangeDelegate } exit: - exchangeContext->Close(); return err; } diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 5daf9ad0422cd7..a4573cd433ad74 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -338,7 +338,6 @@ CHIP_ERROR Device::OnMessageReceived(Messaging::ExchangeContext * exchange, cons HandleDataModelMessage(exchange, std::move(msgBuf)); } } - exchange->Close(); return CHIP_NO_ERROR; } diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 3556b65915482e..c82cb42a930fed 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -574,7 +574,6 @@ CHIP_ERROR DeviceController::OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, System::PacketBufferHandle && msgBuf) { uint16_t index; - bool needClose = true; VerifyOrExit(mState == State::Initialized, ChipLogError(Controller, "OnMessageReceived was called in incorrect state")); @@ -584,14 +583,9 @@ CHIP_ERROR DeviceController::OnMessageReceived(Messaging::ExchangeContext * ec, index = FindDeviceIndex(packetHeader.GetSourceNodeId().Value()); VerifyOrExit(index < kNumMaxActiveDevices, ChipLogError(Controller, "OnMessageReceived was called for unknown device object")); - needClose = false; // Device will handle it mActiveDevices[index].OnMessageReceived(ec, packetHeader, payloadHeader, std::move(msgBuf)); exit: - if (needClose) - { - ec->Close(); - } return CHIP_NO_ERROR; } diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 2dfa925f94ab3c..77c880dcd44249 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -79,6 +79,9 @@ void ExchangeContext::SetResponseTimeout(Timeout timeout) CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgType, PacketBufferHandle && msgBuf, const SendFlags & sendFlags) { + // If we were waiting for a message send, this is it. + mFlags.Clear(Flags::kFlagWillSendMessage); + CHIP_ERROR err = CHIP_NO_ERROR; Transport::PeerConnectionState * state = nullptr; @@ -378,18 +381,26 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con // layer has completed its work on the ExchangeContext. Retain(); + // Keep track of whether we're nested under an outer HandleMessage + // invocation. + bool alreadyHandlingMessage = mFlags.Has(Flags::kFlagHandlingMessage); + mFlags.Set(Flags::kFlagHandlingMessage); + + bool isStandaloneAck = payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::StandaloneAck); + bool isDuplicate = msgFlags.Has(MessageFlagValues::kDuplicateMessage); + CHIP_ERROR err = mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, msgFlags, GetReliableMessageContext()); SuccessOrExit(err); // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. - if (payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::StandaloneAck)) + if (isStandaloneAck) { ExitNow(err = CHIP_NO_ERROR); } // Since the message is duplicate, let's not forward it up the stack - if (msgFlags.Has(MessageFlagValues::kDuplicateMessage)) + if (isDuplicate) { ExitNow(err = CHIP_NO_ERROR); } @@ -412,6 +423,29 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con } exit: + // Don't close ourselves if this message is a standalone ack. Either we're + // still not closed and getting an ack should not affect that, or we're + // already closed and then closing again would be a bad idea. + // + // Don't close for duplicates for similar reasons, except when mDelegate is + // null: that case means we created the exchange just to send an ack for + // this duplicate, and we don't need to exist anymore. + // + // Also don't close if there's an outer HandleMessage invocation. It'll + // deal with the closing. + if (!mFlags.Has(Flags::kFlagWillSendMessage) && !IsResponseExpected() && !isStandaloneAck && + (!isDuplicate || (mDelegate == nullptr)) && !alreadyHandlingMessage) + { + Close(); + } + + if (!alreadyHandlingMessage) + { + // We are the outermost HandleMessage invocation. We're not handling a + // message anymore. + mFlags.Clear(Flags::kFlagHandlingMessage); + } + // Release the reference to the ExchangeContext that was held at the beginning of this function. // This call should also do the needful of closing the ExchangeContext if the protocol has // already made a prior call to Close(). diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 8c8c6032a36c3e..358db8a8aaf557 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -109,6 +109,12 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen std::move(msgPayload), sendFlags); } + /** + * A notification that we will have SendMessage called on us in the future + * (and should stay open until that happens). + */ + void WillSendMessage() { mFlags.Set(Flags::kFlagWillSendMessage); } + /** * Handle a received CHIP message on this exchange. * diff --git a/src/messaging/ExchangeDelegate.h b/src/messaging/ExchangeDelegate.h index 9d623baffa791a..db694dd477b3e2 100644 --- a/src/messaging/ExchangeDelegate.h +++ b/src/messaging/ExchangeDelegate.h @@ -49,7 +49,18 @@ class DLL_EXPORT ExchangeDelegate /** * @brief - * This function is the protocol callback for handling a received CHIP message. + * This function is the protocol callback for handling a received CHIP + * message. + * + * After calling this method an exchange will close itself unless one of + * two things happens: + * + * 1) A call to SendMessage on the exchange with the kExpectResponse flag + * set. + * 2) A call to WillSendMessage on the exchange. + * + * Consumers that don't do one of those things MUST NOT retain a pointer + * to the exchange. * * @param[in] ec A pointer to the ExchangeContext object. * @param[in] packetHeader A reference to the PacketHeader object. diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index a769574354bbd4..a412b74ab467d4 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -201,7 +201,6 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const { CHIP_ERROR err = CHIP_NO_ERROR; UnsolicitedMessageHandler * matchingUMH = nullptr; - bool sendAckAndCloseExchange = false; ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %" PRIu32 " on exchange %d", payloadHeader.GetMessageType(), payloadHeader.GetProtocolID().ToFullyQualifiedSpecForm(), @@ -269,36 +268,23 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ExitNow(err = CHIP_ERROR_UNSOLICITED_MSG_NO_ORIGINATOR); } - // If we didn't find an existing exchange that matches the message, and no unsolicited message handler registered - // to hand this message, we need to create a temporary exchange to send an ack for this message and then close this exchange. - sendAckAndCloseExchange = payloadHeader.NeedsAck() && (matchingUMH == nullptr); - - // If we found a handler or we need to create a new exchange context (EC). - if (matchingUMH != nullptr || sendAckAndCloseExchange) + // If we found a handler or we need to send an ack, create an exchange to + // handle the message. + if (matchingUMH != nullptr || payloadHeader.NeedsAck()) { - ExchangeContext * ec = nullptr; - - if (sendAckAndCloseExchange) - { - // If rcvd msg is from initiator then this exchange is created as not Initiator. - // If rcvd msg is not from initiator then this exchange is created as Initiator. - // TODO: Figure out which channel to use for the received message - ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), nullptr); - } - else - { - ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, false, matchingUMH->Delegate); - } + ExchangeDelegate * delegate = matchingUMH ? matchingUMH->Delegate : nullptr; + // If rcvd msg is from initiator then this exchange is created as not Initiator. + // If rcvd msg is not from initiator then this exchange is created as Initiator. + // Note that if matchingUMH is not null then rcvd msg if from initiator. + // TODO: Figure out which channel to use for the received message + ExchangeContext * ec = + mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), delegate); VerifyOrExit(ec != nullptr, err = CHIP_ERROR_NO_MEMORY); ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%p", ec->GetExchangeId(), ec->GetDelegate()); ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf)); - - // Close exchange if it was created only to send ack for a duplicate message. - if (sendAckAndCloseExchange) - ec->Close(); } exit: diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index d681851deb0629..ffff826b64e91d 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -202,6 +202,12 @@ class ReliableMessageContext /// When set, signifies that at least one message has been received from peer on this exchange context. kFlagMsgRcvdFromPeer = 0x0040, + + /// When set, signifies that this exchange is waiting for a call to SendMessage. + kFlagWillSendMessage = 0x0080, + + /// When set, signifies that we are currently in the middle of HandleMessage. + kFlagHandlingMessage = 0x0100, }; BitFlags mFlags; // Internal state flags diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index fb26006ed1040b..719c394649a27f 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -81,7 +81,6 @@ class MockAppDelegate : public ExchangeDelegate System::PacketBufferHandle && buffer) override { IsOnMessageReceivedCalled = true; - ec->Close(); return CHIP_NO_ERROR; } diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 8e09873399a0b7..befb84aa46d36e 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -115,9 +115,12 @@ class MockAppDelegate : public ExchangeDelegate if (!mRetainExchange) { - ec->Close(); ec = nullptr; } + else + { + ec->WillSendMessage(); + } mExchange = ec; if (mTestSuite != nullptr) @@ -181,7 +184,6 @@ class MockSessionEstablishmentDelegate : public ExchangeDelegate System::PacketBufferHandle && buffer) override { IsOnMessageReceivedCalled = true; - ec->Close(); if (mTestSuite != nullptr) { NL_TEST_ASSERT(mTestSuite, buffer->TotalLength() == sizeof(PAYLOAD)); diff --git a/src/protocols/echo/EchoClient.cpp b/src/protocols/echo/EchoClient.cpp index fe14851805303b..d1024773d70b63 100644 --- a/src/protocols/echo/EchoClient.cpp +++ b/src/protocols/echo/EchoClient.cpp @@ -95,22 +95,14 @@ CHIP_ERROR EchoClient::OnMessageReceived(Messaging::ExchangeContext * ec, const // which clears the OnMessageReceived callback. VerifyOrDie(ec == mExchangeCtx); + mExchangeCtx = nullptr; + // Verify that the message is an Echo Response. - // If not, close the exchange and free the payload. if (!payloadHeader.HasMessageType(MsgType::EchoResponse)) { - ec->Close(); - mExchangeCtx = nullptr; return CHIP_ERROR_INVALID_ARGUMENT; } - // Remove the EC from the app state now. OnEchoResponseReceived can call - // SendEchoRequest and install a new one. We abort rather than close - // because we no longer care whether the echo request message has been - // acknowledged at the transport layer. - mExchangeCtx->Abort(); - mExchangeCtx = nullptr; - // Call the registered OnEchoResponseReceived handler, if any. if (OnEchoResponseReceived != nullptr) { diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index eb9a4111cd2dc5..aeb47a93216dec 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -810,8 +810,8 @@ CHIP_ERROR CASESession::SendSigmaR3() mPairingComplete = true; - // Close the exchange, as no additional messages are expected from the peer - CloseExchange(); + // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt = nullptr; // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -906,8 +906,8 @@ CHIP_ERROR CASESession::HandleSigmaR3(const System::PacketBufferHandle & msg) mPairingComplete = true; - // Close the exchange, as no additional messages are expected from the peer - CloseExchange(); + // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt = nullptr; // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -1142,8 +1142,6 @@ CHIP_ERROR CASESession::ValidateReceivedMessage(ExchangeContext * ec, const Pack { if (mExchangeCtxt != ec) { - // Close the incoming exchange explicitly, as the cleanup code only closes mExchangeCtxt - ec->Close(); ReturnErrorOnFailure(CHIP_ERROR_INVALID_ARGUMENT); } } @@ -1211,6 +1209,9 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PacketHead // Call delegate to indicate session establishment failure. if (err != CHIP_NO_ERROR) { + // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // exchange will handle that. + mExchangeCtxt = nullptr; Clear(); mDelegate->OnSessionEstablishmentError(err); } diff --git a/src/protocols/secure_channel/MessageCounterManager.cpp b/src/protocols/secure_channel/MessageCounterManager.cpp index 264801d5be5868..cd318ad8b58df7 100644 --- a/src/protocols/secure_channel/MessageCounterManager.cpp +++ b/src/protocols/secure_channel/MessageCounterManager.cpp @@ -277,7 +277,6 @@ CHIP_ERROR MessageCounterManager::HandleMsgCounterSyncReq(Messaging::ExchangeCon ChipLogError(SecureChannel, "Failed to handle MsgCounterSyncReq message with error:%s", ErrorStr(err)); } - exchangeContext->Close(); return err; } @@ -324,7 +323,6 @@ CHIP_ERROR MessageCounterManager::HandleMsgCounterSyncResp(Messaging::ExchangeCo ChipLogError(SecureChannel, "Failed to handle MsgCounterSyncResp message with error:%s", ErrorStr(err)); } - exchangeContext->Close(); return err; } diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 8676bb93277c99..82790917564ee7 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -646,8 +646,8 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle mPairingComplete = true; - // Close the exchange, as no additional messages are expected from the peer - CloseExchange(); + // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt = nullptr; // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -688,8 +688,8 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) mPairingComplete = true; - // Close the exchange, as no additional messages are expected from the peer - CloseExchange(); + // Forget our exchange, as no additional messages are expected from the peer + mExchangeCtxt = nullptr; // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -764,8 +764,6 @@ CHIP_ERROR PASESession::ValidateReceivedMessage(ExchangeContext * exchange, cons { if (mExchangeCtxt != exchange) { - // Close the incoming exchange explicitly, as the cleanup code only closes mExchangeCtxt - exchange->Close(); ReturnErrorOnFailure(CHIP_ERROR_INVALID_ARGUMENT); } } @@ -827,6 +825,9 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Pack // Call delegate to indicate pairing failure if (err != CHIP_NO_ERROR) { + // Null out mExchangeCtxt so that Clear() doesn't try closing it. The + // exchange will handle that. + mExchangeCtxt = nullptr; Clear(); ChipLogError(SecureChannel, "Failed during PASE session setup. %s", ErrorStr(err)); mDelegate->OnSessionEstablishmentError(err); diff --git a/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp b/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp index f957d22a5df964..21812a00dbaf8f 100644 --- a/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp +++ b/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp @@ -77,7 +77,6 @@ class MockAppDelegate : public ExchangeDelegate System::PacketBufferHandle && msgBuf) override { ++ReceiveHandlerCallCount; - ec->Close(); return CHIP_NO_ERROR; } diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index 67a52954193f01..2cc57994d3e776 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -123,7 +123,6 @@ class MockAppDelegate : public ExchangeDelegate CHIP_ERROR OnMessageReceived(ExchangeContext * ec, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, System::PacketBufferHandle && buffer) override { - ec->Close(); return CHIP_NO_ERROR; }