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 <bzbarsky@apple.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
project-chip · Aug 21, 2023 · 7521651 · 7521651
1 parent fa3682c
commit 7521651
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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<TestContext *>(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),