From 19514e3a3c46c68e54bd5da26255a9856b0ac594 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Wed, 8 Sep 2021 15:40:04 -0700 Subject: [PATCH 1/8] Expire associated secure sessions when a fabric is deleted --- .../operational-credentials-server.cpp | 26 ++++++++ .../suites/OperationalCredentialsCluster.yaml | 7 ++ .../Framework/CHIPTests/CHIPClustersTests.m | 19 ++++++ src/messaging/ExchangeContext.h | 1 + src/messaging/ReliableMessageMgr.cpp | 2 +- src/protocols/secure_channel/CASESession.cpp | 2 +- src/transport/PeerConnections.h | 34 ++++++++++ src/transport/SecureSessionMgr.cpp | 12 ++++ src/transport/SecureSessionMgr.h | 1 + .../chip-tool/zap-generated/test/Commands.h | 64 ++++++++++++++++++- .../zap-generated/CHIPClusters.cpp | 20 ++++++ .../lighting-app/zap-generated/CHIPClusters.h | 3 + 12 files changed, 188 insertions(+), 3 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index d61676e531a728..3273dc85a5f73d 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -238,6 +238,30 @@ void emberAfPluginOperationalCredentialsServerInitCallback(void) writeFabricsIntoFabricsListAttribute(); } +namespace { +class OpCredsClusterExchangeDelegate : public Messaging::ExchangeDelegate +{ +public: + CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, + const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) override + { + return CHIP_NO_ERROR; + } + void OnResponseTimeout(Messaging::ExchangeContext * ec) override {} + void OnExchangeClosing(Messaging::ExchangeContext * ec) override + { + ec->GetExchangeMgr()->GetReliableMessageMgr()->ClearRetransTable(static_cast(ec)); + ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(mFabricIndex); + mFabricIndex = 0; + } + + FabricIndex mFabricIndex = 0; +}; + +OpCredsClusterExchangeDelegate gFabricCleanupExchangeDelegate; + +} // namespace + bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoint, app::CommandHandler * commandObj, FabricIndex fabricIndex) { @@ -250,6 +274,8 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoin exit: writeFabricsIntoFabricsListAttribute(); emberAfSendImmediateDefaultResponse(status); + gFabricCleanupExchangeDelegate.mFabricIndex = fabricIndex; + emberAfCurrentCommand()->source->SetDelegate(&gFabricCleanupExchangeDelegate); return true; } diff --git a/src/app/tests/suites/OperationalCredentialsCluster.yaml b/src/app/tests/suites/OperationalCredentialsCluster.yaml index a2fbd460c5bd13..fd2d1f2ff8168a 100644 --- a/src/app/tests/suites/OperationalCredentialsCluster.yaml +++ b/src/app/tests/suites/OperationalCredentialsCluster.yaml @@ -34,3 +34,10 @@ tests: constraints: type: uint8 minValue: 1 + + - label: "Remove fabric" + command: "RemoveFabric" + arguments: + values: + - name: "FabricIndex" + value: 1 diff --git a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m index 2ca603e6966ca3..bd8562e3bf87c4 100644 --- a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m +++ b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m @@ -5246,6 +5246,25 @@ - (void)testSendClusterOperationalCredentialsCluster_000001_ReadAttribute [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; } +- (void)testSendClusterOperationalCredentialsCluster_000002_RemoveFabric +{ + XCTestExpectation * expectation = [self expectationWithDescription:@"Remove fabric"]; + CHIPDevice * device = GetPairedDevice(kDeviceId); + dispatch_queue_t queue = dispatch_get_main_queue(); + CHIPOperationalCredentials * cluster = [[CHIPOperationalCredentials alloc] initWithDevice:device endpoint:0 queue:queue]; + XCTAssertNotNil(cluster); + + uint8_t fabricIndexArgument = 1; + [cluster removeFabric:fabricIndexArgument + responseHandler:^(NSError * err, NSDictionary * values) { + NSLog(@"Remove fabric Error: %@", err); + + XCTAssertEqual(err.code, 0); + [expectation fulfill]; + }]; + + [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; +} - (void)testSendClusterAccountLoginReadAttributeClusterRevisionWithResponseHandler { diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index e45dc15098d94a..43ae371e0c8927 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -165,6 +165,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen } SessionHandle GetSecureSession() { return mSecureSession.Value(); } + bool HasSecureSession() const { return mSecureSession.HasValue(); } uint16_t GetExchangeId() const { return mExchangeId; } diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 384ee0ea902b27..c180b9d5701224 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -357,7 +357,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry) } const ExchangeMessageDispatch * dispatcher = rc->GetExchangeContext()->GetMessageDispatch(); - if (dispatcher == nullptr) + if (dispatcher == nullptr || !rc->GetExchangeContext()->HasSecureSession()) { // Using same error message for all errors to reduce code size. ChipLogError(ExchangeManager, "Crit-err %" CHIP_ERROR_FORMAT " when sending CHIP MsgId:%08" PRIX32 ", send tries: %d", diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 6feab01290c096..4198f2f1259543 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1035,7 +1035,7 @@ void CASESession::SendErrorMsg(SigmaErrorType errorCode) msg->SetDataLength(msglen); - VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) != CHIP_NO_ERROR, + VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) == CHIP_NO_ERROR, ChipLogError(SecureChannel, "Failed to send error message")); } diff --git a/src/transport/PeerConnections.h b/src/transport/PeerConnections.h index faa820d1084817..43097e2d2a4397 100644 --- a/src/transport/PeerConnections.h +++ b/src/transport/PeerConnections.h @@ -316,6 +316,40 @@ class PeerConnections return state; } + /** + * Get a peer connection state that matches the given fabric index. + * + * @param fabric The fabric index to match + * @param begin If a member of the pool, will start search from the next item. Can be nullptr to search from start. + * + * @return the state found, nullptr if not found + */ + CHECK_RETURN_VALUE + PeerConnectionState * FindPeerConnectionStateByFabric(FabricIndex fabric, PeerConnectionState * begin) + { + PeerConnectionState * state = nullptr; + PeerConnectionState * iter = &mStates[0]; + + if (begin >= iter && begin < &mStates[kMaxConnectionCount]) + { + iter = begin + 1; + } + + for (; iter < &mStates[kMaxConnectionCount]; iter++) + { + if (!iter->IsInitialized()) + { + continue; + } + if (iter->GetFabricIndex() == fabric) + { + state = iter; + break; + } + } + return state; + } + /// Convenience method to mark a peer connection state as active void MarkConnectionActive(PeerConnectionState * state) { diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index b727c0c75ab302..6fa2612b750b41 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -207,6 +207,18 @@ void SecureSessionMgr::ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric) } } +void SecureSessionMgr::ExpireAllPairingsForFabric(FabricIndex fabric) +{ + ChipLogDetail(Inet, "Expiring all connections for fabric %d!!", fabric); + PeerConnectionState * state = mPeerConnections.FindPeerConnectionStateByFabric(fabric, nullptr); + while (state != nullptr) + { + mPeerConnections.MarkConnectionExpired( + state, [this](const Transport::PeerConnectionState & state1) { HandleConnectionExpired(state1); }); + state = mPeerConnections.FindPeerConnectionStateByFabric(fabric, nullptr); + } +} + CHIP_ERROR SecureSessionMgr::NewPairing(const Optional & peerAddr, NodeId peerNodeId, PairingSession * pairing, SecureSession::SessionRole direction, FabricIndex fabric) { diff --git a/src/transport/SecureSessionMgr.h b/src/transport/SecureSessionMgr.h index 4f3427a8843a7c..740bae8fe1995a 100644 --- a/src/transport/SecureSessionMgr.h +++ b/src/transport/SecureSessionMgr.h @@ -225,6 +225,7 @@ class DLL_EXPORT SecureSessionMgr : public TransportMgrDelegate void ExpirePairing(SessionHandle session); void ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric); + void ExpireAllPairingsForFabric(FabricIndex fabric); /** * @brief diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 6ada916ed52cad..c56dc61756d29d 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -19786,6 +19786,9 @@ class OperationalCredentialsCluster : public TestCommand case 1: err = TestSendClusterOperationalCredentialsCommandReadAttribute_1(); break; + case 2: + err = TestSendClusterOperationalCredentialsCommandRemoveFabric_2(); + break; } if (CHIP_NO_ERROR != err) @@ -19797,7 +19800,7 @@ class OperationalCredentialsCluster : public TestCommand private: std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = 2; + const uint16_t mTestCount = 3; // // Tests methods @@ -19935,6 +19938,65 @@ class OperationalCredentialsCluster : public TestCommand runner->NextTest(); } + + // Test Remove fabric + using SuccessCallback_2 = void (*)(void * context, uint8_t statusCode, uint8_t fabricIndex, chip::ByteSpan debugText); + chip::Callback::Callback mOnSuccessCallback_2{ + OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_SuccessResponse, this + }; + chip::Callback::Callback mOnFailureCallback_2{ + OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_FailureResponse, this + }; + bool mIsFailureExpected_2 = 0; + + CHIP_ERROR TestSendClusterOperationalCredentialsCommandRemoveFabric_2() + { + ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Sending command..."); + + chip::Controller::OperationalCredentialsCluster cluster; + cluster.Associate(mDevice, 0); + + CHIP_ERROR err = CHIP_NO_ERROR; + + uint8_t fabricIndexArgument = 1; + err = cluster.RemoveFabric(mOnSuccessCallback_2.Cancel(), mOnFailureCallback_2.Cancel(), fabricIndexArgument); + + return err; + } + + static void OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_FailureResponse(void * context, uint8_t status) + { + ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Failure Response"); + + OperationalCredentialsCluster * runner = reinterpret_cast(context); + + if (runner->mIsFailureExpected_2 == false) + { + ChipLogError(chipTool, "Error: The test was expecting a success callback. Got failure callback"); + runner->SetCommandExitStatus(CHIP_ERROR_INTERNAL); + return; + } + + runner->NextTest(); + } + + static void OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_SuccessResponse(void * context, uint8_t statusCode, + uint8_t fabricIndex, + chip::ByteSpan debugText) + { + ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Success Response"); + + OperationalCredentialsCluster * runner = reinterpret_cast(context); + + if (runner->mIsFailureExpected_2 == true) + { + ChipLogError(chipTool, "Error: The test was expecting a failure callback. Got success callback"); + runner->SetCommandExitStatus(CHIP_ERROR_INTERNAL); + return; + } + + runner->NextTest(); + } }; void registerCommandsTests(Commands & commands) diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp index 541efcc26190ca..25aefbbc8fea4d 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp @@ -101,6 +101,26 @@ CHIP_ERROR OnOffCluster::ReadAttributeOnOff(Callback::Cancelable * onSuccessCall BasicAttributeFilter); } +CHIP_ERROR OnOffCluster::ConfigureAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, + uint16_t minInterval, uint16_t maxInterval) +{ + COMMAND_HEADER("ReportOnOffOnOff", OnOff::Id); + buf.Put8(kFrameControlGlobalCommand) + .Put8(seqNum) + .Put32(Globals::Commands::Ids::ConfigureReporting) + .Put8(kReportingDirectionReported) + .Put32(OnOff::Attributes::Ids::OnOff) + .Put8(16) + .Put16(minInterval) + .Put16(maxInterval); + COMMAND_FOOTER(); +} + +CHIP_ERROR OnOffCluster::ReportAttributeOnOff(Callback::Cancelable * onReportCallback) +{ + return RequestAttributeReporting(0x0000, onReportCallback); +} + CHIP_ERROR OnOffCluster::ReadAttributeGlobalSceneControl(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) { diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h index d24dd428ebc3f4..c931505c69bc34 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h @@ -50,6 +50,9 @@ class DLL_EXPORT OnOffCluster : public ClusterBase uint16_t value); CHIP_ERROR WriteAttributeStartUpOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint8_t value); + CHIP_ERROR ConfigureAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, + uint16_t minInterval, uint16_t maxInterval); + CHIP_ERROR ReportAttributeOnOff(Callback::Cancelable * onReportCallback); }; } // namespace Controller From 078736fdf1bf774e6bfb8fc2e90fc6d6bb7e6c1e Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Thu, 9 Sep 2021 10:40:05 -0700 Subject: [PATCH 2/8] regen zap files --- .../zap-generated/CHIPClusters.cpp | 114 ++++++++++++++++++ .../lighting-app/zap-generated/CHIPClusters.h | 7 ++ .../zap-generated/IMClusterCommandHandler.cpp | 53 +------- 3 files changed, 122 insertions(+), 52 deletions(-) diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp index 25aefbbc8fea4d..c2eb0329cbe2a9 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp @@ -82,6 +82,120 @@ namespace Controller { // TODO(#4503): Commands should take group id as an argument. // OnOff Cluster Commands +CHIP_ERROR OnOffCluster::Off(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandSender * sender = nullptr; + TLV::TLVWriter * writer = nullptr; + uint8_t argSeqNumber = 0; + + // Used when encoding non-empty command. Suppress error message when encoding empty commands. + (void) writer; + (void) argSeqNumber; + + VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); + + app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::Off, + (app::CommandPathFlags::kEndpointIdValid) }; + + SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); + + SuccessOrExit(err = sender->PrepareCommand(cmdParams)); + + // Command takes no arguments. + + SuccessOrExit(err = sender->FinishCommand()); + + // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. + mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); + + err = mDevice->SendCommands(sender); + +exit: + // On error, we are responsible to close the sender. + if (err != CHIP_NO_ERROR && sender != nullptr) + { + sender->Shutdown(); + } + return err; +} + +CHIP_ERROR OnOffCluster::On(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandSender * sender = nullptr; + TLV::TLVWriter * writer = nullptr; + uint8_t argSeqNumber = 0; + + // Used when encoding non-empty command. Suppress error message when encoding empty commands. + (void) writer; + (void) argSeqNumber; + + VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); + + app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::On, + (app::CommandPathFlags::kEndpointIdValid) }; + + SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); + + SuccessOrExit(err = sender->PrepareCommand(cmdParams)); + + // Command takes no arguments. + + SuccessOrExit(err = sender->FinishCommand()); + + // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. + mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); + + err = mDevice->SendCommands(sender); + +exit: + // On error, we are responsible to close the sender. + if (err != CHIP_NO_ERROR && sender != nullptr) + { + sender->Shutdown(); + } + return err; +} + +CHIP_ERROR OnOffCluster::Toggle(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + app::CommandSender * sender = nullptr; + TLV::TLVWriter * writer = nullptr; + uint8_t argSeqNumber = 0; + + // Used when encoding non-empty command. Suppress error message when encoding empty commands. + (void) writer; + (void) argSeqNumber; + + VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); + + app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::Toggle, + (app::CommandPathFlags::kEndpointIdValid) }; + + SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); + + SuccessOrExit(err = sender->PrepareCommand(cmdParams)); + + // Command takes no arguments. + + SuccessOrExit(err = sender->FinishCommand()); + + // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. + mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); + + err = mDevice->SendCommands(sender); + +exit: + // On error, we are responsible to close the sender. + if (err != CHIP_NO_ERROR && sender != nullptr) + { + sender->Shutdown(); + } + return err; +} + // OnOff Cluster Attributes CHIP_ERROR OnOffCluster::DiscoverAttributes(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) { diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h index c931505c69bc34..56c44c317b344b 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h @@ -36,6 +36,11 @@ class DLL_EXPORT OnOffCluster : public ClusterBase OnOffCluster() : ClusterBase(app::Clusters::OnOff::Id) {} ~OnOffCluster() {} + // Cluster Commands + CHIP_ERROR Off(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); + CHIP_ERROR On(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); + CHIP_ERROR Toggle(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); + // Cluster Attributes CHIP_ERROR DiscoverAttributes(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); CHIP_ERROR ReadAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); @@ -53,6 +58,8 @@ class DLL_EXPORT OnOffCluster : public ClusterBase CHIP_ERROR ConfigureAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval); CHIP_ERROR ReportAttributeOnOff(Callback::Cancelable * onReportCallback); + +private: }; } // namespace Controller diff --git a/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp b/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp index 85daa2b646df31..2d3353ac056ea4 100644 --- a/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp +++ b/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp @@ -3223,58 +3223,7 @@ namespace OnOff { void DispatchServerCommand(CommandHandler * apCommandObj, CommandId aCommandId, EndpointId aEndpointId, TLV::TLVReader & aDataTlv) { - // We are using TLVUnpackError and TLVError here since both of them can be CHIP_END_OF_TLV - // When TLVError is CHIP_END_OF_TLV, it means we have iterated all of the items, which is not a real error. - // Any error value TLVUnpackError means we have received an illegal value. - // The following variables are used for all commands to save code size. - CHIP_ERROR TLVError = CHIP_NO_ERROR; - CHIP_ERROR TLVUnpackError = CHIP_NO_ERROR; - uint32_t validArgumentCount = 0; - uint32_t expectArgumentCount = 0; - uint32_t currentDecodeTagId = 0; - bool wasHandled = false; - { - switch (aCommandId) - { - case Clusters::OnOff::Commands::Ids::Off: { - - wasHandled = emberAfOnOffClusterOffCallback(aEndpointId, apCommandObj); - break; - } - case Clusters::OnOff::Commands::Ids::On: { - - wasHandled = emberAfOnOffClusterOnCallback(aEndpointId, apCommandObj); - break; - } - case Clusters::OnOff::Commands::Ids::Toggle: { - - wasHandled = emberAfOnOffClusterToggleCallback(aEndpointId, apCommandObj); - break; - } - default: { - // Unrecognized command ID, error status will apply. - ReportCommandUnsupported(apCommandObj, aEndpointId, Clusters::OnOff::Id, aCommandId); - return; - } - } - } - - if (CHIP_NO_ERROR != TLVError || CHIP_NO_ERROR != TLVUnpackError || expectArgumentCount != validArgumentCount || !wasHandled) - { - CommandPathParams returnStatusParam = { aEndpointId, - 0, // GroupId - Clusters::OnOff::Id, aCommandId, (CommandPathFlags::kEndpointIdValid) }; - apCommandObj->AddStatusCode(returnStatusParam, Protocols::SecureChannel::GeneralStatusCode::kBadRequest, - Protocols::SecureChannel::Id, Protocols::InteractionModel::ProtocolCode::InvalidCommand); - ChipLogProgress(Zcl, - "Failed to dispatch command, %" PRIu32 "/%" PRIu32 " arguments parsed, TLVError=%" CHIP_ERROR_FORMAT - ", UnpackError=%" CHIP_ERROR_FORMAT " (last decoded tag = %" PRIu32, - validArgumentCount, expectArgumentCount, TLVError.Format(), TLVUnpackError.Format(), currentDecodeTagId); - // A command with no arguments would never write currentDecodeTagId. If - // progress logging is also disabled, it would look unused. Silence that - // warning. - UNUSED_VAR(currentDecodeTagId); - } + ReportCommandUnsupported(apCommandObj, aEndpointId, Clusters::OnOff::Id, aCommandId); } } // namespace OnOff From c9b7b4a32e60cd4b9471e0c529bc3097c3558e0b Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Thu, 9 Sep 2021 11:05:08 -0700 Subject: [PATCH 3/8] another zap regen --- .../zap-generated/CHIPClusters.cpp | 134 ------------------ .../lighting-app/zap-generated/CHIPClusters.h | 10 -- .../zap-generated/IMClusterCommandHandler.cpp | 53 ++++++- 3 files changed, 52 insertions(+), 145 deletions(-) diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp index c2eb0329cbe2a9..541efcc26190ca 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.cpp @@ -82,120 +82,6 @@ namespace Controller { // TODO(#4503): Commands should take group id as an argument. // OnOff Cluster Commands -CHIP_ERROR OnOffCluster::Off(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) -{ - CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandSender * sender = nullptr; - TLV::TLVWriter * writer = nullptr; - uint8_t argSeqNumber = 0; - - // Used when encoding non-empty command. Suppress error message when encoding empty commands. - (void) writer; - (void) argSeqNumber; - - VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); - - app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::Off, - (app::CommandPathFlags::kEndpointIdValid) }; - - SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); - - SuccessOrExit(err = sender->PrepareCommand(cmdParams)); - - // Command takes no arguments. - - SuccessOrExit(err = sender->FinishCommand()); - - // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. - mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); - - err = mDevice->SendCommands(sender); - -exit: - // On error, we are responsible to close the sender. - if (err != CHIP_NO_ERROR && sender != nullptr) - { - sender->Shutdown(); - } - return err; -} - -CHIP_ERROR OnOffCluster::On(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) -{ - CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandSender * sender = nullptr; - TLV::TLVWriter * writer = nullptr; - uint8_t argSeqNumber = 0; - - // Used when encoding non-empty command. Suppress error message when encoding empty commands. - (void) writer; - (void) argSeqNumber; - - VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); - - app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::On, - (app::CommandPathFlags::kEndpointIdValid) }; - - SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); - - SuccessOrExit(err = sender->PrepareCommand(cmdParams)); - - // Command takes no arguments. - - SuccessOrExit(err = sender->FinishCommand()); - - // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. - mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); - - err = mDevice->SendCommands(sender); - -exit: - // On error, we are responsible to close the sender. - if (err != CHIP_NO_ERROR && sender != nullptr) - { - sender->Shutdown(); - } - return err; -} - -CHIP_ERROR OnOffCluster::Toggle(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) -{ - CHIP_ERROR err = CHIP_NO_ERROR; - app::CommandSender * sender = nullptr; - TLV::TLVWriter * writer = nullptr; - uint8_t argSeqNumber = 0; - - // Used when encoding non-empty command. Suppress error message when encoding empty commands. - (void) writer; - (void) argSeqNumber; - - VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); - - app::CommandPathParams cmdParams = { mEndpoint, /* group id */ 0, mClusterId, OnOff::Commands::Ids::Toggle, - (app::CommandPathFlags::kEndpointIdValid) }; - - SuccessOrExit(err = app::InteractionModelEngine::GetInstance()->NewCommandSender(&sender)); - - SuccessOrExit(err = sender->PrepareCommand(cmdParams)); - - // Command takes no arguments. - - SuccessOrExit(err = sender->FinishCommand()); - - // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate. - mDevice->AddIMResponseHandler(sender, onSuccessCallback, onFailureCallback); - - err = mDevice->SendCommands(sender); - -exit: - // On error, we are responsible to close the sender. - if (err != CHIP_NO_ERROR && sender != nullptr) - { - sender->Shutdown(); - } - return err; -} - // OnOff Cluster Attributes CHIP_ERROR OnOffCluster::DiscoverAttributes(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) { @@ -215,26 +101,6 @@ CHIP_ERROR OnOffCluster::ReadAttributeOnOff(Callback::Cancelable * onSuccessCall BasicAttributeFilter); } -CHIP_ERROR OnOffCluster::ConfigureAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, - uint16_t minInterval, uint16_t maxInterval) -{ - COMMAND_HEADER("ReportOnOffOnOff", OnOff::Id); - buf.Put8(kFrameControlGlobalCommand) - .Put8(seqNum) - .Put32(Globals::Commands::Ids::ConfigureReporting) - .Put8(kReportingDirectionReported) - .Put32(OnOff::Attributes::Ids::OnOff) - .Put8(16) - .Put16(minInterval) - .Put16(maxInterval); - COMMAND_FOOTER(); -} - -CHIP_ERROR OnOffCluster::ReportAttributeOnOff(Callback::Cancelable * onReportCallback) -{ - return RequestAttributeReporting(0x0000, onReportCallback); -} - CHIP_ERROR OnOffCluster::ReadAttributeGlobalSceneControl(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback) { diff --git a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h index 56c44c317b344b..d24dd428ebc3f4 100644 --- a/zzz_generated/lighting-app/zap-generated/CHIPClusters.h +++ b/zzz_generated/lighting-app/zap-generated/CHIPClusters.h @@ -36,11 +36,6 @@ class DLL_EXPORT OnOffCluster : public ClusterBase OnOffCluster() : ClusterBase(app::Clusters::OnOff::Id) {} ~OnOffCluster() {} - // Cluster Commands - CHIP_ERROR Off(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); - CHIP_ERROR On(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); - CHIP_ERROR Toggle(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); - // Cluster Attributes CHIP_ERROR DiscoverAttributes(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); CHIP_ERROR ReadAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback); @@ -55,11 +50,6 @@ class DLL_EXPORT OnOffCluster : public ClusterBase uint16_t value); CHIP_ERROR WriteAttributeStartUpOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint8_t value); - CHIP_ERROR ConfigureAttributeOnOff(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, - uint16_t minInterval, uint16_t maxInterval); - CHIP_ERROR ReportAttributeOnOff(Callback::Cancelable * onReportCallback); - -private: }; } // namespace Controller diff --git a/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp b/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp index 2d3353ac056ea4..85daa2b646df31 100644 --- a/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp +++ b/zzz_generated/lighting-app/zap-generated/IMClusterCommandHandler.cpp @@ -3223,7 +3223,58 @@ namespace OnOff { void DispatchServerCommand(CommandHandler * apCommandObj, CommandId aCommandId, EndpointId aEndpointId, TLV::TLVReader & aDataTlv) { - ReportCommandUnsupported(apCommandObj, aEndpointId, Clusters::OnOff::Id, aCommandId); + // We are using TLVUnpackError and TLVError here since both of them can be CHIP_END_OF_TLV + // When TLVError is CHIP_END_OF_TLV, it means we have iterated all of the items, which is not a real error. + // Any error value TLVUnpackError means we have received an illegal value. + // The following variables are used for all commands to save code size. + CHIP_ERROR TLVError = CHIP_NO_ERROR; + CHIP_ERROR TLVUnpackError = CHIP_NO_ERROR; + uint32_t validArgumentCount = 0; + uint32_t expectArgumentCount = 0; + uint32_t currentDecodeTagId = 0; + bool wasHandled = false; + { + switch (aCommandId) + { + case Clusters::OnOff::Commands::Ids::Off: { + + wasHandled = emberAfOnOffClusterOffCallback(aEndpointId, apCommandObj); + break; + } + case Clusters::OnOff::Commands::Ids::On: { + + wasHandled = emberAfOnOffClusterOnCallback(aEndpointId, apCommandObj); + break; + } + case Clusters::OnOff::Commands::Ids::Toggle: { + + wasHandled = emberAfOnOffClusterToggleCallback(aEndpointId, apCommandObj); + break; + } + default: { + // Unrecognized command ID, error status will apply. + ReportCommandUnsupported(apCommandObj, aEndpointId, Clusters::OnOff::Id, aCommandId); + return; + } + } + } + + if (CHIP_NO_ERROR != TLVError || CHIP_NO_ERROR != TLVUnpackError || expectArgumentCount != validArgumentCount || !wasHandled) + { + CommandPathParams returnStatusParam = { aEndpointId, + 0, // GroupId + Clusters::OnOff::Id, aCommandId, (CommandPathFlags::kEndpointIdValid) }; + apCommandObj->AddStatusCode(returnStatusParam, Protocols::SecureChannel::GeneralStatusCode::kBadRequest, + Protocols::SecureChannel::Id, Protocols::InteractionModel::ProtocolCode::InvalidCommand); + ChipLogProgress(Zcl, + "Failed to dispatch command, %" PRIu32 "/%" PRIu32 " arguments parsed, TLVError=%" CHIP_ERROR_FORMAT + ", UnpackError=%" CHIP_ERROR_FORMAT " (last decoded tag = %" PRIu32, + validArgumentCount, expectArgumentCount, TLVError.Format(), TLVUnpackError.Format(), currentDecodeTagId); + // A command with no arguments would never write currentDecodeTagId. If + // progress logging is also disabled, it would look unused. Silence that + // warning. + UNUSED_VAR(currentDecodeTagId); + } } } // namespace OnOff From 4f801517f9588d24b7cdbad154d26cc92f058b4d Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Thu, 9 Sep 2021 17:12:23 -0700 Subject: [PATCH 4/8] address review comments --- .../operational-credentials-server.cpp | 40 ++++++++---- .../suites/OperationalCredentialsCluster.yaml | 5 ++ .../Framework/CHIPTests/CHIPClustersTests.m | 19 ------ src/transport/PeerConnections.h | 10 +-- src/transport/SecureSessionMgr.cpp | 4 +- .../chip-tool/zap-generated/test/Commands.h | 64 +------------------ 6 files changed, 37 insertions(+), 105 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 3273dc85a5f73d..676d1d6daf765f 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -239,43 +239,57 @@ void emberAfPluginOperationalCredentialsServerInitCallback(void) } namespace { -class OpCredsClusterExchangeDelegate : public Messaging::ExchangeDelegate +class FabricCleanupExchangeDelegate : public Messaging::ExchangeDelegate { public: - CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, - const PayloadHeader & payloadHeader, System::PacketBufferHandle && payload) override + CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader, + System::PacketBufferHandle && payload) override { return CHIP_NO_ERROR; } void OnResponseTimeout(Messaging::ExchangeContext * ec) override {} void OnExchangeClosing(Messaging::ExchangeContext * ec) override { - ec->GetExchangeMgr()->GetReliableMessageMgr()->ClearRetransTable(static_cast(ec)); - ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(mFabricIndex); - mFabricIndex = 0; + FabricIndex currentFabricIndex = ec->GetSecureSession().GetFabricIndex(); + ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(currentFabricIndex); } - - FabricIndex mFabricIndex = 0; }; -OpCredsClusterExchangeDelegate gFabricCleanupExchangeDelegate; +FabricCleanupExchangeDelegate gFabricCleanupExchangeDelegate; } // namespace bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoint, app::CommandHandler * commandObj, - FabricIndex fabricIndex) + FabricIndex fabricBeingRemoved) { emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: RemoveFabric"); // TODO: Generate emberAfFabricClusterPrintln + FabricIndex currentFabricIndex = emberAfCurrentCommand()->source->GetSecureSession().GetFabricIndex(); + EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS; - CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricIndex); + CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricBeingRemoved); VerifyOrExit(err == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_FAILURE); exit: writeFabricsIntoFabricsListAttribute(); emberAfSendImmediateDefaultResponse(status); - gFabricCleanupExchangeDelegate.mFabricIndex = fabricIndex; - emberAfCurrentCommand()->source->SetDelegate(&gFabricCleanupExchangeDelegate); + if (err == CHIP_NO_ERROR) + { + Messaging::ExchangeContext * ec = emberAfCurrentCommand()->source; + if (currentFabricIndex == fabricBeingRemoved) + { + // If the current fabric is being removed, expiring all the secure sessions causes crashes as + // the message sent by emberAfSendImmediateDefaultResponse() is still in the queue. Also, RMP + // retries to send the message and runs into issues. + // We are hijacking the exchange delegate here (as no more messages should be received on this exchange), + // and wait for it to close, before expiring the secure sessions for the fabric. + ec->SetDelegate(&gFabricCleanupExchangeDelegate); + } + else + { + ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(fabricBeingRemoved); + } + } return true; } diff --git a/src/app/tests/suites/OperationalCredentialsCluster.yaml b/src/app/tests/suites/OperationalCredentialsCluster.yaml index fd2d1f2ff8168a..d025f11728a5a4 100644 --- a/src/app/tests/suites/OperationalCredentialsCluster.yaml +++ b/src/app/tests/suites/OperationalCredentialsCluster.yaml @@ -35,7 +35,12 @@ tests: type: uint8 minValue: 1 + # This test is currently disabled as it breaks on Darwin. + # The test removes the current fabric, and Darwin test runner reuses + # the same pairing to run all the tests. Due to that, all subsequent + # tests fail. - label: "Remove fabric" + disabled: true command: "RemoveFabric" arguments: values: diff --git a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m index bd8562e3bf87c4..2ca603e6966ca3 100644 --- a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m +++ b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m @@ -5246,25 +5246,6 @@ - (void)testSendClusterOperationalCredentialsCluster_000001_ReadAttribute [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; } -- (void)testSendClusterOperationalCredentialsCluster_000002_RemoveFabric -{ - XCTestExpectation * expectation = [self expectationWithDescription:@"Remove fabric"]; - CHIPDevice * device = GetPairedDevice(kDeviceId); - dispatch_queue_t queue = dispatch_get_main_queue(); - CHIPOperationalCredentials * cluster = [[CHIPOperationalCredentials alloc] initWithDevice:device endpoint:0 queue:queue]; - XCTAssertNotNil(cluster); - - uint8_t fabricIndexArgument = 1; - [cluster removeFabric:fabricIndexArgument - responseHandler:^(NSError * err, NSDictionary * values) { - NSLog(@"Remove fabric Error: %@", err); - - XCTAssertEqual(err.code, 0); - [expectation fulfill]; - }]; - - [self waitForExpectationsWithTimeout:kTimeoutInSeconds handler:nil]; -} - (void)testSendClusterAccountLoginReadAttributeClusterRevisionWithResponseHandler { diff --git a/src/transport/PeerConnections.h b/src/transport/PeerConnections.h index 43097e2d2a4397..f4f5377fc214c0 100644 --- a/src/transport/PeerConnections.h +++ b/src/transport/PeerConnections.h @@ -317,24 +317,18 @@ class PeerConnections } /** - * Get a peer connection state that matches the given fabric index. + * Get the first peer connection state that matches the given fabric index. * * @param fabric The fabric index to match - * @param begin If a member of the pool, will start search from the next item. Can be nullptr to search from start. * * @return the state found, nullptr if not found */ CHECK_RETURN_VALUE - PeerConnectionState * FindPeerConnectionStateByFabric(FabricIndex fabric, PeerConnectionState * begin) + PeerConnectionState * FindPeerConnectionStateByFabric(FabricIndex fabric) { PeerConnectionState * state = nullptr; PeerConnectionState * iter = &mStates[0]; - if (begin >= iter && begin < &mStates[kMaxConnectionCount]) - { - iter = begin + 1; - } - for (; iter < &mStates[kMaxConnectionCount]; iter++) { if (!iter->IsInitialized()) diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index 6fa2612b750b41..093ce844e19ef4 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -210,12 +210,12 @@ void SecureSessionMgr::ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric) void SecureSessionMgr::ExpireAllPairingsForFabric(FabricIndex fabric) { ChipLogDetail(Inet, "Expiring all connections for fabric %d!!", fabric); - PeerConnectionState * state = mPeerConnections.FindPeerConnectionStateByFabric(fabric, nullptr); + PeerConnectionState * state = mPeerConnections.FindPeerConnectionStateByFabric(fabric); while (state != nullptr) { mPeerConnections.MarkConnectionExpired( state, [this](const Transport::PeerConnectionState & state1) { HandleConnectionExpired(state1); }); - state = mPeerConnections.FindPeerConnectionStateByFabric(fabric, nullptr); + state = mPeerConnections.FindPeerConnectionStateByFabric(fabric); } } diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index c56dc61756d29d..6ada916ed52cad 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -19786,9 +19786,6 @@ class OperationalCredentialsCluster : public TestCommand case 1: err = TestSendClusterOperationalCredentialsCommandReadAttribute_1(); break; - case 2: - err = TestSendClusterOperationalCredentialsCommandRemoveFabric_2(); - break; } if (CHIP_NO_ERROR != err) @@ -19800,7 +19797,7 @@ class OperationalCredentialsCluster : public TestCommand private: std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = 3; + const uint16_t mTestCount = 2; // // Tests methods @@ -19938,65 +19935,6 @@ class OperationalCredentialsCluster : public TestCommand runner->NextTest(); } - - // Test Remove fabric - using SuccessCallback_2 = void (*)(void * context, uint8_t statusCode, uint8_t fabricIndex, chip::ByteSpan debugText); - chip::Callback::Callback mOnSuccessCallback_2{ - OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_SuccessResponse, this - }; - chip::Callback::Callback mOnFailureCallback_2{ - OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_FailureResponse, this - }; - bool mIsFailureExpected_2 = 0; - - CHIP_ERROR TestSendClusterOperationalCredentialsCommandRemoveFabric_2() - { - ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Sending command..."); - - chip::Controller::OperationalCredentialsCluster cluster; - cluster.Associate(mDevice, 0); - - CHIP_ERROR err = CHIP_NO_ERROR; - - uint8_t fabricIndexArgument = 1; - err = cluster.RemoveFabric(mOnSuccessCallback_2.Cancel(), mOnFailureCallback_2.Cancel(), fabricIndexArgument); - - return err; - } - - static void OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_FailureResponse(void * context, uint8_t status) - { - ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Failure Response"); - - OperationalCredentialsCluster * runner = reinterpret_cast(context); - - if (runner->mIsFailureExpected_2 == false) - { - ChipLogError(chipTool, "Error: The test was expecting a success callback. Got failure callback"); - runner->SetCommandExitStatus(CHIP_ERROR_INTERNAL); - return; - } - - runner->NextTest(); - } - - static void OnTestSendClusterOperationalCredentialsCommandRemoveFabric_2_SuccessResponse(void * context, uint8_t statusCode, - uint8_t fabricIndex, - chip::ByteSpan debugText) - { - ChipLogProgress(chipTool, "Operational Credentials - Remove fabric: Success Response"); - - OperationalCredentialsCluster * runner = reinterpret_cast(context); - - if (runner->mIsFailureExpected_2 == true) - { - ChipLogError(chipTool, "Error: The test was expecting a failure callback. Got success callback"); - runner->SetCommandExitStatus(CHIP_ERROR_INTERNAL); - return; - } - - runner->NextTest(); - } }; void registerCommandsTests(Commands & commands) From fd5a925e0605a4276b6f3f648090711798d23939 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 13 Sep 2021 12:45:32 -0700 Subject: [PATCH 5/8] address review comments --- .../operational-credentials-server.cpp | 3 +++ src/transport/PeerConnections.h | 14 +++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 676d1d6daf765f..166d3af3cb06dc 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -264,6 +264,8 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoin { emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: RemoveFabric"); // TODO: Generate emberAfFabricClusterPrintln + // We are extracting the current fabric index here, even though it's used after sending the response. + // This is done to prevent accidental update of emberAfCurrentCommand()->source while sending the command's response. FabricIndex currentFabricIndex = emberAfCurrentCommand()->source->GetSecureSession().GetFabricIndex(); EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS; @@ -283,6 +285,7 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoin // retries to send the message and runs into issues. // We are hijacking the exchange delegate here (as no more messages should be received on this exchange), // and wait for it to close, before expiring the secure sessions for the fabric. + // TODO: https://github.com/project-chip/connectedhomeip/issues/9642 ec->SetDelegate(&gFabricCleanupExchangeDelegate); } else diff --git a/src/transport/PeerConnections.h b/src/transport/PeerConnections.h index f4f5377fc214c0..17093b84e7ef65 100644 --- a/src/transport/PeerConnections.h +++ b/src/transport/PeerConnections.h @@ -326,22 +326,18 @@ class PeerConnections CHECK_RETURN_VALUE PeerConnectionState * FindPeerConnectionStateByFabric(FabricIndex fabric) { - PeerConnectionState * state = nullptr; - PeerConnectionState * iter = &mStates[0]; - - for (; iter < &mStates[kMaxConnectionCount]; iter++) + for (auto & state : mStates) { - if (!iter->IsInitialized()) + if (!state.IsInitialized()) { continue; } - if (iter->GetFabricIndex() == fabric) + if (state.GetFabricIndex() == fabric) { - state = iter; - break; + return &state; } } - return state; + return nullptr; } /// Convenience method to mark a peer connection state as active From b684a68ac06eef320e8d082e9e0e8a6992ecc3a8 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 13 Sep 2021 13:29:26 -0700 Subject: [PATCH 6/8] updates --- .../operational-credentials-server.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 166d3af3cb06dc..68a84e83e63697 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -264,10 +264,6 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoin { emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: RemoveFabric"); // TODO: Generate emberAfFabricClusterPrintln - // We are extracting the current fabric index here, even though it's used after sending the response. - // This is done to prevent accidental update of emberAfCurrentCommand()->source while sending the command's response. - FabricIndex currentFabricIndex = emberAfCurrentCommand()->source->GetSecureSession().GetFabricIndex(); - EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS; CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricBeingRemoved); VerifyOrExit(err == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_FAILURE); @@ -277,7 +273,8 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoin emberAfSendImmediateDefaultResponse(status); if (err == CHIP_NO_ERROR) { - Messaging::ExchangeContext * ec = emberAfCurrentCommand()->source; + Messaging::ExchangeContext * ec = commandObj->GetExchangeContext(); + FabricIndex currentFabricIndex = ec->GetSecureSession().GetFabricIndex(); if (currentFabricIndex == fabricBeingRemoved) { // If the current fabric is being removed, expiring all the secure sessions causes crashes as From 00419dc80fce5f8914b6c8eb3b3c389765c0c1fd Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 13 Sep 2021 14:04:09 -0700 Subject: [PATCH 7/8] update CASESession error handling --- src/protocols/secure_channel/CASESession.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 4198f2f1259543..471271adcce0cb 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1035,8 +1035,10 @@ void CASESession::SendErrorMsg(SigmaErrorType errorCode) msg->SetDataLength(msglen); - VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) == CHIP_NO_ERROR, - ChipLogError(SecureChannel, "Failed to send error message")); + if (mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) != CHIP_NO_ERROR) + { + ChipLogError(SecureChannel, "Failed to send error message")); + } } CHIP_ERROR CASESession::ConstructSaltSigmaR2(const ByteSpan & rand, const Crypto::P256PublicKey & pubkey, const ByteSpan & ipk, From 626e657f3260636831478222e5887af9d66f3da3 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 13 Sep 2021 14:19:48 -0700 Subject: [PATCH 8/8] fix build error --- src/protocols/secure_channel/CASESession.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 471271adcce0cb..65c3026a402b7a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1037,7 +1037,7 @@ void CASESession::SendErrorMsg(SigmaErrorType errorCode) if (mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) != CHIP_NO_ERROR) { - ChipLogError(SecureChannel, "Failed to send error message")); + ChipLogError(SecureChannel, "Failed to send error message"); } }