This code is simple and elegant, however it is prone to buffer overflows because it blindly trust Serial inputs coming from the VE.Direct device.
The two weaknesses are:
- When parsing the data, the blockindex counter is not limited in any way, and there are conditions where it could exceed recv_label and recv_value arrays, leading to array overflow and undefined behaviour (it is really sneaky and took me weeks to figure it out)
- use of strcpy functions instead of safer strlcpy functions
I suggest to add a check before copying strings into the arrays, and use strlcpy to do so
if(blockindex < num_keywords) {
strlcpy(recv_label[blockindex], strtokIndx,sizeof(recv_label[blockindex])-1);
}
if(blockindex < num_keywords) {
strlcpy(recv_value[blockindex], strtokIndx,sizeof(recv_value[blockindex])-1);
}
Additionally, in the data parsing function, when checksum control is passed, set blockindex counter to the num_keywords - 1 number if it exceed that, otherwise this may lead to another array overflow in the value array
if(blockindex >= num_keywords) blockindex = num_keywords - 1;
Yes, we may lose data this way, but VE.Direct data is sent every second, so it would be quickly restored, and more importantly, the program doesn't get into undefined behavior.
This code is simple and elegant, however it is prone to buffer overflows because it blindly trust Serial inputs coming from the VE.Direct device.
The two weaknesses are:
I suggest to add a check before copying strings into the arrays, and use strlcpy to do so
Additionally, in the data parsing function, when checksum control is passed, set blockindex counter to the num_keywords - 1 number if it exceed that, otherwise this may lead to another array overflow in the value array
if(blockindex >= num_keywords) blockindex = num_keywords - 1;Yes, we may lose data this way, but VE.Direct data is sent every second, so it would be quickly restored, and more importantly, the program doesn't get into undefined behavior.