Fixed array index out of range for binary-value, multistate-output, and lighting-output objects. Thanks, Roy! (#430)

Co-authored-by: Steve Karg <skarg@users.sourceforge.net>

src/bacnet/basic/object/bv.c

@@ -196,6 +196,36 @@ BACNET_BINARY_PV Binary_Value_Present_Value(uint32_t object_instance)
     return value;
 }
 
+/**
+ * @brief Encode a BACnetARRAY property element
+ * @param object_instance [in] BACnet network port object instance number
+ * @param priority [in] array index requested:
+ *    0 to N for individual array members
+ * @param apdu [out] Buffer in which the APDU contents are built, or NULL to
+ * return the length of buffer if it had been built
+ * @return The length of the apdu encoded or
+ *   BACNET_STATUS_ERROR for ERROR_CODE_INVALID_ARRAY_INDEX
+ */
+static int Binary_Value_Priority_Array_Encode(
+    uint32_t object_instance, BACNET_ARRAY_INDEX priority, uint8_t *apdu)
+{
+    int apdu_len = BACNET_STATUS_ERROR;
+    BACNET_BINARY_PV value = RELINQUISH_DEFAULT;
+    unsigned index = 0;
+
+    index = Binary_Value_Instance_To_Index(object_instance);
+    if ((index < MAX_BINARY_VALUES) && (priority < BACNET_MAX_PRIORITY)) {
+        if (Binary_Value_Level[index][priority] != BINARY_NULL) {
+            apdu_len = encode_application_null(apdu);
+        } else {
+            value = Binary_Value_Level[index][priority];
+            apdu_len = encode_application_enumerated(apdu, value);
+        }
+    }
+
+    return apdu_len;
+}
+
 /**
  * For a given object instance-number, return the name.
  *
@@ -266,13 +296,12 @@ void Binary_Value_Out_Of_Service_Set(uint32_t instance, bool oos_flag)
  */
 int Binary_Value_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
 {
-    int len = 0;
     int apdu_len = 0; /* return value */
+    int apdu_size = 0;
     BACNET_BIT_STRING bit_string;
     BACNET_CHARACTER_STRING char_string;
     BACNET_BINARY_PV present_value = BINARY_INACTIVE;
     unsigned object_index = 0;
-    unsigned i = 0;
     bool state = false;
     uint8_t *apdu = NULL;
 
@@ -294,6 +323,7 @@ int Binary_Value_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
     }
 
     apdu = rpdata->application_data;
