Potential invalid access in strncpy0

[nolange]

Hello,

at (around) line 176 you call:

                strncpy0(section, start + 1, sizeof(section));

which will do the same as:

memcpy(section, start + 1, sizeof(section) - 1);

but you don't know that start has sizeof(section) - 1 bytes accessible (you are just lucky that this is likely). You would need to take the minimum of characters available in start as an upper bound.

You have the end calculated a line above, so why not do something like:

                // *end = '\0'; Not needed?
                helper = (size_t)(end - (start + 1)) < sizeof(section) ? (size_t)(end - (start + 1)) : sizeof(section)  ;
                memcpy(section, start + 1, helper);
               section[helper] = '\0';

[benhoyt]

Thanks for the report -- you're dead right! I actually had this strncpy0 using strncpy earlier, but changed it to memcpy due to a gcc warning. Heh, my comment here proved all too true:

I'm a little bit wary about changing it as the current version has been "battle tested" in a bunch of different compilers by now.

Sure enough, changing it introduced an invalid access! I switched to using a manual loop: which will fix this issue, presumably the gcc warnings with strncpy, and possibly result in smaller code (this library's optimized for embedded systems).

Let me know if you see any issues with this, otherwise I'll tag a new release soon.

[benhoyt]

Oh, for what it's worth, I was able to repro this fairly easily with a line with lots of spaces and then a section tag, like [x] at the very end. Then the memcpy read ~50 bytes (MAX_SECTION) off the end of the line. And LLVM's -fsanitize=address option spewed out errors.

This change fixes it because the line will always be NUL-terminated (the new code looks for the NUL terminator, just like strncpy did).

[nolange]

So in the end you implemented strncpy, just to squelch a warning (IMHO a false positive).

Please look at the new PR, if contains my fix (which already was in the old one), there no new search for \0 is necessary, means the embedded systems will have some more CPU cycles to use for other stuff ;)
Could potentially do that on more places