[icd] fix DefaultICDClientStorage::LoadFabricList (#30762)

project-chip · Dec 1, 2023 · d9b2f5b · d9b2f5b
1 parent 223c054
commit d9b2f5b
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 55 deletions.
diff --git a/src/app/icd/client/DefaultICDClientStorage.cpp b/src/app/icd/client/DefaultICDClientStorage.cpp
@@ -23,6 +23,7 @@
 #include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/SafeInt.h>
 #include <lib/support/logging/CHIPLogging.h>
+#include <limits>
 
 namespace {
 // FabricIndex is uint8_t, the tlv size with anonymous tag is 1(control bytes) + 1(value) = 2
@@ -31,6 +32,9 @@ constexpr size_t kFabricIndexTlvSize = 2;
 // The array itself has a control byte and an end-of-array marker.
 constexpr size_t kArrayOverHead  = 2;
 constexpr size_t kFabricIndexMax = 255;
+
+constexpr size_t kMaxFabricListTlvLength = kFabricIndexTlvSize * kFabricIndexMax + kArrayOverHead;
+static_assert(kMaxFabricListTlvLength <= std::numeric_limits<uint16_t>::max(), "Expected size for fabric list TLV is too large!");
 } // namespace
 
 namespace chip {
@@ -72,14 +76,12 @@ CHIP_ERROR DefaultICDClientStorage::UpdateFabricList(FabricIndex fabricIndex)
 CHIP_ERROR DefaultICDClientStorage::LoadFabricList()
 {
     Platform::ScopedMemoryBuffer<uint8_t> backingBuffer;
-    size_t len = kFabricIndexTlvSize * kFabricIndexMax + kArrayOverHead;
-    VerifyOrReturnError(CanCastTo<uint16_t>(len), CHIP_ERROR_BUFFER_TOO_SMALL);
-    ReturnErrorCodeIf(!backingBuffer.Calloc(len), CHIP_ERROR_NO_MEMORY);
-    uint16_t length = static_cast<uint16_t>(len);
+    ReturnErrorCodeIf(!backingBuffer.Calloc(kMaxFabricListTlvLength), CHIP_ERROR_NO_MEMORY);
+    uint16_t length = kMaxFabricListTlvLength;
     ReturnErrorOnFailure(
         mpClientInfoStore->SyncGetKeyValue(DefaultStorageKeyAllocator::ICDFabricList().KeyName(), backingBuffer.Get(), length));
 
-    TLV::ScopedBufferTLVReader reader(std::move(backingBuffer), len);
+    TLV::ScopedBufferTLVReader reader(std::move(backingBuffer), length);
     ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Array, TLV::AnonymousTag()));
     TLV::TLVType arrayType;
     ReturnErrorOnFailure(reader.EnterContainer(arrayType));

diff --git a/src/app/tests/TestDefaultICDClientStorage.cpp b/src/app/tests/TestDefaultICDClientStorage.cpp
@@ -57,62 +57,74 @@ void TestClientInfoCount(nlTestSuite * apSuite, void * apContext)
     FabricIndex fabricId = 1;
     NodeId nodeId1       = 6666;
     NodeId nodeId2       = 6667;
-    DefaultICDClientStorage manager;
+
     TestPersistentStorageDelegate clientInfoStorage;
     TestSessionKeystoreImpl keystore;
-    err = manager.Init(&clientInfoStorage, &keystore);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    err = manager.UpdateFabricList(fabricId);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    // Write some ClientInfos and see the counts are correct
-    ICDClientInfo clientInfo1;
-    clientInfo1.peer_node = ScopedNodeId(nodeId1, fabricId);
-    ICDClientInfo clientInfo2;
-    clientInfo2.peer_node = ScopedNodeId(nodeId2, fabricId);
-    ICDClientInfo clientInfo3;
-    clientInfo3.peer_node = ScopedNodeId(nodeId1, fabricId);
-    err                   = manager.SetKey(clientInfo1, ByteSpan(kKeyBuffer1));
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    err = manager.StoreEntry(clientInfo1);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-
-    err = manager.SetKey(clientInfo2, ByteSpan(kKeyBuffer2));
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    err = manager.StoreEntry(clientInfo2);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-
-    err = manager.SetKey(clientInfo3, ByteSpan(kKeyBuffer3));
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    err = manager.StoreEntry(clientInfo3);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
-    // Make sure iterator counts correctly
-    auto * iterator = manager.IterateICDClientInfo();
-    // same nodeId for clientInfo2 and clientInfo3, so the new one replace old one
-    NL_TEST_ASSERT(apSuite, iterator->Count() == 2);
-
-    ICDClientInfo clientInfo;
-    NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
-    NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId2);
-    NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
-    NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId1);
-
-    iterator->Release();
-
-    // Delete all and verify iterator counts 0
-    err = manager.DeleteAllEntries(fabricId);
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-    iterator = manager.IterateICDClientInfo();
-    NL_TEST_ASSERT(apSuite, iterator->Count() == 0);
+    {
+        DefaultICDClientStorage manager;
+        err = manager.Init(&clientInfoStorage, &keystore);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        err = manager.UpdateFabricList(fabricId);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        // Write some ClientInfos and see the counts are correct
+        ICDClientInfo clientInfo1;
+        clientInfo1.peer_node = ScopedNodeId(nodeId1, fabricId);
+        ICDClientInfo clientInfo2;
+        clientInfo2.peer_node = ScopedNodeId(nodeId2, fabricId);
+        ICDClientInfo clientInfo3;
+        clientInfo3.peer_node = ScopedNodeId(nodeId1, fabricId);
+        err                   = manager.SetKey(clientInfo1, ByteSpan(kKeyBuffer1));
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        err = manager.StoreEntry(clientInfo1);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+        err = manager.SetKey(clientInfo2, ByteSpan(kKeyBuffer2));
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        err = manager.StoreEntry(clientInfo2);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+        err = manager.SetKey(clientInfo3, ByteSpan(kKeyBuffer3));
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        err = manager.StoreEntry(clientInfo3);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+        // Make sure iterator counts correctly
+        auto * iterator = manager.IterateICDClientInfo();
+        // same nodeId for clientInfo2 and clientInfo3, so the new one replace old one
+        NL_TEST_ASSERT(apSuite, iterator->Count() == 2);
+
+        ICDClientInfo clientInfo;
+        NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
+        NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId2);
+        NL_TEST_ASSERT(apSuite, iterator->Next(clientInfo));
+        NL_TEST_ASSERT(apSuite, clientInfo.peer_node.GetNodeId() == nodeId1);
+
+        iterator->Release();
+
+        // Delete all and verify iterator counts 0
+        err = manager.DeleteAllEntries(fabricId);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        iterator = manager.IterateICDClientInfo();
+        NL_TEST_ASSERT(apSuite, iterator->Count() == 0);
+
+        // Verify ClientInfos manually count correctly
+        size_t count = 0;
+        while (iterator->Next(clientInfo))
+        {
+            count++;
+        }
+        iterator->Release();
+        NL_TEST_ASSERT(apSuite, count == 0);
+    }
 
-    // Verify ClientInfos manually count correctly
-    size_t count = 0;
-    while (iterator->Next(clientInfo))
     {
-        count++;
+        DefaultICDClientStorage manager;
+        err = manager.Init(&clientInfoStorage, &keystore);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+        err = manager.UpdateFabricList(fabricId);
+        NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
     }
-    iterator->Release();
-    NL_TEST_ASSERT(apSuite, count == 0);
 }
 
 void TestClientInfoCountMultipleFabric(nlTestSuite * apSuite, void * apContext)