Revert "Revert "Fix status report handling in write handler (#21440)"… (

#21707)

* Revert "Revert "Fix status report handling in write handler (#21440)" (#21701)"

This reverts commit 5ab5be3.

* Fix nrf test

in nrf test, somehow CONFIG_BUILD_FOR_HOST_UNIT_TEST is disabled,
need to disable this test in nrf tests

apply restyle

* Update src/app/WriteHandler.cpp

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Boris Zbarsky <[email protected]>

src/app/InteractionModelEngine.cpp

@@ -202,6 +202,24 @@ ReadHandler * InteractionModelEngine::ActiveHandlerAt(unsigned int aIndex)
     return ret;
 }
 
+WriteHandler * InteractionModelEngine::ActiveWriteHandlerAt(unsigned int aIndex)
+{
+    unsigned int i = 0;
+
+    for (auto & writeHandler : mWriteHandlers)
+    {
+        if (!writeHandler.IsFree())
+        {
+            if (i == aIndex)
+            {
+                return &writeHandler;
+            }
+            i++;
+        }
+    }
+    return nullptr;
+}
+
 uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const
 {
     uint32_t numActive = 0;

src/app/InteractionModelEngine.h

@@ -159,6 +159,11 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      */
     ReadHandler * ActiveHandlerAt(unsigned int aIndex);
 
+    /**
+     * Returns the write handler at a particular index within the active handler list.
+     */
+    WriteHandler * ActiveWriteHandlerAt(unsigned int aIndex);
+
     /**
      * The Magic number of this InteractionModelEngine, the magic number is set during Init()
      */

src/app/WriteHandler.cpp

@@ -31,7 +31,7 @@ namespace chip {
 namespace app {
 
 using namespace Protocols::InteractionModel;
-
+using Status                         = Protocols::InteractionModel::Status;
 constexpr uint8_t kListAttributeType = 0x48;
 
 CHIP_ERROR WriteHandler::Init()
@@ -118,7 +118,14 @@ CHIP_ERROR WriteHandler::OnMessageReceived(Messaging::ExchangeContext * apExchan
                        "OnMessageReceived should not be called on GroupExchangeContext");
     if (!aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest))
     {
+        if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse))
+        {
+            CHIP_ERROR statusError = CHIP_NO_ERROR;
+            // Parse the status response so we can log it properly.
+            StatusResponse::ProcessStatusResponse(std::move(aPayload), statusError);
+        }
         ChipLogDetail(DataManagement, "Unexpected message type %d", aPayloadHeader.GetMessageType());
+        StatusResponse::Send(Status::InvalidAction, apExchangeContext, false /*aExpectResponse*/);
         Close();
         return CHIP_ERROR_INVALID_MESSAGE_TYPE;
     }
@@ -133,7 +140,7 @@ CHIP_ERROR WriteHandler::OnMessageReceived(Messaging::ExchangeContext * apExchan
             Close();
         }
     }
-    else if (status != Protocols::InteractionModel::Status::Success)
+    else
     {
         err = StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/);
         Close();
@@ -312,7 +319,7 @@ CHIP_ERROR WriteHandler::ProcessAttributeDataIBs(TLV::TLVReader & aAttributeData
             // it with Busy status code.
             (dataAttributePath.IsListItemOperation() && !IsSameAttribute(mProcessingAttributePath, dataAttributePath)))
         {
-            err = AddStatus(dataAttributePath, StatusIB(Protocols::InteractionModel::Status::Busy));
+            err = AddStatus(dataAttributePath, StatusIB(Status::Busy));
             continue;
         }
 
@@ -619,13 +626,11 @@ CHIP_ERROR WriteHandler::AddStatus(const ConcreteDataAttributePath & aPath, cons
 
 CHIP_ERROR WriteHandler::AddClusterSpecificSuccess(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus)
 {
-    using Protocols::InteractionModel::Status;
     return AddStatus(aPath, StatusIB(Status::Success, aClusterStatus));
 }
 
 CHIP_ERROR WriteHandler::AddClusterSpecificFailure(const ConcreteDataAttributePath & aPath, ClusterStatus aClusterStatus)
 {
-    using Protocols::InteractionModel::Status;
     return AddStatus(aPath, StatusIB(Status::Failure, aClusterStatus));
 }
 

src/app/WriteHandler.h

@@ -114,16 +114,18 @@ class WriteHandler : public Messaging::ExchangeDelegate
     }
 
 private:
+    friend class TestWriteInteraction;
     enum class State
     {
         Uninitialized = 0, // The handler has not been initialized
         Initialized,       // The handler has been initialized and is ready
         AddStatus,         // The handler has added status code
         Sending,           // The handler has sent out the write response
     };
-    Protocols::InteractionModel::Status ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite);
-    Protocols::InteractionModel::Status HandleWriteRequestMessage(Messaging::ExchangeContext * apExchangeContext,
-                                                                  System::PacketBufferHandle && aPayload, bool aIsTimedWrite);
+    using Status = Protocols::InteractionModel::Status;
+    Status ProcessWriteRequest(System::PacketBufferHandle && aPayload, bool aIsTimedWrite);
+    Status HandleWriteRequestMessage(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload,
+                                     bool aIsTimedWrite);
 
     CHIP_ERROR FinalizeMessage(System::PacketBufferTLVWriter && aMessageWriter, System::PacketBufferHandle & packet);
     CHIP_ERROR SendWriteResponse(System::PacketBufferTLVWriter && aMessageWriter);

