Added verification for SceneName in logging and applied correction to…

… scene implementation
project-chip · Aug 21, 2023 · 1016400 · 1016400
1 parent 3e45676
commit 1016400
Show file tree

Hide file tree

Showing 7 changed files with 428 additions and 25 deletions.
diff --git a/src/app/clusters/scenes/SceneTable.h b/src/app/clusters/scenes/SceneTable.h
@@ -20,6 +20,7 @@
 #include <app/clusters/scenes/ExtensionFieldsSets.h>
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/CommonIterator.h>
+#include <lib/support/CommonPersistentData.h>
 
 namespace chip {
 namespace scenes {
@@ -111,13 +112,12 @@ class SceneTable
         ExtensionFieldsSets extentsionFieldsSets;
         TransitionTime100ms transitionTime100 = 0;
 
-        SceneData() = default;
-        SceneData(const char * sceneName, SceneTransitionTime time = 0, TransitionTime100ms time100ms = 0) :
+        SceneData(const char * sceneName = nullptr, SceneTransitionTime time = 0, TransitionTime100ms time100ms = 0) :
             sceneTransitionTime(time), transitionTime100(time100ms)
         {
             SetName(sceneName);
         }
-        SceneData(const char * sceneName, ExtensionFieldsSets fields, SceneTransitionTime time = 0,
+        SceneData(ExtensionFieldsSets fields, const char * sceneName = nullptr, SceneTransitionTime time = 0,
                   TransitionTime100ms time100ms = 0) :
             sceneTransitionTime(time),
             transitionTime100(time100ms)
@@ -136,8 +136,13 @@ class SceneTable
             TLV::TLVType container;
             ReturnErrorOnFailure(writer.StartContainer(TLV::ContextTag(1), TLV::kTLVType_Structure, container));
 
-            size_t name_size = strnlen(this->name, kSceneNameMax);
-            ReturnErrorOnFailure(writer.PutString(TagSceneName(), this->name, static_cast<uint32_t>(name_size)));
+            // assumes a 0 size means the name wasn't used so it doesn't get stored
+            if (this->name[0] != 0)
+            {
+                size_t name_size = strnlen(this->name, kSceneNameMax);
+                ReturnErrorOnFailure(writer.PutString(TagSceneName(), this->name, static_cast<uint32_t>(name_size)));
+            }
+
             ReturnErrorOnFailure(writer.Put(TagSceneTransitionTime(), static_cast<uint16_t>(this->sceneTransitionTime)));
             ReturnErrorOnFailure(writer.Put(TagSceneTransitionTime100(), static_cast<uint8_t>(this->transitionTime100)));
             ReturnErrorOnFailure(this->extentsionFieldsSets.Serialize(writer));
@@ -150,10 +155,20 @@ class SceneTable
             ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Structure, TLV::ContextTag(1)));
             ReturnErrorOnFailure(reader.EnterContainer(container));
 
-            ReturnErrorOnFailure(reader.Next(TagSceneName()));
-            ReturnErrorOnFailure(reader.GetString(this->name, sizeof(this->name)));
-            size_t name_size      = strnlen(this->name, kSceneNameMax); // TODO : verify use of strnlen is ok
-            this->name[name_size] = 0;                                  // Putting a null terminator
+            ReturnErrorOnFailure(reader.Next());
+            TLV::Tag currTag = reader.GetTag();
+            VerifyOrReturnError(TagSceneName() == currTag || TagSceneTransitionTime() == currTag, CHIP_ERROR_WRONG_TLV_TYPE);
+
+            // If there was no error, a name is expected from the storage, if there was an unexpectec TLV element,
+            if (currTag == TagSceneName())
+            {
+                size_t name_size = reader.GetLength();
+                VerifyOrReturnError(name_size <= (kSceneNameMax - 1), CHIP_ERROR_BUFFER_TOO_SMALL);
+                ReturnErrorOnFailure(reader.GetString(this->name, name_size));
+                this->name[name_size] = 0;
+            }
+
+            // Putting a null terminator
             ReturnErrorOnFailure(reader.Next(TagSceneTransitionTime()));
             ReturnErrorOnFailure(reader.Get(this->sceneTransitionTime));
             ReturnErrorOnFailure(reader.Next(TagSceneTransitionTime100()));
@@ -212,7 +227,10 @@ class SceneTable
         SceneTableEntry(SceneStorageId id) : storageId(id) {}
         SceneTableEntry(const SceneStorageId id, const SceneData data) : storageId(id), storageData(data) {}
 
-        bool operator==(const SceneTableEntry & other) { return (this->storageId == other.storageId); }
+        bool operator==(const SceneTableEntry & other)
+        {
+            return (this->storageId == other.storageId && this->storageData == other.storageData);
+        }
 
         void operator=(const SceneTableEntry & other)
         {
@@ -229,8 +247,8 @@ class SceneTable
     SceneTable(const SceneTable &)             = delete;
     SceneTable & operator=(const SceneTable &) = delete;
 
-    virtual CHIP_ERROR Init() = 0;
-    virtual void Finish()     = 0;
+    virtual CHIP_ERROR Init(PersistentStorageDelegate * storage) = 0;
+    virtual void Finish()                                        = 0;
 
     // Data
     virtual CHIP_ERROR SetSceneTableEntry(FabricIndex fabric_index, const SceneTableEntry & entry)                    = 0;
@@ -241,6 +259,12 @@ class SceneTable
     using SceneEntryIterator = CommonIterator<SceneTableEntry>;
 
     virtual SceneEntryIterator * IterateSceneEntry(FabricIndex fabric_index) = 0;
+
+    // Fabrics
+    virtual CHIP_ERROR RemoveFabric(FabricIndex fabric_index) = 0;
+
+protected:
+    const uint8_t mMaxScenePerFabric = kMaxScenePerFabric;
 };
 
 } // namespace scenes

