From 92093d7c0f8570262c9ec057dd86c0a122a7ee94 Mon Sep 17 00:00:00 2001 From: yunhanw Date: Wed, 3 Aug 2022 13:37:10 -0700 Subject: [PATCH 1/2] fix subscription id ReadClient should return error when receiving report with subscription Id when current action is read. --- src/app/ReadClient.cpp | 21 +++++++++------ src/app/ReadClient.h | 17 ++++++------ src/app/tests/TestReadInteraction.cpp | 38 +++++++++++++++++++++++++-- 3 files changed, 58 insertions(+), 18 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 007cc6d0192133..6694e271148544 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,12 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) err = report.GetSubscriptionId(&subscriptionId); if (CHIP_NO_ERROR == err) { - if (mIsPrimingReports) + if (!IsSubscriptionType()) + { + err = CHIP_ERROR_INVALID_ARGUMENT; + goto exit; + } + if (mWaitingForFirstPrimingReport) { mSubscriptionId = subscriptionId; } @@ -592,7 +597,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), From 268ceee27117bf89275546be8c307ce9f179d589 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Wed, 3 Aug 2022 14:32:30 -0700 Subject: [PATCH 2/2] Update src/app/ReadClient.cpp Co-authored-by: Boris Zbarsky --- src/app/ReadClient.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 6694e271148544..be5b7359d38d1a 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -504,11 +504,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) err = report.GetSubscriptionId(&subscriptionId); if (CHIP_NO_ERROR == err) { - if (!IsSubscriptionType()) - { - err = CHIP_ERROR_INVALID_ARGUMENT; - goto exit; - } + VerifyOrExit(IsSubscriptionType(), err = CHIP_ERROR_INVALID_ARGUMENT); if (mWaitingForFirstPrimingReport) { mSubscriptionId = subscriptionId;