Skip to content

Commit 7f2690f

Browse files
skargskarg
authored
Fixed a regression in the rpm_ack_object_property_process() function that prevented proper parsing of multi-object ReadPropertyMultiple ACK responses. The bug was introduced in PR bacnet-stack#765 and caused the function to incorrectly return ERROR_CODE_INVALID_TAG after processing the first object, even when additional valid objects were present in the response. Added tests that use rpm_ack_object_property_process() with a multi-object RPM ACK to verify the fix and prevent regression. (bacnet-stack#1183)
1 parent 2b32826 commit 7f2690f

3 files changed

Lines changed: 226 additions & 10 deletions

File tree

‎src/bacnet/rpm.c‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,30 @@ int rpm_encode_apdu_object_end(uint8_t *apdu)
107107
return apdu_len;
108108
}
109109

110+
/**
111+
* @brief Initialize an array (or single) #BACNET_READ_ACCESS_DATA linked list.
112+
* @param data - one or more #BACNET_READ_ACCESS_DATA elements
113+
* @param count - number of #BACNET_READ_ACCESS_DATA elements
114+
*/
115+
void bacnet_read_access_data_init(BACNET_READ_ACCESS_DATA *data, size_t count)
116+
{
117+
size_t i = 0;
118+
119+
if (data && count) {
120+
for (i = 0; i < count; i++) {
121+
data->object_type = OBJECT_NONE;
122+
data->object_instance = 0;
123+
data->listOfProperties = NULL;
124+
if ((i + 1) < count) {
125+
data->next = data + 1;
126+
} else {
127+
data->next = NULL;
128+
}
129+
data++;
130+
}
131+
}
132+
}
133+
110134
/**
111135
* @brief Encode the ReadPropertyMultiple-Request
112136
* @param apdu application data unit buffer for encoding, or NULL for length
@@ -692,15 +716,7 @@ void rpm_ack_object_property_process(
692716
if (bacnet_is_closing_tag_number(apdu, apdu_len, 1, &len)) {
693717
/* end of list-of-results [1] SEQUENCE OF SEQUENCE */
694718
apdu_len -= len;
695-
if (apdu_len > 0) {
696-
/* malformed */
697-
rp_data->error_class = ERROR_CLASS_SERVICES;
698-
rp_data->error_code = ERROR_CODE_INVALID_TAG;
699-
if (callback) {
700-
callback(device_id, rp_data);
701-
}
702-
return;
703-
}
719+
apdu += len;
704720
break;
705721
}
706722
len = rpm_ack_decode_object_property(

‎src/bacnet/rpm.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ int rpm_encode_apdu_object_property(
8686
BACNET_STACK_EXPORT
8787
int rpm_encode_apdu_object_end(uint8_t *apdu);
8888

89+
BACNET_STACK_EXPORT
90+
void bacnet_read_access_data_init(BACNET_READ_ACCESS_DATA *data, size_t count);
8991
BACNET_STACK_EXPORT
9092
int read_property_multiple_request_encode(
9193
uint8_t *apdu, BACNET_READ_ACCESS_DATA *data);

‎test/bacnet/rpm/src/main.c‎

Lines changed: 199 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,66 @@ static void testReadPropertyMultiple(void)
204204
zassert_equal(len, service_request_len, NULL);
205205
}
206206

207+
#if defined(CONFIG_ZTEST_NEW_API)
208+
ZTEST(rpm_tests, testReadPropertyMultipleRequest)
209+
#else
210+
static void testReadPropertyMultipleRequest(void)
211+
#endif
212+
{
213+
uint8_t apdu[480] = { 0 };
214+
int null_len = 0;
215+
int apdu_len = 0;
216+
uint8_t invoke_id = 12;
217+
BACNET_READ_ACCESS_DATA read_access_data[2] = { 0 };
218+
BACNET_PROPERTY_REFERENCE property_list_all = { 0 };
219+
BACNET_PROPERTY_REFERENCE property_list_array = { 0 };
220+
uint32_t device_id = 123;
221+
222+
bacnet_read_access_data_init(
223+
&read_access_data[0], ARRAY_SIZE(read_access_data));
224+
/* configure property list #1 */
225+
property_list_all.error.error_class = ERROR_CLASS_DEVICE;
226+
property_list_all.error.error_code = ERROR_CODE_OTHER;
227+
property_list_all.value = NULL;
228+
property_list_all.propertyArrayIndex = BACNET_ARRAY_ALL;
229+
property_list_all.propertyIdentifier = PROP_ALL;
230+
property_list_all.next = NULL;
231+
/* configure the read access data #1 */
232+
read_access_data[0].listOfProperties = &property_list_all;
233+
read_access_data[0].object_instance = device_id;
234+
read_access_data[0].object_type = OBJECT_DEVICE;
235+
/* configure property list #2 */
236+
property_list_all.error.error_class = ERROR_CLASS_DEVICE;
237+
property_list_all.error.error_code = ERROR_CODE_OTHER;
238+
property_list_all.value = NULL;
239+
property_list_all.propertyArrayIndex = 0;
240+
property_list_all.propertyIdentifier = PROP_PRIORITY_ARRAY;
241+
property_list_all.next = NULL;
242+
/* configure the read access data #2 */
243+
read_access_data[1].listOfProperties = &property_list_array;
244+
read_access_data[1].object_instance = 42;
245+
read_access_data[1].object_type = OBJECT_ANALOG_OUTPUT;
246+
247+
null_len = read_property_multiple_request_service_encode(
248+
NULL, sizeof(apdu), &read_access_data[0]);
249+
apdu_len = read_property_multiple_request_service_encode(
250+
apdu, sizeof(apdu), &read_access_data[0]);
251+
zassert_equal(apdu_len, null_len, NULL);
252+
zassert_true(apdu_len > 0, NULL);
253+
null_len = read_property_multiple_request_service_encode(
254+
NULL, 0, &read_access_data[0]);
255+
zassert_equal(null_len, 0, NULL);
256+
257+
null_len =
258+
rpm_encode_apdu(NULL, sizeof(apdu), invoke_id, &read_access_data[0]);
259+
apdu_len =
260+
rpm_encode_apdu(apdu, sizeof(apdu), invoke_id, &read_access_data[0]);
261+
zassert_equal(apdu_len, null_len, NULL);
262+
zassert_true(apdu_len > 0, NULL);
263+
null_len = rpm_encode_apdu(NULL, 0, invoke_id, &read_access_data[0]);
264+
zassert_equal(null_len, 0, NULL);
265+
}
266+
207267
#if defined(CONFIG_ZTEST_NEW_API)
208268
ZTEST(rpm_tests, testReadPropertyMultipleAck)
209269
#else
@@ -432,6 +492,142 @@ static void testReadPropertyMultipleAck(void)
432492
zassert_equal(test_len, 0, NULL);
433493
zassert_equal(len, service_request_len, NULL);
434494
}
495+
496+
#define TEST_READ_PROPERTY_ACK_DATA_COUNT 5
497+
static BACNET_READ_PROPERTY_DATA
498+
Read_Property_Ack_Data[TEST_READ_PROPERTY_ACK_DATA_COUNT];
499+
static uint32_t Read_Property_Ack_Device_ID[TEST_READ_PROPERTY_ACK_DATA_COUNT];
500+
static size_t Read_Property_Ack_Count = 0;
501+
/**
502+
* @brief Process a ReadProperty-ACK message
503+
* @param device_id [in] The device ID of the source of the message
504+
* @param rp_data [in] The contents of the service request.
505+
*/
506+
static void bacnet_read_property_ack_process(
507+
uint32_t device_id, BACNET_READ_PROPERTY_DATA *rp_data)
508+
{
509+
if (Read_Property_Ack_Count < TEST_READ_PROPERTY_ACK_DATA_COUNT) {
510+
Read_Property_Ack_Device_ID[Read_Property_Ack_Count] = device_id;
511+
memcpy(
512+
&Read_Property_Ack_Data[Read_Property_Ack_Count], rp_data,
513+
sizeof(BACNET_READ_PROPERTY_DATA));
514+
}
515+
Read_Property_Ack_Count++;
516+
}
517+
518+
#if defined(CONFIG_ZTEST_NEW_API)
519+
ZTEST(rpm_tests, testReadPropertyMultipleAck)
520+
#else
521+
static void testReadPropertyMultipleAckProcess(void)
522+
#endif
523+
{
524+
uint8_t apdu[480] = { 0 };
525+
int apdu_len = 0;
526+
BACNET_APPLICATION_DATA_VALUE application_data[3] = { { 0 } };
527+
uint8_t application_data_buffer[MAX_APDU] = { 0 };
528+
int application_data_buffer_len = 0;
529+
BACNET_RPM_DATA rpmdata;
530+
BACNET_READ_PROPERTY_DATA rp_data = { 0 };
531+
uint32_t device_id = 0;
532+
533+
/* first object beginning */
534+
rpmdata.object_type = OBJECT_DEVICE;
535+
rpmdata.object_instance = 123;
536+
apdu_len += rpm_ack_encode_apdu_object_begin(&apdu[apdu_len], &rpmdata);
537+
/* reply property */
538+
apdu_len += rpm_ack_encode_apdu_object_property(
539+
&apdu[apdu_len], PROP_OBJECT_IDENTIFIER, BACNET_ARRAY_ALL);
540+
/* reply value */
541+
application_data[0].tag = BACNET_APPLICATION_TAG_OBJECT_ID;
542+
application_data[0].type.Object_Id.type = OBJECT_DEVICE;
543+
application_data[0].type.Object_Id.instance = 123;
544+
application_data_buffer_len = bacapp_encode_application_data(
545+
&application_data_buffer[0], &application_data[0]);
546+
apdu_len += rpm_ack_encode_apdu_object_property_value(
547+
&apdu[apdu_len], &application_data_buffer[0],
548+
application_data_buffer_len);
549+
/* reply property */
550+
apdu_len += rpm_ack_encode_apdu_object_property(
551+
&apdu[apdu_len], PROP_OBJECT_TYPE, BACNET_ARRAY_ALL);
552+
/* reply value */
553+
application_data[1].tag = BACNET_APPLICATION_TAG_ENUMERATED;
554+
application_data[1].type.Enumerated = OBJECT_DEVICE;
555+
application_data_buffer_len = bacapp_encode_application_data(
556+
&application_data_buffer[0], &application_data[1]);
557+
apdu_len += rpm_ack_encode_apdu_object_property_value(
558+
&apdu[apdu_len], &application_data_buffer[0],
559+
application_data_buffer_len);
560+
/* first object end */
561+
apdu_len += rpm_ack_encode_apdu_object_end(&apdu[apdu_len]);
562+
563+
/* second object beginning */
564+
rpmdata.object_type = OBJECT_ANALOG_INPUT;
565+
rpmdata.object_instance = 33;
566+
apdu_len += rpm_ack_encode_apdu_object_begin(&apdu[apdu_len], &rpmdata);
567+
/* reply property */
568+
apdu_len += rpm_ack_encode_apdu_object_property(
569+
&apdu[apdu_len], PROP_PRESENT_VALUE, BACNET_ARRAY_ALL);
570+
/* reply value */
571+
application_data[2].tag = BACNET_APPLICATION_TAG_REAL;
572+
application_data[2].type.Real = 0.0;
573+
application_data_buffer_len = bacapp_encode_application_data(
574+
&application_data_buffer[0], &application_data[2]);
575+
apdu_len += rpm_ack_encode_apdu_object_property_value(
576+
&apdu[apdu_len], &application_data_buffer[0],
577+
application_data_buffer_len);
578+
/* reply property */
579+
apdu_len += rpm_ack_encode_apdu_object_property(
580+
&apdu[apdu_len], PROP_DEADBAND, BACNET_ARRAY_ALL);
581+
/* reply error */
582+
apdu_len += rpm_ack_encode_apdu_object_property_error(
583+
&apdu[apdu_len], ERROR_CLASS_PROPERTY, ERROR_CODE_UNKNOWN_PROPERTY);
584+
/* second object end */
585+
apdu_len += rpm_ack_encode_apdu_object_end(&apdu[apdu_len]);
586+
zassert_not_equal(apdu_len, 0, NULL);
587+
588+
rpm_ack_object_property_process(
589+
apdu, apdu_len, device_id, &rp_data, bacnet_read_property_ack_process);
590+
zassert_equal(
591+
Read_Property_Ack_Count, 4, "RPM-ACK count=%zu, expected %d",
592+
Read_Property_Ack_Count, 4);
593+
594+
zassert_equal(
595+
Read_Property_Ack_Data[0].object_property, PROP_OBJECT_IDENTIFIER,
596+
NULL);
597+
zassert_equal(Read_Property_Ack_Data[0].object_type, OBJECT_DEVICE, NULL);
598+
zassert_equal(Read_Property_Ack_Data[0].object_instance, 123, NULL);
599+
600+
zassert_equal(
601+
Read_Property_Ack_Data[1].object_property, PROP_OBJECT_TYPE, NULL);
602+
zassert_equal(Read_Property_Ack_Data[1].object_type, OBJECT_DEVICE, NULL);
603+
zassert_equal(Read_Property_Ack_Data[1].object_instance, 123, NULL);
604+
605+
zassert_equal(
606+
Read_Property_Ack_Data[2].object_property, PROP_PRESENT_VALUE, NULL);
607+
zassert_equal(
608+
Read_Property_Ack_Data[2].object_type, OBJECT_ANALOG_INPUT, NULL);
609+
zassert_equal(Read_Property_Ack_Data[2].object_instance, 33, NULL);
610+
611+
zassert_equal(
612+
Read_Property_Ack_Data[3].object_property, PROP_DEADBAND, NULL);
613+
zassert_equal(
614+
Read_Property_Ack_Data[3].object_type, OBJECT_ANALOG_INPUT, NULL);
615+
zassert_equal(Read_Property_Ack_Data[3].object_instance, 33, NULL);
616+
617+
/* extra data at the end */
618+
Read_Property_Ack_Count = 0;
619+
rpm_ack_object_property_process(
620+
apdu, apdu_len + 2, device_id, &rp_data,
621+
bacnet_read_property_ack_process);
622+
zassert_equal(
623+
Read_Property_Ack_Count, 5, "RPM-ACK count=%zu, expected %d",
624+
Read_Property_Ack_Count, 5);
625+
zassert_equal(
626+
Read_Property_Ack_Data[4].error_class, ERROR_CLASS_SERVICES, NULL);
627+
zassert_equal(
628+
Read_Property_Ack_Data[4].error_code, ERROR_CODE_INVALID_TAG, NULL);
629+
}
630+
435631
/**
436632
* @}
437633
*/
@@ -443,7 +639,9 @@ void test_main(void)
443639
{
444640
ztest_test_suite(
445641
rpm_tests, ztest_unit_test(testReadPropertyMultiple),
446-
ztest_unit_test(testReadPropertyMultipleAck));
642+
ztest_unit_test(testReadPropertyMultipleRequest),
643+
ztest_unit_test(testReadPropertyMultipleAck),
644+
ztest_unit_test(testReadPropertyMultipleAckProcess));
447645

448646
ztest_run_test_suite(rpm_tests);
449647
}

0 commit comments

Comments
 (0)