src/app/tests/TestWriteInteraction.cpp

@@ -32,7 +32,9 @@
 #include <messaging/ExchangeContext.h>
 #include <messaging/Flags.h>
 
+#include <memory>
 #include <nlunit-test.h>
+#include <utility>
 
 using TestContext = chip::Test::AppContext;
 
@@ -61,7 +63,9 @@ class TestWriteInteraction
     static void TestWriteRoundtripWithClusterObjects(nlTestSuite * apSuite, void * apContext);
     static void TestWriteRoundtripWithClusterObjectsVersionMatch(nlTestSuite * apSuite, void * apContext);
     static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext);
-
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+    static void TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext);
+#endif
 private:
     static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient);
     static void AddAttributeStatus(nlTestSuite * apSuite, void * apContext, WriteHandler & aWriteHandler);
@@ -90,13 +94,18 @@ class TestWriteClientCallback : public chip::app::WriteClient::Callback
         mStatus = status;
         mOnSuccessCalled++;
     }
-    void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override { mOnErrorCalled++; }
+    void OnError(const WriteClient * apWriteClient, CHIP_ERROR chipError) override
+    {
+        mOnErrorCalled++;
+        mLastErrorReason = app::StatusIB(chipError);
+    }
     void OnDone(WriteClient * apWriteClient) override { mOnDoneCalled++; }
 
     int mOnSuccessCalled = 0;
     int mOnErrorCalled   = 0;
     int mOnDoneCalled    = 0;
     StatusIB mStatus;
+    StatusIB mLastErrorReason;
 };
 
 void TestWriteInteraction::AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient)
@@ -543,6 +552,75 @@ void TestWriteInteraction::TestWriteRoundtrip(nlTestSuite * apSuite, void * apCo
     engine->Shutdown();
 }
 
+// This test creates a chunked write request, we drop the second write chunk message, then write handler receives unknown
+// report message and sends out a status report with invalid action.
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext)
+{
+    TestContext & ctx  = *static_cast<TestContext *>(apContext);
+    auto sessionHandle = ctx.GetSessionBobToAlice();
+
+    app::AttributePathParams attributePath(2, 3, 4);
+
+    CHIP_ERROR err                     = CHIP_NO_ERROR;
+    Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
+    // Shouldn't have anything in the retransmit table when starting the test.
+    NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0);
+
+    TestWriteClientCallback writeCallback;
+    auto * engine = chip::app::InteractionModelEngine::GetInstance();
+    err           = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable());
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+    app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
+                                 static_cast<uint16_t>(900) /* reserved buffer size */);
+
+    ByteSpan list[5];
+
+    err = writeClient.EncodeAttribute(attributePath, app::DataModel::List<ByteSpan>(list, 5));
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+    ctx.GetLoopback().mSentMessageCount                 = 0;
+    ctx.GetLoopback().mNumMessagesToDrop                = 1;
+    ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 2;
+    err                                                 = writeClient.SendWriteRequest(sessionHandle);
+    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+    ctx.DrainAndServiceIO();
+
+    NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 1);
+    NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 3);
+    NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1);
+
+    System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
+    NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
+    System::PacketBufferTLVWriter writer;
+    writer.Init(std::move(msgBuf));
+
+    ReportDataMessage::Builder response;
+    response.Init(&writer);
+    NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR);
+
+    PayloadHeader payloadHeader;
+    payloadHeader.SetExchangeID(0);
+    payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData);
+
+    auto * writeHandler = InteractionModelEngine::GetInstance()->ActiveWriteHandlerAt(0);
+    rm->ClearRetransTable(writeClient.mExchangeCtx.Get());
+    rm->ClearRetransTable(writeHandler->mExchangeCtx.Get());
+    ctx.GetLoopback().mSentMessageCount  = 0;
+    ctx.GetLoopback().mNumMessagesToDrop = 0;
+    writeHandler->OnMessageReceived(writeHandler->mExchangeCtx.Get(), payloadHeader, std::move(msgBuf));
+    ctx.DrainAndServiceIO();
+
+    NL_TEST_ASSERT(apSuite, writeCallback.mLastErrorReason.mStatus == Protocols::InteractionModel::Status::InvalidAction);
+    NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 0);
+    engine->Shutdown();
+    ctx.ExpireSessionAliceToBob();
+    ctx.ExpireSessionBobToAlice();
+    ctx.CreateSessionAliceToBob();
+    ctx.CreateSessionBobToAlice();
+}
+#endif
 } // namespace app
 } // namespace chip
 
@@ -562,6 +640,9 @@ const nlTest sTests[] =
         NL_TEST_DEF("TestWriteRoundtripWithClusterObjects", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjects),
         NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMatch),
         NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch),
+#if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+        NL_TEST_DEF("TestWriteHandlerReceiveInvalidMessage", chip::app::TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage),
+#endif
         NL_TEST_SENTINEL()
 };
 // clang-format on