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; }