object/nc: Fix incorrect apdu_len calculation (#517)

* object/nc: Fix incorrect apdu_len calculation when encoding Recipient_List which had resulted in malformed APDU.

* enabled unit testing on notification-class object example

---------

Co-authored-by: Tomasz Kazimierz Motyl <butterfly_tm666@yahoo.com>
Co-authored-by: Steve Karg <skarg@users.sourceforge.net>

src/bacnet/basic/object/nc.c

@@ -267,6 +267,7 @@ int Notification_Class_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
                 break;
             }
             /* size fits, therefore, encode all entry of Recipient_List */
+            apdu_len = 0;
             for (idx = 0; idx < NC_MAX_RECIPIENTS; idx++) {
                 BACNET_DESTINATION *Destination;
                 BACNET_RECIPIENT *Recipient;
@@ -447,7 +448,11 @@ bool Notification_Class_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             status = true;
             break;
 
+        case PROP_OBJECT_IDENTIFIER:
         case PROP_OBJECT_NAME:
+        case PROP_OBJECT_TYPE:
+        case PROP_DESCRIPTION:
+        case PROP_NOTIFICATION_CLASS:
             wp_data->error_class = ERROR_CLASS_PROPERTY;
             wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             break;
@@ -704,8 +709,7 @@ void Notification_Class_find_recipient(void)
  * the BACnetLIST all of the specified elements, or to neither add nor
  * update any elements at all.
  */
-int Notification_Class_Add_List_Element(
+int Notification_Class_Add_List_Element(BACNET_LIST_ELEMENT_DATA *list_element)
-    BACNET_LIST_ELEMENT_DATA * list_element)
 {
     NOTIFICATION_CLASS_INFO *notification = NULL;
     BACNET_DESTINATION recipient_list[NC_MAX_RECIPIENTS] = { 0 };
@@ -755,8 +759,8 @@ int Notification_Class_Add_List_Element(
     application_data = list_element->application_data;
     application_data_len = list_element->application_data_len;
     while (application_data_len > 0) {
-        len = bacnet_destination_decode(application_data,
+        len = bacnet_destination_decode(
-            application_data_len, &recipient_list[index]);
+            application_data, application_data_len, &recipient_list[index]);
         if (len > 0) {
             new_element_count++;
             application_data_len -= len;
@@ -793,7 +797,7 @@ int Notification_Class_Add_List_Element(
             same_element_count++;
         } else {
             added_element_count++;
-            if ((added_element_count+element_count) > NC_MAX_RECIPIENTS) {
+            if ((added_element_count + element_count) > NC_MAX_RECIPIENTS) {
                 list_element->first_failed_element_number = 1 + i;
                 list_element->error_class = ERROR_CLASS_RESOURCES;
                 list_element->error_code =
@@ -862,7 +866,7 @@ int Notification_Class_Add_List_Element(
  * a 'Result(-)' response primitive shall be issued.
  */
 int Notification_Class_Remove_List_Element(
-    BACNET_LIST_ELEMENT_DATA * list_element)
+    BACNET_LIST_ELEMENT_DATA *list_element)
 {
     NOTIFICATION_CLASS_INFO *notification = NULL;
     uint32_t notify_index = 0;
@@ -911,8 +915,8 @@ int Notification_Class_Remove_List_Element(
     application_data = list_element->application_data;
     application_data_len = list_element->application_data_len;
     while (application_data_len > 0) {
-        len = bacnet_destination_decode(application_data,
+        len = bacnet_destination_decode(
-            application_data_len, &recipient_list[index]);
+            application_data, application_data_len, &recipient_list[index]);
         if (len > 0) {
             remove_element_count++;
             application_data_len -= len;

test/CMakeLists.txt

@@ -125,6 +125,7 @@ list(APPEND testdirs
   bacnet/basic/object/mso
   bacnet/basic/object/msv
   bacnet/basic/object/netport
+  bacnet/basic/object/nc
   bacnet/basic/object/objects
   bacnet/basic/object/osv
   bacnet/basic/object/piv

test/bacnet/basic/object/nc/src/main.c

@@ -6,9 +6,10 @@
  *
  * SPDX-License-Identifier: MIT
  */
-#include <ztest.h>
+#include <zephyr/ztest.h>
 #include <bacnet/bactext.h>
 #include <bacnet/rp.h>
+#include <bacnet/wp.h>
 #include <bacnet/basic/object/nc.h>
 
 /**
@@ -25,29 +26,103 @@ ZTEST(notification_class_tests, test_Notification_Class)
 static void test_Notification_Class(void)
 #endif
 {
-    BACNET_READ_PROPERTY_DATA rpdata;
     uint8_t apdu[MAX_APDU] = { 0 };
     int len = 0;
-    uint32_t len_value = 0;
+    int test_len = 0;
-    uint8_t tag_number = 0;
+    BACNET_READ_PROPERTY_DATA rpdata = { 0 };
-    BACNET_OBJECT_TYPE decoded_type = 0;
+    BACNET_APPLICATION_DATA_VALUE value = { 0 };
-    uint32_t decoded_instance = 0;
+    const int *pRequired = NULL;
+    const int *pOptional = NULL;
+    const int *pProprietary = NULL;
+    const uint32_t instance = 1;
+    BACNET_WRITE_PROPERTY_DATA wpdata = { 0 };
+    bool status = false;
+    unsigned index;
 
     Notification_Class_Init();
+    status = Notification_Class_Valid_Instance(instance);
+    zassert_true(status, NULL);
+    index = Notification_Class_Instance_To_Index(instance);
+    zassert_equal(index, instance, "index=%u", index);
+
     rpdata.application_data = &apdu[0];
     rpdata.application_data_len = sizeof(apdu);
     rpdata.object_type = OBJECT_NOTIFICATION_CLASS;
-    rpdata.object_instance = 1;
+    rpdata.object_instance = instance;
     rpdata.object_property = PROP_OBJECT_IDENTIFIER;
-    rpdata.array_index = BACNET_ARRAY_ALL;
+
+    Notification_Class_Property_Lists(&pRequired, &pOptional, &pProprietary);
+    while ((*pRequired) >= 0) {
+        rpdata.object_property = *pRequired;
+        rpdata.array_index = BACNET_ARRAY_ALL;
+        len = Notification_Class_Read_Property(&rpdata);
+        zassert_not_equal(len, BACNET_STATUS_ERROR, 
+            "property '%s': failed to ReadProperty!\n",
+            bactext_property_name(rpdata.object_property));
+        if (len >= 0) {
+            test_len = bacapp_decode_known_property(rpdata.application_data,
+                len, &value, rpdata.object_type, rpdata.object_property);
+            if ((rpdata.object_property != PROP_PRIORITY) &&
+                (rpdata.object_property != PROP_RECIPIENT_LIST)) {
+                zassert_equal(len, test_len, "property '%s': failed to decode!\n",
+                        bactext_property_name(rpdata.object_property));
+            }
+            /* check WriteProperty properties */
+            wpdata.object_type = rpdata.object_type;
+            wpdata.object_instance = rpdata.object_instance;
+            wpdata.object_property = rpdata.object_property;
+            wpdata.array_index = rpdata.array_index;
+            memcpy(&wpdata.application_data, rpdata.application_data, MAX_APDU);
+            wpdata.application_data_len = len;
+            wpdata.error_code = ERROR_CODE_SUCCESS;
+            status = Notification_Class_Write_Property(&wpdata);
+            if (!status) {
+                /* verify WriteProperty property is known */
+                zassert_not_equal(wpdata.error_code,
+                    ERROR_CODE_UNKNOWN_PROPERTY,
+                    "property '%s': WriteProperty Unknown!\n",
+                    bactext_property_name(rpdata.object_property));
+            }
+        }
+        pRequired++;
+    }
+    while ((*pOptional) != -1) {
+        rpdata.object_property = *pOptional;
+        rpdata.array_index = BACNET_ARRAY_ALL;
+        len = Notification_Class_Read_Property(&rpdata);
+        zassert_not_equal(len, BACNET_STATUS_ERROR, 
+            "property '%s': failed to ReadProperty!\n",
+            bactext_property_name(rpdata.object_property));
+        if (len > 0) {
+            test_len = bacapp_decode_application_data(rpdata.application_data,
+                (uint8_t)rpdata.application_data_len, &value);
+            zassert_equal(len, test_len, "property '%s': failed to decode!\n",
+                bactext_property_name(rpdata.object_property));
+            /* check WriteProperty properties */
+            wpdata.object_type = rpdata.object_type;
+            wpdata.object_instance = rpdata.object_instance;
+            wpdata.object_property = rpdata.object_property;
+            wpdata.array_index = rpdata.array_index;
+            memcpy(&wpdata.application_data, rpdata.application_data, MAX_APDU);
+            wpdata.application_data_len = len;
+            wpdata.error_code = ERROR_CODE_SUCCESS;
+            status = Notification_Class_Write_Property(&wpdata);
+            if (!status) {
+                /* verify WriteProperty property is known */
+                zassert_not_equal(wpdata.error_code,
+                    ERROR_CODE_UNKNOWN_PROPERTY,
+                    "property '%s': WriteProperty Unknown!\n",
+                    bactext_property_name(rpdata.object_property));
+            }
+        }
+        pOptional++;
+    }
+    rpdata.object_property = PROP_ALL;
     len = Notification_Class_Read_Property(&rpdata);
-    zassert_not_equal(len, 0, NULL);
+    zassert_equal(len, BACNET_STATUS_ERROR, NULL);
-    len = bacnet_decode_tag_number_and_value(
+    wpdata.object_property = PROP_ALL;
-        apdu, sizeof(apdu), &tag_number, &len_value);
+    status = Notification_Class_Write_Property(&wpdata);
-    zassert_equal(tag_number, BACNET_APPLICATION_TAG_OBJECT_ID, NULL);
+    zassert_false(status, NULL);
-    len = decode_object_id(&apdu[len], &decoded_type, &decoded_instance);
-    zassert_equal(decoded_type, rpdata.object_type, NULL);
-    zassert_equal(decoded_instance, rpdata.object_instance, NULL);
 
     return;
 }