From 53af3b0e5635bbbefc601974b787b0a53c00f455 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 9 Jul 2021 17:08:55 -0400 Subject: [PATCH] Make exchanges handle closing on message receipt more automatically. (#8050) 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 | 13 +++++ 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 | 43 +++++++++++++- src/messaging/ExchangeContext.h | 6 ++ src/messaging/ExchangeDelegate.h | 13 ++++- src/messaging/ExchangeMgr.cpp | 34 ++++------- src/messaging/ReliableMessageContext.h | 8 +++ src/messaging/tests/TestExchangeMgr.cpp | 1 - .../tests/TestReliableMessageProtocol.cpp | 57 ++++++++++++++++++- 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, 172 insertions(+), 78 deletions(-) diff --git a/examples/shell/shell_common/cmd_send.cpp b/examples/shell/shell_common/cmd_send.cpp index 5caa790c712464..631b3fe59d5300 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 248cf94270f68f..20e3d8cd9776a3 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -72,6 +72,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 73b87bba2a1af4..8ba0e2b69e55bc 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -192,7 +192,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 adca991dc3a4ee..e68342a808b2e0 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -229,8 +229,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..344fb01ad909cf 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,17 @@ 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(); + } + + // There must be no code after the WillSendMessage() call that can cause + // this method to return a failure. exit: ChipLogFunctError(err); diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index ba43e62a7409ca..c64c147205c34c 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 Write 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 17d619dc2a741d..f726c13c5e3c8a 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -75,6 +75,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 9215fdeb8188e3..bb6997ccecd2ce 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 82d4fd5e0e73eb..d04ba07ae9f71b 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -337,7 +337,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 f748aa9516a46b..0fd3b28e8080e3 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -572,7 +572,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")); @@ -582,14 +581,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 585aa9e4c7c0c3..fee80615221a15 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; @@ -143,6 +146,8 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp void ExchangeContext::DoClose(bool clearRetransTable) { + mFlags.Set(Flags::kFlagClosed); + // Clear protocol callbacks if (mDelegate != nullptr) { @@ -379,6 +384,14 @@ 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); @@ -393,13 +406,13 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con } // 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); } @@ -422,6 +435,32 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con } exit: + // Don't close ourselves if we're already closed. + // + // Don't close ourselves if this message is a standalone ack. We're still + // not closed and getting an ack should not affect that. In particular, + // since the standalone ack was not passed to the delegate, the delegate + // never got a chance to say "stay open". The one exception here is if + // mDelegate is null: in that case this is an unsolicited message and we + // were just created to ack it and close after that. + // + // Don't close for duplicates for similar reasons, with the same exception. + // + // Also don't close if there's an outer HandleMessage invocation. It'll + // deal with the closing. + if (!mFlags.Has(Flags::kFlagClosed) && !mFlags.Has(Flags::kFlagWillSendMessage) && !IsResponseExpected() && + (!isStandaloneAck || (mDelegate == nullptr)) && (!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 5a83e66af772c5..c766af817a0538 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..af6c394dd9a697 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -202,6 +202,14 @@ 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, + /// When set, we have had Close() or Abort() called on us already. + kFlagClosed = 0x0200, }; BitFlags mFlags; // Internal state flags diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index 27b0a0a22c1837..2964d6f7e798b9 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -67,7 +67,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 f6c0c22ceecc1f..84f6891b9894f6 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -88,9 +88,12 @@ class MockAppDelegate : public ExchangeDelegate if (!mRetainExchange) { - ec->Close(); ec = nullptr; } + else + { + ec->WillSendMessage(); + } mExchange = ec; if (mTestSuite != nullptr) @@ -154,7 +157,6 @@ class MockSessionEstablishmentDelegate : public ExchangeDelegate System::PacketBufferHandle && buffer) override { IsOnMessageReceivedCalled = true; - ec->Close(); if (mTestSuite != nullptr) { NL_TEST_ASSERT(mTestSuite, buffer->TotalLength() == sizeof(PAYLOAD)); @@ -943,6 +945,56 @@ void CheckNoPiggybackAfterPiggyback(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); } +void CheckSendUnsolicitedStandaloneAckMessage(nlTestSuite * inSuite, void * inContext) +{ + /** + * Tests sending a standalone ack message that is: + * 1) Unsolicited. + * 2) Requests an ack. + * + * This is not a thing that would normally happen, but a malicious entity + * could absolutely do this. + */ + TestContext & ctx = *reinterpret_cast(inContext); + + ctx.GetInetLayer().SystemLayer()->Init(nullptr); + + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData("", 0); + NL_TEST_ASSERT(inSuite, !buffer.IsNull()); + + CHIP_ERROR err = CHIP_NO_ERROR; + + MockAppDelegate mockSender; + ExchangeContext * exchange = ctx.NewExchangeToPeer(&mockSender); + NL_TEST_ASSERT(inSuite, exchange != nullptr); + + mockSender.mTestSuite = inSuite; + + ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + NL_TEST_ASSERT(inSuite, rm != nullptr); + + // Ensure the retransmit table is empty right now + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); + + // We send a message, have it get received by the peer, expect an ack from + // the peer. + gLoopback.mSentMessageCount = 0; + gLoopback.mNumMessagesToDrop = 0; + gLoopback.mDroppedMessageCount = 0; + + // Purposefully sending a standalone ack that requests an ack! + err = exchange->SendMessage(SecureChannel::MsgType::StandaloneAck, std::move(buffer)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + exchange->Close(); + + // Ensure the message and its ack were sent. + NL_TEST_ASSERT(inSuite, gLoopback.mSentMessageCount == 2); + NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); + + // And that nothing is waiting for acks. + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); +} + void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); @@ -1106,6 +1158,7 @@ const nlTest sTests[] = NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessageClosedExchange", CheckDuplicateMessageClosedExchange), NL_TEST_DEF("Test that a reply after a standalone ack comes through correctly", CheckReceiveAfterStandaloneAck), NL_TEST_DEF("Test that a reply to a non-MRP message does not piggyback an ack even if there were MRP things happening on the context before", CheckNoPiggybackAfterPiggyback), + NL_TEST_DEF("Test sending an unsolicited ack-soliciting 'standalone ack' message", CheckSendUnsolicitedStandaloneAckMessage), NL_TEST_DEF("Test ReliableMessageMgr::CheckSendStandaloneAckMessage", CheckSendStandaloneAckMessage), NL_TEST_DEF("Test command, response, default response, with receiver closing exchange after sending response", CheckMessageAfterClosed), 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 4a3b22c207e859..ff18f1b4be259e 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -935,8 +935,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(); @@ -1079,8 +1079,8 @@ CHIP_ERROR CASESession::HandleSigmaR3(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(); @@ -1332,8 +1332,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); } } @@ -1401,6 +1399,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 bb935e2f3e0c70..8259524a4e9e33 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 8e12adc713dc52..db3bfc92042ade 100644 --- a/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp +++ b/src/protocols/secure_channel/tests/TestMessageCounterManager.cpp @@ -63,7 +63,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 9d72adb7cce958..236fb0ffbde500 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -99,7 +99,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; }