Fix multiple buffer overflows and a buffer underflow.

[SeaTechRC]

Hello,

Upon investigation there are multiple places where buffer-overflows could occur aswell as one case of a potential underflow in strip().
The solution i propose is: instead of using sprintf directly without checking the if the buffer overflows or using nsprintf and adding a bunch of code for checking the return value, add csprintf (checked sprintf) which throws an exception in case the buffer is too small.

This change requires some changes in function signatures to propogate the size of the buffer used (notably in the Serial module).

The echo function is a special case which should never fail (since the device running lizard could become "mute"). Therefore the output will get truncated if the buffer overflows.
Ideally, the echo function should not truncate either. This would require the use of vprintf.
The drawback would be the potential of blocking the main thread in case the output buffer of the uart driver is full (the potential is already there if the uart driver buffer has not been cleared until the next echo()).

Kind regards

[SeaTechRC]

I want to add, for clarity, that the return of sprintf, snprintf and vsnprintf says how much would have been written to the buffer (in case of sprintf the actual amount written).
It is therefore necessary to check the return values of snprintf and vsnprintf in more detail.

The echo() overflow was caused by this behavior, as the pos value added by vsnprintf to pos could exceed the length of the buffer (whilst not actually writing past the buffer). The issue arises when adding the newline char in the following sprintf call, where the offset then could exceed the buffer size.

[SeaTechRC]

There is one more potential issue (which might warrant its own issue/pr).

When the function Serial::read_line is called and there are more than 2 lines in the uart buffer, the pattern positions returned by uart_pattern_get_pos and uart_pattern_pop_pos for the second pattern position can be incorrect, when the buffer is read in 2 different read_line calls.

As far as I could see, the pattern positions never get updated when calling uart_read_bytes.
Therefore, all currently detected patterns must be handled even if only one line is supposed to be read.

[falkoschindler]

Thanks a lot for yet another valuable pull request, @SeaTechRC!

After reviewing your changes, I've just got a few thoughts:

1. I simplified the truncation in echo() a bit, because pos was 0 and could be omitted, so that the new pos is basically the minimum of num_chars and the buffer size. I think using std::min reflects the intention of this code more clearly.

2. Can you elaborate a bit on the new if (pos >= buffer_len) { } block in Serial::read_line? There's quite a lot going on and it's not completely obvious to me how it works.

3. Currently you call buffer-related functions like this:

   write(buffer, sizeof(buffer), ...)
   write(&buffer[pos], sizeof(buffer) - pos, ...)

   I wonder if it would be more intuitive to change it to

   write(buffer, 0, ...)
   write(buffer, pos, ...)

   i.e. passing buffer and start position rather than buffer and size. This would remove boilerplate code from the callees into the function, which can determine the size itself using sizeof(buffer). What do you think?

[SeaTechRC]

1. After another thought about the min value in the echo() function:
   The max value for num_chars is set to sizeof(buffer) - 1 (which is 1023 currently). That would mean that the string termination gets put at 1024 (outside of buffer).
   vsnprintf can write at most n-1 characters into the buffer. n is sizeof(buffer)-1, thus 1023, so max 1022 chars are written with the termination at 1023 (correctly leaving space for the newline).
   If the vsnprintf writes 1022, but returns 1023 or above (since it returns the amount of chars it would have written), the newline gets written at index 1023 and the termination at 1024 (buffer overflow).
   It should be std::min(num_chars, static_cast(sizeof(buffer) - 2)) instead.

2. If the length of the received line in the uart buffer exceeds the buffer size given to Serial::read_line, we cannot read the line.
   There are 2 options to handle this:

   • Discard the line.
   • Wait for another call to Serial::read_line with sufficiently sized buffer.

   I picked the first option since we don't want the serial to clog up, but neither way is ideal. The if(serial.available() < pos) {} block is for flushing the serial buffer and pattern positions if the pattern position is invalid. (This somewhat more related to the my previous comment about the pattern positions not getting updated when reading.)
   Although throwing an exception might be a better idea.

3. I am unsure if the function can actually determine the size of the buffer themselves. As far as I know sizeof is calculated at compile time (edit: not runtime) and using sizeof in the functions would result in getting the size of a char pointer. I agree with reducing boilerplate code, but I don't think its possible.

[falkoschindler]

1. Ok, I changed the upper limit to sizeof(buffer) - 2.
2. I guess this makes sense.
3. Good point! I wasn't aware of this limitation.

@JensOgorek I've reviewed this PR and it looks good to me. Feel free to merge it whenever you're ready.

[JensOgorek]

I was almost done, but waited since you were changing some stuff.
Looks good to me as well.