Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix -Wstringop-truncation warning. #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

denravonska
Copy link
Contributor

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
Copy link
Owner

Plan to fix in next release; thanks.

@denravonska
Copy link
Contributor Author

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

@robertlipe
Copy link

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants