From 764e0e84480ebdb5861c5b4ed1308af169161779 Mon Sep 17 00:00:00 2001 From: Roy Schneider Date: Sun, 24 May 2020 16:19:52 +0200 Subject: [PATCH] Feature/zeroing rx buffer remain (#90) * Added zeroing rx buffer remain * Added zeroing rx buffer remain * Added safety margin for the rx-buffer in the different ports. * Added safety margin for the receive buffer. * Added DoxyGen comments. * Fixed checking return value when calculating distance between opening and closing tag on multiple properties. --- ports/arduino_uno/bip.c | 13 ++++++++ ports/arduino_uno/main.c | 14 ++++++-- ports/at91sam7s/main.c | 14 ++++++-- ports/atmega168/main.c | 15 +++++++-- ports/bdk-atxx4-mstp/bacnet.c | 14 ++++++-- ports/bsd/bip-init.c | 13 +++++++- ports/bsd/main.c | 10 ++++-- ports/esp32/src/main.c | 12 +++++-- ports/linux/bip-init.c | 13 +++++++- ports/rx62n/bacnet.c | 12 +++++-- ports/stm32f10x/bacnet.c | 14 ++++++-- ports/win32/bip-init.c | 13 +++++++- ports/win32/main.c | 10 ++++-- src/bacnet/bacapp.c | 16 ++++++--- src/bacnet/timestamp.c | 62 ++++++++++++++++++++++++++++++++++- src/bacnet/wpm.c | 33 +++++++++++-------- 16 files changed, 238 insertions(+), 40 deletions(-) diff --git a/ports/arduino_uno/bip.c b/ports/arduino_uno/bip.c index a24d4f1e..0920e19e 100644 --- a/ports/arduino_uno/bip.c +++ b/ports/arduino_uno/bip.c @@ -244,6 +244,7 @@ uint16_t bip_receive(BACNET_ADDRESS *src, /* source address */ unsigned timeout) { int received_bytes = 0; + int max = 0; uint16_t pdu_len = 0; /* return value */ uint8_t src_addr[] = { 0, 0, 0, 0 }; uint16_t src_port = 0; @@ -274,6 +275,18 @@ uint16_t bip_receive(BACNET_ADDRESS *src, /* source address */ if (pdu[0] != BVLL_TYPE_BACNET_IP) return 0; + /* Erase up to 16 bytes after the received bytes as safety margin to + * ensure that the decoding functions will run into a 'safe field' + * of zero, if for any reason they would overrun, when parsing the + * message. */ + max = (int)max_pdu - received_bytes; + if (max > 0) { + if (max > 16) { + max = 16; + } + memset(&pdu[received_bytes], 0, max); + } + if (bvlc_for_non_bbmd(src_addr, &src_port, pdu, received_bytes) > 0) { /* Handled, usually with a NACK. */ #if PRINT_ENABLED diff --git a/ports/arduino_uno/main.c b/ports/arduino_uno/main.c index 7634e1e1..ca4e2bde 100644 --- a/ports/arduino_uno/main.c +++ b/ports/arduino_uno/main.c @@ -70,7 +70,17 @@ void setup() #endif } -static uint8_t PDUBuffer[MAX_MPDU]; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t PDUBuffer[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + +/** Main */ + int main(void) { uint16_t pdu_len = 0; @@ -84,7 +94,7 @@ int main(void) for (;;) { /* other tasks */ /* BACnet handling */ - pdu_len = datalink_receive(&src, &PDUBuffer[0], sizeof(PDUBuffer), 0); + pdu_len = datalink_receive(&src, &PDUBuffer[0], MAX_MPDU, 0); if (pdu_len) { npdu_handler(&src, &PDUBuffer[0], pdu_len); } diff --git a/ports/at91sam7s/main.c b/ports/at91sam7s/main.c index 09613cbc..9b67ccf7 100644 --- a/ports/at91sam7s/main.c +++ b/ports/at91sam7s/main.c @@ -162,7 +162,17 @@ static inline void bacnet_init(void) handler_device_communication_control); } -static uint8_t Receive_PDU[MAX_MPDU]; /* PDU data */ +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t Receive_PDU[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + +/** Main */ + int main(void) { unsigned long IdleCount = 0; /* idle loop blink counter */ @@ -240,7 +250,7 @@ int main(void) IdleCount++; /* BACnet handling */ pdu_len = - datalink_receive(&src, &Receive_PDU[0], sizeof(Receive_PDU), 0); + datalink_receive(&src, &Receive_PDU[0], MAX_MPDU, 0); if (pdu_len) { pPIO->PIO_CODR = LED3; npdu_handler(&src, &Receive_PDU[0], pdu_len); diff --git a/ports/atmega168/main.c b/ports/atmega168/main.c index 935cfa26..0595a71f 100644 --- a/ports/atmega168/main.c +++ b/ports/atmega168/main.c @@ -135,7 +135,18 @@ static void input_switch_read(void) } } -static uint8_t PDUBuffer[MAX_MPDU]; + +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t PDUBuffer[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + +/** Main */ + int main(void) { uint16_t pdu_len = 0; @@ -153,7 +164,7 @@ int main(void) task_milliseconds(); /* other tasks */ /* BACnet handling */ - pdu_len = datalink_receive(&src, &PDUBuffer[0], sizeof(PDUBuffer), 0); + pdu_len = datalink_receive(&src, &PDUBuffer[0], MAX_MPDU, 0); if (pdu_len) { LED_NPDU_ON(); npdu_handler(&src, &PDUBuffer[0], pdu_len); diff --git a/ports/bdk-atxx4-mstp/bacnet.c b/ports/bdk-atxx4-mstp/bacnet.c index 4ad249d4..536a654b 100644 --- a/ports/bdk-atxx4-mstp/bacnet.c +++ b/ports/bdk-atxx4-mstp/bacnet.c @@ -141,7 +141,17 @@ void bacnet_init(void) Send_I_Am(&Handler_Transmit_Buffer[0]); } -static uint8_t PDUBuffer[MAX_MPDU]; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t PDUBuffer[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + +/** BACnet task doing receive and transmit. */ + void bacnet_task(void) { uint8_t mstp_mac_address; @@ -208,7 +218,7 @@ void bacnet_task(void) dcc_timer_seconds(DCC_CYCLE_SECONDS); } /* handle the messaging */ - pdu_len = datalink_receive(&src, &PDUBuffer[0], sizeof(PDUBuffer), 0); + pdu_len = datalink_receive(&src, &PDUBuffer[0], MAX_MPDU, 0); if (pdu_len) { npdu_handler(&src, &PDUBuffer[0], pdu_len); } diff --git a/ports/bsd/bip-init.c b/ports/bsd/bip-init.c index 53cdf969..7a94b58a 100644 --- a/ports/bsd/bip-init.c +++ b/ports/bsd/bip-init.c @@ -319,7 +319,7 @@ uint16_t bip_receive( max = BIP_Socket; /* see if there is a packet for us */ if (select(max + 1, &read_fds, NULL, NULL, &select_timeout) > 0) { - received_bytes = recvfrom(BIP_Socket, (char *)&npdu[0], max_npdu, 0, + received_bytes = recvfrom(max, (char *)&npdu[0], max_npdu, 0, (struct sockaddr *)&sin, &sin_len); } else { return 0; @@ -336,6 +336,17 @@ uint16_t bip_receive( if (npdu[0] != BVLL_TYPE_BACNET_IP) { return 0; } + /* Erase up to 16 bytes after the received bytes as safety margin to + * ensure that the decoding functions will run into a 'safe field' + * of zero, if for any reason they would overrun, when parsing the + * message. */ + max = (int)max_npdu - received_bytes; + if (max > 0) { + if (max > 16) { + max = 16; + } + memset(&npdu[received_bytes], 0, max); + } /* Data link layer addressing between B/IPv4 nodes consists of a 32-bit IPv4 address followed by a two-octet UDP port number (both of which shall be transmitted with the most significant octet first). This diff --git a/ports/bsd/main.c b/ports/bsd/main.c index 6d6702ae..e574ccc6 100644 --- a/ports/bsd/main.c +++ b/ports/bsd/main.c @@ -51,8 +51,14 @@ bool Who_Is_Request = true; -/* buffers used for receiving */ -static uint8_t Rx_Buf[MAX_MPDU] = { 0 }; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t Rx_Buf[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */] = { 0 }; static void LocalIAmHandler( uint8_t *service_request, uint16_t service_len, BACNET_ADDRESS *src) diff --git a/ports/esp32/src/main.c b/ports/esp32/src/main.c index c4dc63d2..0c2f4e5a 100644 --- a/ports/esp32/src/main.c +++ b/ports/esp32/src/main.c @@ -48,7 +48,15 @@ wifi_config_t wifi_config = { #define BACNET_LED 5 uint8_t Handler_Transmit_Buffer[MAX_PDU] = { 0 }; -uint8_t Rx_Buf[MAX_MPDU] = { 0 }; + +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +uint8_t Rx_Buf[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */] = { 0 }; EventGroupHandle_t wifi_event_group; const static int CONNECTED_BIT = BIT0; @@ -207,4 +215,4 @@ void app_main() NULL, /* Task input parameter */ 20, /* Priority of the task */ NULL); /* Task handle. */ -} \ No newline at end of file +} diff --git a/ports/linux/bip-init.c b/ports/linux/bip-init.c index a6718eff..ab284f6f 100644 --- a/ports/linux/bip-init.c +++ b/ports/linux/bip-init.c @@ -331,7 +331,7 @@ uint16_t bip_receive( max = BIP_Socket; /* see if there is a packet for us */ if (select(max + 1, &read_fds, NULL, NULL, &select_timeout) > 0) { - received_bytes = recvfrom(BIP_Socket, (char *)&npdu[0], max_npdu, 0, + received_bytes = recvfrom(max, (char *)&npdu[0], max_npdu, 0, (struct sockaddr *)&sin, &sin_len); } else { return 0; @@ -348,6 +348,17 @@ uint16_t bip_receive( if (npdu[0] != BVLL_TYPE_BACNET_IP) { return 0; } + /* Erase up to 16 bytes after the received bytes as safety margin to + * ensure that the decoding functions will run into a 'safe field' + * of zero, if for any reason they would overrun, when parsing the + * message. */ + max = (int)max_npdu - received_bytes; + if (max > 0) { + if (max > 16) { + max = 16; + } + memset(&npdu[received_bytes], 0, max); + } /* Data link layer addressing between B/IPv4 nodes consists of a 32-bit IPv4 address followed by a two-octet UDP port number (both of which shall be transmitted with the most significant octet first). This diff --git a/ports/rx62n/bacnet.c b/ports/rx62n/bacnet.c index b5bd59c0..b25c94ab 100644 --- a/ports/rx62n/bacnet.c +++ b/ports/rx62n/bacnet.c @@ -74,7 +74,15 @@ void bacnet_init(void) Send_I_Am(&Handler_Transmit_Buffer[0]); } -static uint8_t PDUBuffer[MAX_MPDU]; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t PDUBuffer[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + void bacnet_task(void) { uint16_t pdu_len; @@ -110,7 +118,7 @@ void bacnet_task(void) dcc_timer_seconds(DCC_CYCLE_SECONDS); } /* handle the messaging */ - pdu_len = datalink_receive(&src, &PDUBuffer[0], sizeof(PDUBuffer), 0); + pdu_len = datalink_receive(&src, &PDUBuffer[0], MAX_MPDU, 0); if (pdu_len) { npdu_handler(&src, &PDUBuffer[0], pdu_len); } diff --git a/ports/stm32f10x/bacnet.c b/ports/stm32f10x/bacnet.c index 8c941c4c..2496918e 100644 --- a/ports/stm32f10x/bacnet.c +++ b/ports/stm32f10x/bacnet.c @@ -79,7 +79,17 @@ void bacnet_init(void) Send_I_Am(&Handler_Transmit_Buffer[0]); } -static uint8_t PDUBuffer[MAX_MPDU]; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t PDUBuffer[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */]; + +/** BACnet task handling receiving and transmitting of messages. */ + void bacnet_task(void) { uint16_t pdu_len; @@ -123,7 +133,7 @@ void bacnet_task(void) dcc_timer_seconds(DCC_CYCLE_SECONDS); } /* handle the messaging */ - pdu_len = datalink_receive(&src, &PDUBuffer[0], sizeof(PDUBuffer), 0); + pdu_len = datalink_receive(&src, &PDUBuffer[0], MAX_MPDU, 0); if (pdu_len) { npdu_handler(&src, &PDUBuffer[0], pdu_len); } diff --git a/ports/win32/bip-init.c b/ports/win32/bip-init.c index e9495e56..5087aff8 100644 --- a/ports/win32/bip-init.c +++ b/ports/win32/bip-init.c @@ -484,7 +484,7 @@ uint16_t bip_receive( max = BIP_Socket; /* see if there is a packet for us */ if (select(max + 1, &read_fds, NULL, NULL, &select_timeout) > 0) { - received_bytes = recvfrom(BIP_Socket, (char *)&npdu[0], max_npdu, 0, + received_bytes = recvfrom(max, (char *)&npdu[0], max_npdu, 0, (struct sockaddr *)&sin, &sin_len); } else { return 0; @@ -501,6 +501,17 @@ uint16_t bip_receive( if (npdu[0] != BVLL_TYPE_BACNET_IP) { return 0; } + /* Erase up to 16 bytes after the received bytes as safety margin to + * ensure that the decoding functions will run into a 'safe field' + * of zero, if for any reason they would overrun, when parsing the + * message. */ + max = (int)max_npdu - received_bytes; + if (max > 0) { + if (max > 16) { + max = 16; + } + memset(&npdu[received_bytes], 0, max); + } /* Data link layer addressing between B/IPv4 nodes consists of a 32-bit IPv4 address followed by a two-octet UDP port number (both of which shall be transmitted with the most significant octet first). This diff --git a/ports/win32/main.c b/ports/win32/main.c index e582467b..317cc732 100644 --- a/ports/win32/main.c +++ b/ports/win32/main.c @@ -59,8 +59,14 @@ #include "bacnet/basic/object/bacfile.h" #endif -/* buffer used for receive */ -static uint8_t Rx_Buf[MAX_MPDU] = { 0 }; +/** Static receive buffer, initialized with zeros by the C Library Startup Code. */ + +static uint8_t Rx_Buf[MAX_MPDU + 16 /* Add a little safety margin to the buffer, + * so that in the rare case, the message + * would be filled up to MAX_MPDU and some + * decoding functions would overrun, these + * decoding functions will just end up in + * a safe field of static zeros. */] = { 0 }; /* send a whois to see who is on the network */ static bool Who_Is_Request = true; diff --git a/src/bacnet/bacapp.c b/src/bacnet/bacapp.c index 7347114d..b9731750 100644 --- a/src/bacnet/bacapp.c +++ b/src/bacnet/bacapp.c @@ -926,10 +926,18 @@ bool bacapp_copy(BACNET_APPLICATION_DATA_VALUE *dest_value, return status; } -/* returns the length of data between an opening tag and a closing tag. - Expects that the first octet contain the opening tag. - Include a value property identifier for context specific data - such as the value received in a WriteProperty request */ +/** + * @brief Returns the length of data between an opening tag and a closing tag. + * Expects that the first octet contain the opening tag. + * Include a value property identifier for context specific data + * such as the value received in a WriteProperty request. + * + * @param Pointer to the APDU buffer + * @param apdu_len_max Bytes valid in the buffer + * @param property ID of the propery to get the length for. + * + * @return Length in bytes or BACNET_STATUS_ERROR. + */ int bacapp_data_len( uint8_t *apdu, unsigned apdu_len_max, BACNET_PROPERTY_ID property) { diff --git a/src/bacnet/timestamp.c b/src/bacnet/timestamp.c index ee90eb39..25557965 100644 --- a/src/bacnet/timestamp.c +++ b/src/bacnet/timestamp.c @@ -37,6 +37,11 @@ /** @file timestamp.c Encode/Decode BACnet Timestamps */ +/** Set the sequence number in a timestamp structure. + * + * @param dest Pointer to the destination time stamp structure. + * @param sequenceNum Sequence number to be set into the time stamp. + */ void bacapp_timestamp_sequence_set(BACNET_TIMESTAMP *dest, uint16_t sequenceNum) { if (dest) { @@ -45,6 +50,12 @@ void bacapp_timestamp_sequence_set(BACNET_TIMESTAMP *dest, uint16_t sequenceNum) } } +/** Set a timestamp structure with the value given + * from a time structure. + * + * @param dest Pointer to the destination time stamp structure. + * @param btime Pointer to the BACNet time structure. + */ void bacapp_timestamp_time_set(BACNET_TIMESTAMP *dest, BACNET_TIME *btime) { if (dest && btime) { @@ -53,6 +64,12 @@ void bacapp_timestamp_time_set(BACNET_TIMESTAMP *dest, BACNET_TIME *btime) } } +/** Set a timestamp structure with the value given + * from a date/time structure. + * + * @param dest Pointer to the destination time stamp structure. + * @param bdateTime Pointer to the BACNet date/time structure. + */ void bacapp_timestamp_datetime_set( BACNET_TIMESTAMP *dest, BACNET_DATE_TIME *bdateTime) { @@ -62,6 +79,11 @@ void bacapp_timestamp_datetime_set( } } +/** Copy a timestamp depending of the tag it holds. + * + * @param dest Pointer to the destination time stamp structure. + * @param src Pointer to the source time stamp structure. + */ void bacapp_timestamp_copy(BACNET_TIMESTAMP *dest, BACNET_TIMESTAMP *src) { if (dest && src) { @@ -82,6 +104,14 @@ void bacapp_timestamp_copy(BACNET_TIMESTAMP *dest, BACNET_TIMESTAMP *src) } } +/** Encode a time stamp. + * + * @param apdu Pointer to the APDU buffer. + * @param value Pointer to the variable that holds + * the time stamp values to be encoded. + * + * @return Bytes encoded or 0 on error. + */ int bacapp_encode_timestamp(uint8_t *apdu, BACNET_TIMESTAMP *value) { int len = 0; /* length of each encoding */ @@ -112,6 +142,17 @@ int bacapp_encode_timestamp(uint8_t *apdu, BACNET_TIMESTAMP *value) return len; } +/** Encode a time stamp for the given tag + * number, using an opening and closing tag. + * + * @param apdu Pointer to the APDU buffer. + * @param tag_number The tag number that shall + * hold the time stamp. + * @param value Pointer to the variable that holds + * the time stamp values to be encoded. + * + * @return Bytes encoded or 0 on error. + */ int bacapp_encode_context_timestamp( uint8_t *apdu, uint8_t tag_number, BACNET_TIMESTAMP *value) { @@ -129,6 +170,14 @@ int bacapp_encode_context_timestamp( return apdu_len; } +/** Decode a time stamp from the given buffer. + * + * @param apdu Pointer to the APDU buffer. + * @param value Pointer to the variable that shall + * take the time stamp values. + * + * @return Bytes decoded or -1 on error. + */ int bacapp_decode_timestamp(uint8_t *apdu, BACNET_TIMESTAMP *value) { int len = 0; @@ -136,7 +185,7 @@ int bacapp_decode_timestamp(uint8_t *apdu, BACNET_TIMESTAMP *value) uint32_t len_value_type; BACNET_UNSIGNED_INTEGER unsigned_value; - if (apdu) { + if (apdu && value) { section_len = decode_tag_number_and_value( &apdu[len], &value->tag, &len_value_type); @@ -184,6 +233,17 @@ int bacapp_decode_timestamp(uint8_t *apdu, BACNET_TIMESTAMP *value) return len; } +/** Decode a time stamp and check for + * opening and closing tags. + * + * @param apdu Pointer to the APDU buffer. + * @param tag_number The tag number that shall + * hold the time stamp. + * @param value Pointer to the variable that shall + * take the time stamp values. + * + * @return Bytes decoded or -1 on error. + */ int bacapp_decode_context_timestamp( uint8_t *apdu, uint8_t tag_number, BACNET_TIMESTAMP *value) { diff --git a/src/bacnet/wpm.c b/src/bacnet/wpm.c index d260c9e8..f7bab75d 100644 --- a/src/bacnet/wpm.c +++ b/src/bacnet/wpm.c @@ -151,20 +151,25 @@ int wpm_decode_object_property( (unsigned)(apdu_len - len), wp_data->object_property); len++; - /* copy application data, check max lengh */ - if (imax > (apdu_len - len)) { - imax = (apdu_len - len); - } - for (i = 0; i < imax; i++) { - wp_data->application_data[i] = apdu[len + i]; - } - wp_data->application_data_len = imax; - len += imax; - if (len < apdu_len) { - len += decode_tag_number_and_value( - &apdu[len], &tag_number, &len_value); - /* closing tag 2 */ - if ((tag_number != 2) && (decode_is_closing_tag(&apdu[len - 1]))) { + if (imax != BACNET_STATUS_ERROR) { + /* copy application data, check max lengh */ + if (imax > (apdu_len - len)) { + imax = (apdu_len - len); + } + for (i = 0; i < imax; i++) { + wp_data->application_data[i] = apdu[len + i]; + } + wp_data->application_data_len = imax; + len += imax; + if (len < apdu_len) { + len += decode_tag_number_and_value( + &apdu[len], &tag_number, &len_value); + /* closing tag 2 */ + if ((tag_number != 2) && (decode_is_closing_tag(&apdu[len - 1]))) { + wp_data->error_code = ERROR_CODE_REJECT_INVALID_TAG; + return BACNET_STATUS_REJECT; + } + } else { wp_data->error_code = ERROR_CODE_REJECT_INVALID_TAG; return BACNET_STATUS_REJECT; }