diff --git a/src/app/clusters/scenes/SceneTableImpl.cpp b/src/app/clusters/scenes/SceneTableImpl.cpp
@@ -16,7 +16,6 @@
  */
 
 #include <app/clusters/scenes/SceneTableImpl.h>
-#include <lib/support/CommonPersistentData.h>
 #include <lib/support/DefaultStorageKeyAllocator.h>
 #include <stdlib.h>
 
@@ -401,6 +400,32 @@ CHIP_ERROR DefaultSceneTableImpl::RemoveSceneTableEntry(FabricIndex fabric_index
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR DefaultSceneTableImpl::RemoveFabric(FabricIndex fabric_index)
+{
+    FabricSceneData fabric(fabric_index);
+
+    CHIP_ERROR err = fabric.Load(mStorage);
+    VerifyOrReturnError(CHIP_NO_ERROR == err || CHIP_ERROR_NOT_FOUND == err, err);
+
+    // Remove scene entries
+    SceneTableData scene(fabric_index, fabric.first_scene);
+    size_t scene_count = 0;
+
+    while (scene_count < fabric.scene_count)
+    {
+        if (CHIP_NO_ERROR != scene.Load(mStorage))
+        {
+            break;
+        }
+        RemoveSceneTableEntry(fabric_index, scene.storageId);
+        scene.storageId = scene.next;
+        scene_count++;
+    }
+
+    // Remove fabric
+    return fabric.Delete(mStorage);
+}
+
 DefaultSceneTableImpl::SceneEntryIterator * DefaultSceneTableImpl::IterateSceneEntry(FabricIndex fabric_index)
 {
     VerifyOrReturnError(IsInitialized(), nullptr);

diff --git a/src/app/clusters/scenes/SceneTableImpl.h b/src/app/clusters/scenes/SceneTableImpl.h
@@ -40,22 +40,25 @@ class DefaultSceneTableImpl : public SceneTable
 
     ~DefaultSceneTableImpl() override {}
 
-    CHIP_ERROR Init(PersistentStorageDelegate * storage);
-    void Finish();
+    CHIP_ERROR Init(PersistentStorageDelegate * storage) override;
+    void Finish() override;
 
     //
     // Scene Data
     //
 
     // By id
-    CHIP_ERROR SetSceneTableEntry(FabricIndex fabric_index, const SceneTableEntry & entry);
+    CHIP_ERROR SetSceneTableEntry(FabricIndex fabric_index, const SceneTableEntry & entry) override;
     CHIP_ERROR GetSceneTableEntry(FabricIndex fabric_index, DefaultSceneTableImpl::SceneStorageId scene_id,
-                                  SceneTableEntry & entry);
-    CHIP_ERROR RemoveSceneTableEntry(FabricIndex fabric_index, DefaultSceneTableImpl::SceneStorageId scene_id);
+                                  SceneTableEntry & entry) override;
+    CHIP_ERROR RemoveSceneTableEntry(FabricIndex fabric_index, DefaultSceneTableImpl::SceneStorageId scene_id) override;
 
     // Iterators
     SceneEntryIterator * IterateSceneEntry(FabricIndex fabric_index) override;
 
+    // Fabrics
+    CHIP_ERROR RemoveFabric(FabricIndex fabric_index) override;
+
 protected:
     class SceneEntryIteratorImpl : public SceneEntryIterator
     {
@@ -76,8 +79,6 @@ class DefaultSceneTableImpl : public SceneTable
 
     chip::PersistentStorageDelegate * mStorage = nullptr;
     ObjectPool<SceneEntryIteratorImpl, kIteratorsMax> mSceneEntryIterators;
-
-    const uint8_t mMaxScenePerFabric = kMaxScenePerFabric;
 }; // class DefaultSceneTableImpl
 
 /**
@@ -96,7 +97,7 @@ DefaultSceneTableImpl * GetSceneTable();
  *
  * The `provider` can be set to nullptr if the owner is done with it fully.
  *
- * @param[in] provider pointer to the Scene Table global isntance to use
+ * @param[in] provider pointer to the Scene Table global instance to use
  */
 void SetSceneTable(DefaultSceneTableImpl * provider);
 } // namespace scenes

diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn
@@ -79,6 +79,22 @@ source_set("ota-requestor-test-srcs") {
   ]
 }
 
+source_set("scenes-table-test-srcs") {
+  sources = [
+    "${chip_root}/src/app/clusters/scenes/ExtensionFieldsSets.cpp",
+    "${chip_root}/src/app/clusters/scenes/ExtensionFieldsSets.h",
+    "${chip_root}/src/app/clusters/scenes/SceneTable.h",
+    "${chip_root}/src/app/clusters/scenes/SceneTableImpl.cpp",
+    "${chip_root}/src/app/clusters/scenes/SceneTableImpl.h",
+    "${chip_root}/src/app/clusters/scenes/scenes-tokens.h",
+  ]
+
+  public_deps = [
+    "${chip_root}/src/app/common:cluster-objects",
+    "${chip_root}/src/lib/core",
+  ]
+}
+
 chip_test_suite("tests") {
   output_name = "libAppTests"
 
@@ -105,6 +121,7 @@ chip_test_suite("tests") {
     "TestPendingNotificationMap.cpp",
     "TestReadInteraction.cpp",
     "TestReportingEngine.cpp",
+    "TestSceneTable.cpp",
     "TestStatusIB.cpp",
     "TestStatusResponseMessage.cpp",
     "TestTimedHandler.cpp",
@@ -140,6 +157,7 @@ chip_test_suite("tests") {
     ":binding-test-srcs",
     ":client-monitoring-test-srcs",
     ":ota-requestor-test-srcs",
+    ":scenes-table-test-srcs",
     "${chip_root}/src/app",
     "${chip_root}/src/app/common:cluster-objects",
     "${chip_root}/src/app/tests:helpers",