Merge branch 'fix-str-len-checks' into 'main'

Fix problems with string type attributes

See merge request app-frameworks/esp-matter!618

Author: dhrishi

RELEASE_NOTES.md

@@ -1,3 +1,9 @@
+# 14-February-2024
+
+- An optional argument, `max_val_size`, has been introduced to the `esp_matter::attribute::create()` API.
+  This argument is utilized specifically when creating attributes of the char string and long char string data types
+  to specify the maximum supported value size of an attribute.
+
 # 29-January-2024
 
 - Add a new parameter for esp_matter::client::connect() to set the CASESessionManager for finding or establishing the CASE sessions.

components/esp_matter/esp_matter_attribute.cpp

@@ -14,6 +14,8 @@
 
 #include <esp_log.h>
 #include <esp_matter_attribute.h>
+#include <esp_matter.h>
+#include <esp_matter_core.h>
 
 static const char *TAG = "esp_matter_attribute";
 
@@ -183,7 +185,7 @@ attribute_t *create_node_label(cluster_t *cluster, char *value, uint16_t length)
     }
     return esp_matter::attribute::create(cluster, BasicInformation::Attributes::NodeLabel::Id,
                                          ATTRIBUTE_FLAG_WRITABLE | ATTRIBUTE_FLAG_NONVOLATILE,
-                                         esp_matter_char_str(value, length));
+                                         esp_matter_char_str(value, length), k_max_node_label_length);
 }
 
 attribute_t *create_location(cluster_t *cluster, char *value, uint16_t length)
@@ -926,7 +928,7 @@ attribute_t *create_node_label(cluster_t *cluster, char *value, uint16_t length)
     }
     return esp_matter::attribute::create(cluster, BridgedDeviceBasicInformation::Attributes::NodeLabel::Id,
                                          ATTRIBUTE_FLAG_WRITABLE | ATTRIBUTE_FLAG_NONVOLATILE,
-                                         esp_matter_char_str(value, length));
+                                         esp_matter_char_str(value, length), k_max_node_label_length);
 }
 
 attribute_t *create_hardware_version(cluster_t *cluster, uint16_t value)
@@ -3652,7 +3654,9 @@ attribute_t *create_mode_select_description(cluster_t *cluster, const char * val
         ESP_LOGE(TAG, "Could not create attribute, string length out of bound");
         return NULL;
     }
-    return esp_matter::attribute::create(cluster, ModeSelect::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length));
+    return esp_matter::attribute::create(cluster, ModeSelect::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE,
+                                         esp_matter_char_str((char *)value, length),
+                                         k_max_mode_select_description_length);
 }
 
 attribute_t *create_standard_namespace(cluster_t *cluster, const nullable<uint16_t> value)
@@ -3708,7 +3712,8 @@ attribute_t *create_description(cluster_t *cluster, const char * value, uint16_t
         ESP_LOGE(TAG, "Could not create attribute, string length out of bound");
         return NULL;
     }
-    return esp_matter::attribute::create(cluster, PowerSource::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length));
+    return esp_matter::attribute::create(cluster, PowerSource::Attributes::Description::Id, ATTRIBUTE_FLAG_NONE,
+                                         esp_matter_char_str((char *)value, length), k_max_description_length);
 }
 
 attribute_t *create_wired_assessed_input_voltage(cluster_t *cluster, nullable<uint32_t> value, nullable<uint32_t> min, nullable<uint32_t> max)
@@ -3853,7 +3858,9 @@ attribute_t *create_bat_replacement_description(cluster_t *cluster, const char *
         ESP_LOGE(TAG, "Could not create attribute, string size out of bound");
         return NULL;
     }
-    return esp_matter::attribute::create(cluster, PowerSource::Attributes::BatReplacementDescription::Id, ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length));
+    return esp_matter::attribute::create(cluster, PowerSource::Attributes::BatReplacementDescription::Id,
+                                         ATTRIBUTE_FLAG_NONE, esp_matter_char_str((char *)value, length),
+                                         k_max_bat_replacement_description_length);
 }
 
 attribute_t *create_bat_common_designation(cluster_t *cluster, const uint8_t value, uint8_t min, uint8_t max)

components/esp_matter/esp_matter_attribute.h

@@ -14,7 +14,6 @@
 
 #pragma once
 
