VeDirectFrameHandler icon indicating copy to clipboard operation
VeDirectFrameHandler copied to clipboard

temp buffer overflow possible

Open I-Connect opened this issue 3 years ago • 1 comments

Hi,

I am using your lib and want to point out a potential issue.

We had an issue with reading VE-Direct frames from the (software) serial buffer. This caused strange behavior like existing label names in the public buffer being overwritten (and most possibly also crashes)

I tracked it down to frameIndex becoming larger then FRAME_LEN causing the tempBuffer to overflow (in the rare case where multiple partial frames without a checksum label were somehow stored in the serial buffer after each other. Possibly because I was not reading a circular buffer fast enough).

Maybe it is rare but this can easily be caught by only increasing frameIndex when frameIndex < FRAME_LEN:

void VeDirectFrameHandler::textRxEvent(char* name, char* value) {
  if (frameIndex < FRAME_LEN) {
    strcpy(tempName[frameIndex], name);   // copy name to temporary buffer
    strcpy(tempValue[frameIndex], value); // copy value to temporary buffer
    frameIndex++;
  } else {
    log_w("temp buffer overrun!");
  }
}

Regards, Jeroen

I-Connect avatar Apr 14 '22 11:04 I-Connect

I just came here to log this exact issue.

Exact behaviour is variable depending on the extent of the overrun. I was experiencing crashing that took a while to tract down but it was sending frame lengths over 22 lines that was causing it.

The obvious solution is to modify my sender to limit frame size, but allowing an external source to arbitrarily write past the temp buffer is not good.

sticilface avatar Sep 27 '24 05:09 sticilface