mikalhart / TinyGPSPlus

A new, customizable Arduino NMEA parsing library
http://arduiniana.org
1.08k stars 491 forks source link

Fix -Wstringop-truncation warning. #66

Open denravonska opened 4 years ago

denravonska commented 4 years ago

This fixes a warning I encountered when cross compiling for Raspberry Pi. If term is longer than stagingBuffer it won't be 0-terminated.

Also, fun fact. strncpy always copies the specified bytes. If the source is shorter than that it pads the rest with 0. TIL.

If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.

For the low end chips like AVR it could be worth adding a custom strncpy which guarantees termination without the overhead of filling the entire buffer.

char* tinygps_strncpy(char *dest, const char *src, size_t n) {
   size_t i;
   for (i = 0; i < n && src[i] != '\0'; i++)
      dest[i] = src[i];

   dest[i - 1] = 0;
   return dest;
}
mikalhart commented 10 months ago

Plan to fix in next release; thanks.

denravonska commented 10 months ago

Don't use my particular strncpy though since it goes oob if the first character is 0 :D

robertlipe commented 1 month ago

strncpy, like strcpy, has far too many foot-guns. Most environments provide a strlcpy() that works like you intend this one to, but it also has problems. See https://lwn.net/Articles/507319/ C11 gave us (via Microsoft's similarly named functions from their almost-C99 implementation) so that's available in any C++ that includes C++ via reference (which was about c++14 or so, I think), but it was renamed to strcpy_s(afe) https://en.cppreference.com/w/c/string/byte/strcpy

Honestly, once I find myself this deep into a hole, I find it wise to ask if it's time to stop digging and find a fresh outlook: This is a C++ project. Is it time to quit torturing the developers with C strings and use C++ strings?

stagingBuffer doesn't seem to be used in many places or in particularly complicated ways. Should it just be a std::string where it can be dynamically resized as needed and without all the mind-bending memsets and explicit settings of foo[something] = '\0'?

I think it'll become more readable. Best I can tell, set() becomes this->stagingBuffer = term, commit() becomes ...this->buffer = this->stagingBuffer; and the various memsets becomes a method call to std::string.clear()

Despite the name, I'm pretty sure this code has a strong C heritage. I'm pretty sure that fromDecimal and parseDecimal would be fromChars, parseDegrees() would just return a pair of floats (doubles?), for example. On a Pi, it seems you have a good test bed to run with some develoment on this.

Does at least the change to the type of stagingBuffer play out in this code? Might that be more acceptable to the reviewers? (of which I'm not; I'm just playing armchair code reviewer tonight...)