-#include <esp_matter.h>
 #include <esp_matter_core.h>
 
 namespace esp_matter {
@@ -768,6 +767,9 @@ attribute_t *state_value(cluster_t *cluster, bool value);
 } /* boolean_state */
 
 namespace localization_configuration {
+
+constexpr uint8_t k_max_active_locale_length = 35;
+
 namespace attribute {
 attribute_t *create_active_locale(cluster_t *cluster, char *value, uint16_t length);
 attribute_t *create_supported_locales(cluster_t *cluster, uint8_t *value, uint16_t length, uint16_t count);
@@ -861,6 +863,7 @@ constexpr uint8_t k_max_description_length = 60;
 constexpr uint8_t k_max_fault_count = 8;
 constexpr uint8_t k_max_designation_count = 20;
 constexpr uint8_t k_max_charge_faults_count = 16;
+constexpr uint8_t k_max_bat_replacement_description_length = 60;
 
 namespace attribute {
 attribute_t *create_status(cluster_t *cluster, uint8_t value);

components/esp_matter/esp_matter_cluster.cpp

@@ -246,7 +246,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags)
         /* Attributes not managed internally */
         if (config) {
             global::attribute::create_cluster_revision(cluster, config->cluster_revision);
-            attribute::create_node_label(cluster, config->node_label, sizeof(config->node_label));
+            attribute::create_node_label(cluster, config->node_label, strlen(config->node_label));
         } else {
             ESP_LOGE(TAG, "Config is NULL. Cannot add some attributes.");
         }
@@ -937,7 +937,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags, uint32_
             global::attribute::create_cluster_revision(cluster, config->cluster_revision);
             attribute::create_status(cluster, config->status);
             attribute::create_order(cluster, config->order, 0x00, 0xFF);
-            attribute::create_description(cluster, config->description, sizeof(config->description));
+            attribute::create_description(cluster, config->description, strlen(config->description));
         } else {
             ESP_LOGE(TAG, "Config is NULL. Cannot add some attributes.");
         }
@@ -2704,7 +2704,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags)
         if (config) {
             /* Attributes not managed internally */
             global::attribute::create_cluster_revision(cluster, config->cluster_revision);
-            attribute::create_active_locale(cluster, config->active_locale, sizeof(config->active_locale));
+            attribute::create_active_locale(cluster, config->active_locale, strlen(config->active_locale));
 
             /* Attributes managed internally */
             attribute::create_supported_locales(cluster, NULL, 0, 0);
@@ -2968,7 +2968,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags, uint32_
         /** Attributes not managed internally **/
         if (config) {
             global::attribute::create_cluster_revision(cluster, config->cluster_revision);
-            attribute::create_mode_select_description(cluster, config->mode_select_description, sizeof(config->mode_select_description));
+            attribute::create_mode_select_description(cluster, config->mode_select_description, strlen(config->mode_select_description));
             attribute::create_standard_namespace(cluster, config->standard_namespace);
             attribute::create_current_mode(cluster, config->current_mode);
         } else {

components/esp_matter/esp_matter_cluster.h

@@ -15,6 +15,7 @@
 #pragma once
 
 #include <esp_matter_core.h>
+#include <esp_matter_attribute.h>
 #include <esp_matter_feature.h>
 #include <stdint.h>
 
@@ -70,7 +71,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags);
 namespace basic_information {
 typedef struct config {
     uint16_t cluster_revision;
-    char node_label[32];
+    char node_label[k_max_node_label_length + 1];
     config() : cluster_revision(3), node_label{0} {}
 } config_t;
 
@@ -231,7 +232,7 @@ typedef struct config {
     uint16_t cluster_revision;
     uint8_t status;
     uint8_t order;
-    char description[61];
+    char description[k_max_description_length + 1];
     feature::wired::config_t wired;
     feature::battery::config_t battery;
     feature::rechargeable::config_t rechargeable;
@@ -627,7 +628,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags);
 namespace localization_configuration {
 typedef struct config {
     uint16_t cluster_revision;
-    char active_locale[35];
+    char active_locale[k_max_active_locale_length + 1];
     config() : cluster_revision(4), active_locale{0} {}
 } config_t;
 
@@ -708,7 +709,7 @@ cluster_t *create(endpoint_t *endpoint, config_t *config, uint8_t flags);
 namespace mode_select {
 typedef struct config {
     uint16_t cluster_revision;
-    char mode_select_description[64];
+    char mode_select_description[k_max_mode_select_description_length + 1];
     const nullable<uint16_t> standard_namespace;
     uint8_t current_mode;
     feature::on_off::config_t on_off;

components/esp_matter/esp_matter_core.cpp

@@ -154,6 +154,9 @@ typedef struct _attribute {
     esp_matter_attr_bounds_t *bounds;
     EmberAfDefaultOrMinMaxAttributeValue default_value;
     uint16_t default_value_size;
+    // This is required when creating metadata for char string and long char string types of attributes.
+    // The size in the attribute metadata remains constant and is verified during write operations.
+    uint16_t max_val_size;
     attribute::callback_t override_callback;
     struct _attribute *next;
 } _attribute_t;
@@ -617,8 +620,14 @@ esp_err_t enable(endpoint_t *endpoint)
              * when writing a longer string.
              */
             if (attribute->val.type == ESP_MATTER_VAL_TYPE_CHAR_STRING ||
-                attribute->val.type  == ESP_MATTER_VAL_TYPE_LONG_CHAR_STRING) {
-                matter_attributes[attribute_index].size = attribute->val.val.a.s;
+                attribute->val.type == ESP_MATTER_VAL_TYPE_LONG_CHAR_STRING) {
+                // Once the metadata is created, the attribute size becomes fixed and cannot be modified thereafter.
+                // For string and long string types, the size should be the maximum size defined in the specification
+                // plus the size_for_storing_str_len. The length byte is 1 for char string and 2 for long char string.
+                // For example, the maximum size of the Node-Label in the basic information cluster is 32 bytes,
+                // and it is a char string. Therefore, the size should be (32 + 1).
+                uint16_t size_for_storing_str_len = attribute->val.val.a.t - attribute->val.val.a.s;
+                matter_attributes[attribute_index].size = attribute->max_val_size + size_for_storing_str_len;
             }
 
             matter_clusters[cluster_index].clusterSize += matter_attributes[attribute_index].size;
@@ -1094,7 +1103,8 @@ esp_err_t factory_reset()
 }
 
 namespace attribute {
-attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val)
+attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val,
+                    uint16_t max_val_size)
 {
     /* Find */
     if (!cluster) {
@@ -1122,6 +1132,7 @@ attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, es
     attribute->endpoint_id = current_cluster->endpoint_id;
     attribute->flags = flags;
     attribute->flags |= ATTRIBUTE_FLAG_EXTERNAL_STORAGE;
+    attribute->max_val_size = max_val_size;
 
     // After reboot, string and array are treated as Invalid. So need to store val.type and size of attribute value.
     attribute->val.type = val.type;

components/esp_matter/esp_matter_core.h

@@ -425,15 +425,19 @@ namespace attribute {
  *
  * This will create a new attribute and add it to the cluster.
  *
- * @param[in] cluster Cluster handle.
- * @param[in] attribute_id Attribute ID for the attribute.
- * @param[in] flags Bitmap of `attribute_flags_t`.
- * @param[in] val Default type and value of the attribute. Use appropriate elements as per the value type.
+ * @param[in] cluster       Cluster handle.
+ * @param[in] attribute_id  Attribute ID for the attribute.
+ * @param[in] flags         Bitmap of `attribute_flags_t`.
+ * @param[in] val           Default type and value of the attribute. Use appropriate elements as per the value type.
+ * @param[in] max_val_size  For attributes of type char string and long char string, the size should correspond to the
+ *                          maximum size defined in the specification. However, for other types of attributes, this
+ *                          parameter remains unused, and therefore the default value is set to 0
  *
  * @return Attribute handle on success.
  * @return NULL in case of failure.
  */
-attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val);
+attribute_t *create(cluster_t *cluster, uint32_t attribute_id, uint8_t flags, esp_matter_attr_val_t val,
+                    uint16_t max_val_size = 0);
 
 /** Get attribute
  *

components/esp_matter/esp_matter_feature.cpp

@@ -222,7 +222,7 @@ esp_err_t add(cluster_t *cluster, config_t *config)
         update_feature_map(cluster, get_id());
 
         /* Attributes not managed internally */
-        attribute::create_bat_replacement_description(cluster, config->bat_replacement_description, sizeof(config->bat_replacement_description));
+        attribute::create_bat_replacement_description(cluster, config->bat_replacement_description, strlen(config->bat_replacement_description));
         attribute::create_bat_quantity(cluster, config->bat_quantity, 0, 255);
     } else {
         ESP_LOGE(TAG, "Cluster shall support Battery feature");

components/esp_matter/esp_matter_feature.h

@@ -14,8 +14,8 @@
 
 #pragma once
 
-#include <esp_matter_core.h>
 #include <stdint.h>
+#include <esp_matter_attribute.h>
 
 #define ESP_MATTER_NONE_FEATURE_ID 0x0000
 
@@ -101,7 +101,7 @@ esp_err_t add(cluster_t *cluster, config_t *config);
 // Replaceable feature one must add Battery feature first.
 namespace replaceable {
 typedef struct config {
-    char bat_replacement_description[61];
+    char bat_replacement_description[k_max_bat_replacement_description_length + 1];
     uint8_t bat_quantity;
     config(): bat_replacement_description{0}, bat_quantity(0) {}
 } config_t;