+    apdu_size = rpdata->application_data_len;
     switch (rpdata->object_property) {
         case PROP_OBJECT_IDENTIFIER:
             apdu_len = encode_application_object_id(
@@ -337,49 +367,15 @@ int Binary_Value_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
             apdu_len = encode_application_boolean(&apdu[0], state);
             break;
         case PROP_PRIORITY_ARRAY:
-            /* Array element zero is the number of elements in the array */
+            apdu_len = bacnet_array_encode(rpdata->object_instance,
-            if (rpdata->array_index == 0) {
+                rpdata->array_index, Binary_Value_Priority_Array_Encode,
-                apdu_len =
+                BACNET_MAX_PRIORITY, apdu, apdu_size);
-                    encode_application_unsigned(&apdu[0], BACNET_MAX_PRIORITY);
+            if (apdu_len == BACNET_STATUS_ABORT) {
-                /* if no index was specified, then try to encode the entire list
+                rpdata->error_code =
-                 */
+                    ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                /* into one packet. */
+            } else if (apdu_len == BACNET_STATUS_ERROR) {
-            } else if (rpdata->array_index == BACNET_ARRAY_ALL) {
+                rpdata->error_class = ERROR_CLASS_PROPERTY;
-                for (i = 0; i < BACNET_MAX_PRIORITY; i++) {
+                rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    /* FIXME: check if we have room before adding it to APDU */
-                    if (Binary_Value_Level[object_index][i] == BINARY_NULL) {
-                        len = encode_application_null(&apdu[apdu_len]);
-                    } else {
-                        present_value = Binary_Value_Level[object_index][i];
-                        len = encode_application_enumerated(
-                            &apdu[apdu_len], present_value);
-                    }
-                    /* add it if we have room */
-                    if ((apdu_len + len) < MAX_APDU) {
-                        apdu_len += len;
-                    } else {
-                        rpdata->error_code =
-                            ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                        apdu_len = BACNET_STATUS_ABORT;
-                        break;
-                    }
-                }
-            } else {
-                if (rpdata->array_index <= BACNET_MAX_PRIORITY) {
-                    if (Binary_Value_Level[object_index][rpdata->array_index] ==
-                        BINARY_NULL) {
-                        apdu_len = encode_application_null(&apdu[apdu_len]);
-                    } else {
-                        present_value = Binary_Value_Level[object_index]
-                                                          [rpdata->array_index];
-                        apdu_len = encode_application_enumerated(
-                            &apdu[apdu_len], present_value);
-                    }
-                } else {
-                    rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    apdu_len = BACNET_STATUS_ERROR;
-                }
             }
             break;
         case PROP_RELINQUISH_DEFAULT:

src/bacnet/basic/object/lo.c

@@ -216,58 +216,33 @@ float Lighting_Output_Present_Value(uint32_t object_instance)
 }
 
 /**
- * For a given object instance-number, determines the value at a
+ * @brief Encode a BACnetARRAY property element
- * given priority.
+ * @param object_instance [in] BACnet network port object instance number
- *
+ * @param priority [in] array index requested:
- * @param  object_instance - object-instance number of the object
+ *    0 to N for individual array members
- * @param  priority - priority 1..16
+ * @param apdu [out] Buffer in which the APDU contents are built, or NULL to
- *
+ * return the length of buffer if it had been built
- * @return  priority-value of the object
+ * @return The length of the apdu encoded or
+ *   BACNET_STATUS_ERROR for ERROR_CODE_INVALID_ARRAY_INDEX
  */
-static float Lighting_Output_Priority_Value(
+static int Lighting_Output_Priority_Array_Encode(
-    uint32_t object_instance, unsigned priority)
+    uint32_t object_instance, BACNET_ARRAY_INDEX priority, uint8_t *apdu)
 {
-    float value = 0.0;
+    int apdu_len = BACNET_STATUS_ERROR;
+    float real_value = 0.0;
     unsigned index = 0;
 
     index = Lighting_Output_Instance_To_Index(object_instance);
-    if (index < MAX_LIGHTING_OUTPUTS) {
+    if ((index < MAX_LIGHTING_OUTPUTS) && (priority < BACNET_MAX_PRIORITY)) {
-        if (priority && (priority <= BACNET_MAX_PRIORITY)) {
+        if (BIT_CHECK(Lighting_Output[index].Priority_Active_Bits, priority)) {
-            priority--;
+            real_value = Lighting_Output[index].Priority_Array[priority];
-            value = Lighting_Output[index].Priority_Array[priority];
+            apdu_len = encode_application_real(apdu, real_value);
+        } else {
+            apdu_len = encode_application_null(apdu);
         }
     }
 
-    return value;
+    return apdu_len;
-}
-
-/**
- * For a given object instance-number, determines if the given priority
- * is active or NULL.
- *
- * @param  object_instance - object-instance number of the object
- * @param  priority - priority 1..16
- *
- * @return  true if the priority slot is active
- */
-static bool Lighting_Output_Priority_Active(
-    uint32_t object_instance, unsigned priority)
-{
-    bool status = false;
-    unsigned index = 0;
-
-    index = Lighting_Output_Instance_To_Index(object_instance);
-    if (index < MAX_LIGHTING_OUTPUTS) {
-        if (priority && (priority <= BACNET_MAX_PRIORITY)) {
-            priority--;
-            if (BIT_CHECK(
-                    Lighting_Output[index].Priority_Active_Bits, priority)) {
-                status = true;
-            }
-        }
-    }
-
-    return status;
 }
 
 /**
@@ -908,8 +883,8 @@ bool Lighting_Output_Relinquish_Default_Set(
  */
 int Lighting_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
 {
-    int len = 0;
     int apdu_len = 0; /* return value */
+    int apdu_size = 0;
     BACNET_BIT_STRING bit_string;
     BACNET_CHARACTER_STRING char_string;
     BACNET_LIGHTING_COMMAND lighting_command;
@@ -924,6 +899,7 @@ int Lighting_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
         return 0;
     }
     apdu = rpdata->application_data;
+    apdu_size = rpdata->application_data_len;
     switch (rpdata->object_property) {
         case PROP_OBJECT_IDENTIFIER:
             apdu_len = encode_application_object_id(
@@ -999,59 +975,15 @@ int Lighting_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
             apdu_len = encode_application_real(&apdu[0], real_value);
             break;
         case PROP_PRIORITY_ARRAY:
-            /* Array element zero is the number of elements in the array */
+            apdu_len = bacnet_array_encode(rpdata->object_instance,
-            if (rpdata->array_index == 0) {
+                rpdata->array_index, Lighting_Output_Priority_Array_Encode,
-                apdu_len =
+                BACNET_MAX_PRIORITY, apdu, apdu_size);
-                    encode_application_unsigned(&apdu[0], BACNET_MAX_PRIORITY);
+            if (apdu_len == BACNET_STATUS_ABORT) {
-                /* if no index was specified, then try to encode the entire list
+                rpdata->error_code =
-                 */
+                    ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                /* into one packet. */
+            } else if (apdu_len == BACNET_STATUS_ERROR) {
-            } else if (rpdata->array_index == BACNET_ARRAY_ALL) {
+                rpdata->error_class = ERROR_CLASS_PROPERTY;
-                for (i = 1; i <= BACNET_MAX_PRIORITY; i++) {
+                rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    if (Lighting_Output_Priority_Active(
-                            rpdata->object_instance, i)) {
-                        real_value = Lighting_Output_Priority_Value(
-                            rpdata->object_instance, i);
-                        len = encode_application_real(
-                            &apdu[apdu_len], real_value);
-                    } else {
-                        len = encode_application_null(&apdu[apdu_len]);
-                    }
-                    /* add it if we have room */
-                    if ((apdu_len + len) < MAX_APDU) {
-                        apdu_len += len;
-                    } else {
-                        rpdata->error_code =
-                            ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                        apdu_len = BACNET_STATUS_ABORT;
-                        break;
-                    }
-                }
-            } else {
-                if (rpdata->array_index <= BACNET_MAX_PRIORITY) {
-                    if (Lighting_Output_Priority_Active(
-                            rpdata->object_instance, rpdata->array_index)) {
-                        real_value = Lighting_Output_Priority_Value(
-                            rpdata->object_instance, rpdata->array_index);
-                        len = encode_application_real(
-                            &apdu[apdu_len], real_value);
-                    } else {
-                        len = encode_application_null(&apdu[apdu_len]);
-                    }
-                    /* add it if we have room */
-                    if ((apdu_len + len) < MAX_APDU) {
-                        apdu_len += len;
-                    } else {
-                        rpdata->error_code =
-                            ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                        apdu_len = BACNET_STATUS_ABORT;
-                        break;
-                    }
-                } else {
-                    rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    apdu_len = BACNET_STATUS_ERROR;
-                }
             }
             break;
         case PROP_RELINQUISH_DEFAULT:

src/bacnet/basic/object/mso.c

@@ -273,6 +273,36 @@ uint32_t Multistate_Output_Present_Value(uint32_t object_instance)
     return value;
 }
 
+/**
+ * @brief Encode a BACnetARRAY property element
+ * @param object_instance [in] BACnet network port object instance number
+ * @param priority [in] array index requested:
+ *    0 to N for individual array members
+ * @param apdu [out] Buffer in which the APDU contents are built, or NULL to
+ * return the length of buffer if it had been built
+ * @return The length of the apdu encoded or
+ *   BACNET_STATUS_ERROR for ERROR_CODE_INVALID_ARRAY_INDEX
+ */
+static int Multistate_Output_Priority_Array_Encode(
+    uint32_t object_instance, BACNET_ARRAY_INDEX priority, uint8_t *apdu)
+{
+    int apdu_len = BACNET_STATUS_ERROR;
+    uint32_t value = 1;
+    struct object_data *pObject;
+
+    pObject = Keylist_Data(Object_List, object_instance);
+    if (pObject && (priority < BACNET_MAX_PRIORITY)) {
+        if (pObject->Relinquished[priority]) {
+            apdu_len = encode_application_null(apdu);
+        } else {
+            value = pObject->Priority_Array[priority];
+            apdu_len = encode_application_enumerated(apdu, value);
+        }
+    }
+
+    return apdu_len;
+}
+
 /**
  * @brief For a given object instance-number, determines the active priority
  * @param  object_instance - object-instance number of the object
@@ -297,54 +327,6 @@ unsigned Multistate_Output_Present_Value_Priority(uint32_t object_instance)
     return priority;
 }
 
-/**
- * @brief For a given object instance-number and priority 1..16, determines the
- *  priority-array value
- * @param  object_instance - object-instance number of the object
- * @param  priority - priority-array index value 1..16
- * @return priority-array value of the object
- */
-static uint32_t Multistate_Output_Priority_Array(
-    uint32_t object_instance, unsigned priority)
-{
-    uint32_t value = 1;
-    struct object_data *pObject;
-
-    pObject = Keylist_Data(Object_List, object_instance);
-    if (pObject) {
-        if ((priority >= 1) && (priority <= BACNET_MAX_PRIORITY)) {
-            value = pObject->Priority_Array[priority - 1];
-        }
-    }
-
-    return value;
-}
-
-/**
- * @brief For a given object instance-number and priority 1..16, determines
- *  if the priority-array slot is NULL
- * @param  object_instance - object-instance number of the object
- * @param  priority - priority-array index value 1..16
- * @return true if the priority array slot is NULL
- */
-static bool Multistate_Output_Priority_Array_Null(
-    uint32_t object_instance, unsigned priority)
-{
-    bool null_value = false;
-    struct object_data *pObject;
-
-    pObject = Keylist_Data(Object_List, object_instance);
-    if (pObject) {
-        if ((priority >= 1) && (priority <= BACNET_MAX_PRIORITY)) {
-            if (pObject->Relinquished[priority - 1]) {
-                null_value = true;
-            }
-        }
-    }
-
-    return null_value;
-}
-
 /**
  * @brief For a given object instance-number, determines the
  *  relinquish-default value
@@ -694,6 +676,35 @@ char *Multistate_Output_State_Text(
     return pName;
 }
 
+/**
+ * @brief Encode a BACnetARRAY property element
+ * @param object_instance [in] BACnet network port object instance number
+ * @param index [in] array index requested:
+ *    0 to N for individual array members
+ * @param apdu [out] Buffer in which the APDU contents are built, or NULL to
+ * return the length of buffer if it had been built
+ * @return The length of the apdu encoded or
+ *   BACNET_STATUS_ERROR for ERROR_CODE_INVALID_ARRAY_INDEX
+ */
+static int Multistate_Output_State_Text_Encode(
+    uint32_t object_instance, BACNET_ARRAY_INDEX index, uint8_t *apdu)
+{
+    int apdu_len = BACNET_STATUS_ERROR;
+    char *pName = NULL; /* return value */
+    BACNET_CHARACTER_STRING char_string = { 0 };
+    uint32_t state_index = 1;
+
+    state_index += index;
+    pName = Multistate_Output_State_Text(object_instance, state_index);
+    if (pName) {
+        characterstring_init_ansi(&char_string, pName);
+        apdu_len = encode_application_character_string(
+            apdu, &char_string);
+    }
+
+    return apdu_len;
+}
+
 /**
  * @brief For a given object instance-number, sets the list of state-text from
  * a C string array. The state_text_list consists of C strings separated
@@ -914,8 +925,8 @@ bool Multistate_Output_Encode_Value_List(
  */
 int Multistate_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
 {
-    int len = 0;
     int apdu_len = 0; /* return value */
+    int apdu_size = 0;
     BACNET_BIT_STRING bit_string;
     BACNET_CHARACTER_STRING char_string;
     uint32_t present_value = 0;
@@ -929,6 +940,7 @@ int Multistate_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
         return 0;
     }
     apdu = rpdata->application_data;
+    apdu_size = rpdata->application_data_len;
     switch (rpdata->object_property) {
         case PROP_OBJECT_IDENTIFIER:
             apdu_len = encode_application_object_id(
@@ -976,49 +988,15 @@ int Multistate_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
                 Multistate_Output_Max_States(rpdata->object_instance));
             break;
         case PROP_PRIORITY_ARRAY:
-            if (rpdata->array_index == 0) {
+            apdu_len = bacnet_array_encode(rpdata->object_instance,
-                /* Array element zero = the number of elements in the array */
+                rpdata->array_index, Multistate_Output_Priority_Array_Encode,
-                apdu_len =
+                BACNET_MAX_PRIORITY, apdu, apdu_size);
-                    encode_application_unsigned(&apdu[0], BACNET_MAX_PRIORITY);
+            if (apdu_len == BACNET_STATUS_ABORT) {
-            } else if (rpdata->array_index == BACNET_ARRAY_ALL) {
+                rpdata->error_code =
-                /* no index was specified; try to encode the entire list */
+                    ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                for (i = 1; i <= BACNET_MAX_PRIORITY; i++) {
+            } else if (apdu_len == BACNET_STATUS_ERROR) {
-                    if (Multistate_Output_Priority_Array_Null(
+                rpdata->error_class = ERROR_CLASS_PROPERTY;
-                            rpdata->object_instance, i)) {
+                rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                        len = encode_application_null(&apdu[apdu_len]);
-                    } else {
-                        present_value = Multistate_Output_Priority_Array(
-                            rpdata->object_instance, i);
-                        len = encode_application_unsigned(
-                            &apdu[apdu_len], present_value);
-                    }
-                    /* add it if we have room */
-                    if ((apdu_len + len) < MAX_APDU) {
-                        apdu_len += len;
-                    } else {
-                        /* Abort response */
-                        rpdata->error_code =
-                            ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                        apdu_len = BACNET_STATUS_ABORT;
-                        break;
-                    }
-                }
-            } else {
-                if (rpdata->array_index <= BACNET_MAX_PRIORITY) {
-                    if (Multistate_Output_Priority_Array_Null(
-                            rpdata->object_instance, rpdata->array_index)) {
-                        apdu_len = encode_application_null(&apdu[apdu_len]);
-                    } else {
-                        present_value = Multistate_Output_Priority_Array(
-                            rpdata->object_instance, rpdata->array_index);
-                        apdu_len = encode_application_unsigned(
-                            &apdu[apdu_len], present_value);
-                    }
-                } else {
-                    rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    apdu_len = BACNET_STATUS_ERROR;
-                }
             }
             break;
         case PROP_RELINQUISH_DEFAULT:
@@ -1028,42 +1006,15 @@ int Multistate_Output_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
             break;
         case PROP_STATE_TEXT:
             max_states = Multistate_Output_Max_States(rpdata->object_instance);
-            if (rpdata->array_index == 0) {
+            apdu_len = bacnet_array_encode(rpdata->object_instance,
-                /* Array element zero is the number of elements in the array */
+                rpdata->array_index, Multistate_Output_State_Text_Encode,
-                apdu_len = encode_application_unsigned(&apdu[0], max_states);
+                max_states, apdu, apdu_size);
-            } else if (rpdata->array_index == BACNET_ARRAY_ALL) {
+            if (apdu_len == BACNET_STATUS_ABORT) {
-                /* if no index was specified, then try to encode the entire list
+                rpdata->error_code =
-                 */
+                    ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED;
-                /* into one packet. */
+            } else if (apdu_len == BACNET_STATUS_ERROR) {
-                for (i = 1; i <= max_states; i++) {
+                rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    characterstring_init_ansi(&char_string,
+                rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                        Multistate_Output_State_Text(
-                            rpdata->object_instance, i));
-                    /* FIXME: this might go beyond MAX_APDU length! */
-                    len = encode_application_character_string(
-                        &apdu[apdu_len], &char_string);
-                    /* add it if we have room */
-                    if ((apdu_len + len) < MAX_APDU) {
-                        apdu_len += len;
-                    } else {
-                        rpdata->error_class = ERROR_CLASS_SERVICES;
-                        rpdata->error_code = ERROR_CODE_NO_SPACE_FOR_OBJECT;
-                        apdu_len = BACNET_STATUS_ERROR;
-                        break;
-                    }
-                }
-            } else {
-                if (rpdata->array_index <= max_states) {
-                    characterstring_init_ansi(&char_string,
-                        Multistate_Output_State_Text(
-                            rpdata->object_instance, rpdata->array_index));
-                    apdu_len = encode_application_character_string(
-                        &apdu[0], &char_string);
-                } else {
-                    rpdata->error_class = ERROR_CLASS_PROPERTY;
-                    rpdata->error_code = ERROR_CODE_INVALID_ARRAY_INDEX;
-                    apdu_len = BACNET_STATUS_ERROR;
-                }
             }
             break;
         case PROP_DESCRIPTION: