Fix status report handling in write handler (#21440)
diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index b21d5b5..94233c8 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -202,6 +202,24 @@
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;
diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
index 2863749..9f21baf 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -160,6 +160,11 @@
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()
*/
uint32_t GetMagicNumber() const { return mMagic; }
diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp
index 12a2086..36d92ca 100644
--- a/src/app/WriteHandler.cpp
+++ b/src/app/WriteHandler.cpp
@@ -31,7 +31,7 @@
namespace app {
using namespace Protocols::InteractionModel;
-
+using Status = Protocols::InteractionModel::Status;
constexpr uint8_t kListAttributeType = 0x48;
CHIP_ERROR WriteHandler::Init()
@@ -118,7 +118,14 @@
"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;
+ //Logging purpose to print the status error code
+ 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 @@
Close();
}
}
- else if (status != Protocols::InteractionModel::Status::Success)
+ else
{
err = StatusResponse::Send(status, apExchangeContext, false /*aExpectResponse*/);
Close();
@@ -312,7 +319,7 @@
// 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::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));
}
diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h
index 43163a2..7a27266 100644
--- a/src/app/WriteHandler.h
+++ b/src/app/WriteHandler.h
@@ -114,6 +114,7 @@
}
private:
+ friend class TestWriteInteraction;
enum class State
{
Uninitialized = 0, // The handler has not been initialized
@@ -121,9 +122,10 @@
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);
diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp
index f1af80c..7115770 100644
--- a/src/app/tests/TestWriteInteraction.cpp
+++ b/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,6 +63,7 @@
static void TestWriteRoundtripWithClusterObjects(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtripWithClusterObjectsVersionMatch(nlTestSuite * apSuite, void * apContext);
static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext);
+ static void TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext);
private:
static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient);
@@ -90,13 +93,18 @@
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 +551,74 @@
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.
+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();
+}
+
} // namespace app
} // namespace chip
@@ -562,6 +638,7 @@
NL_TEST_DEF("TestWriteRoundtripWithClusterObjects", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjects),
NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMatch),
NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch),
+ NL_TEST_DEF("TestWriteHandlerReceiveInvalidMessage", chip::app::TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage),
NL_TEST_SENTINEL()
};
// clang-format on
