From f32d1c8794296ae02db090788e786e59adc24b35 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Thu, 4 Aug 2022 09:26:08 -0700 Subject: [PATCH] fix subscription id (#21609) * fix subscription id ReadClient should return error when receiving report with subscription Id when current action is read. * Update src/app/ReadClient.cpp Co-authored-by: Boris Zbarsky Co-authored-by: Boris Zbarsky --- src/app/ReadClient.cpp | 17 ++++++------ src/app/ReadClient.h | 17 ++++++------ src/app/tests/TestReadInteraction.cpp | 38 +++++++++++++++++++++++++-- 3 files changed, 54 insertions(+), 18 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 007cc6d0192133..be5b7359d38d1a 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -57,12 +57,12 @@ ReadClient::ReadClient(InteractionModelEngine * apImEngine, Messaging::ExchangeM void ReadClient::ClearActiveSubscriptionState() { - mIsReporting = false; - mIsPrimingReports = true; - mPendingMoreChunks = false; - mMinIntervalFloorSeconds = 0; - mMaxInterval = 0; - mSubscriptionId = 0; + mIsReporting = false; + mWaitingForFirstPrimingReport = true; + mPendingMoreChunks = false; + mMinIntervalFloorSeconds = 0; + mMaxInterval = 0; + mSubscriptionId = 0; MoveToState(ClientState::Idle); } @@ -504,7 +504,8 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) err = report.GetSubscriptionId(&subscriptionId); if (CHIP_NO_ERROR == err) { - if (mIsPrimingReports) + VerifyOrExit(IsSubscriptionType(), err = CHIP_ERROR_INVALID_ARGUMENT); + if (mWaitingForFirstPrimingReport) { mSubscriptionId = subscriptionId; } @@ -592,7 +593,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) err = StatusResponse::Send(Status::Success, mExchange.Get(), !noResponseExpected); } - mIsPrimingReports = false; + mWaitingForFirstPrimingReport = false; return err; } diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 6a4f1505b5d7a8..f5c516243b4d16 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -496,14 +496,15 @@ class ReadClient : public Messaging::ExchangeDelegate Messaging::ExchangeManager * mpExchangeMgr = nullptr; Messaging::ExchangeHolder mExchange; Callback & mpCallback; - ClientState mState = ClientState::Idle; - bool mIsReporting = false; - bool mIsInitialReport = true; - bool mIsPrimingReports = true; - bool mPendingMoreChunks = false; - uint16_t mMinIntervalFloorSeconds = 0; - uint16_t mMaxInterval = 0; - SubscriptionId mSubscriptionId = 0; + ClientState mState = ClientState::Idle; + bool mIsReporting = false; + bool mIsInitialReport = true; + // boolean to check if client is waiting for the first priming report + bool mWaitingForFirstPrimingReport = true; + bool mPendingMoreChunks = false; + uint16_t mMinIntervalFloorSeconds = 0; + uint16_t mMaxInterval = 0; + SubscriptionId mSubscriptionId = 0; ScopedNodeId mPeer; InteractionType mInteractionType = InteractionType::Read; Timestamp mEventTimestamp; diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 20921bf72794d4..720ab0f06324a8 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -286,6 +286,7 @@ class TestReadInteraction { public: static void TestReadClient(nlTestSuite * apSuite, void * apContext); + static void TestReadUnexpectedSubscriptionId(nlTestSuite * apSuite, void * apContext); static void TestReadHandler(nlTestSuite * apSuite, void * apContext); static void TestReadClientGenerateAttributePathList(nlTestSuite * apSuite, void * apContext); static void TestReadClientGenerateInvalidAttributePathList(nlTestSuite * apSuite, void * apContext); @@ -331,11 +332,11 @@ class TestReadInteraction private: static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport, bool aSuppressResponse); + bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId); }; void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport, bool aSuppressResponse) + bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId = false) { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVWriter writer; @@ -346,6 +347,12 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon err = reportDataMessageBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + if (aHasSubscriptionId) + { + reportDataMessageBuilder.SubscriptionId(1); + NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); + } + AttributeReportIBs::Builder & attributeReportIBsBuilder = reportDataMessageBuilder.CreateAttributeReportIBs(); NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); @@ -435,6 +442,32 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } +void TestReadInteraction::TestReadUnexpectedSubscriptionId(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + TestContext & ctx = *static_cast(apContext); + MockInteractionModelApp delegate; + app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, + chip::app::ReadClient::InteractionType::Read); + System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); + + ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); + err = readClient.SendRequest(readPrepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // We don't actually want to deliver that message, because we want to + // synthesize the read response. But we don't want it hanging around + // forever either. + ctx.GetLoopback().mNumMessagesToDrop = 1; + ctx.DrainAndServiceIO(); + + // For read, we don't expect there is subscription id in report data. + GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/, + true /*aHasSubscriptionId*/); + err = readClient.ProcessReportData(std::move(buf)); + NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT); +} + void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContext) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -3350,6 +3383,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestReadChunking", chip::app::TestReadInteraction::TestReadChunking), NL_TEST_DEF("TestSetDirtyBetweenChunks", chip::app::TestReadInteraction::TestSetDirtyBetweenChunks), NL_TEST_DEF("CheckReadClient", chip::app::TestReadInteraction::TestReadClient), + NL_TEST_DEF("TestReadUnexpectedSubscriptionId", chip::app::TestReadInteraction::TestReadUnexpectedSubscriptionId), NL_TEST_DEF("CheckReadHandler", chip::app::TestReadInteraction::TestReadHandler), NL_TEST_DEF("TestReadClientGenerateAttributePathList", chip::app::TestReadInteraction::TestReadClientGenerateAttributePathList), NL_TEST_DEF("TestReadClientGenerateInvalidAttributePathList", chip::app::TestReadInteraction::TestReadClientGenerateInvalidAttributePathList),