Page MenuHomePhabricator

c: possiblility of buffer overflow with strncat.
Changes PlannedPublic

Authored by brunobelo on Oct 7 2019, 2:12 PM.

Details

Summary

scan-build and https://en.cppreference.com/w/c/string/byte/strncat
recommends use sizeof(buf) - strlen(buf) - 1 to avoid overflow.

Diff Detail

Repository
rEFL core/efl
Branch
coverity
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13866
Build 9622: arc lint + arc unit
brunobelo created this revision.Oct 7 2019, 2:12 PM
brunobelo requested review of this revision.Oct 7 2019, 2:12 PM
brunobelo edited the summary of this revision. (Show Details)Oct 7 2019, 2:58 PM
segfaultxavi added inline comments.
src/bin/edje/edje_player.c
845

This seems a bit complicated given that buf is always an empty string at this point, no?

src/lib/elementary/elc_naviframe.c
517

Same comment as above.

521

?

ProhtMeyhet added a subscriber: ProhtMeyhet.EditedOct 8 2019, 12:51 AM

One of the main reasons I hate with C is that strclat, or better, strl*, are not portable. Maybe it works, maybe it doesn't. Isn't there some Eina* function to work around this?

segfaultxavi requested changes to this revision.Oct 8 2019, 1:01 AM

Well, it does not exist in my Ubuntu 18.04 with gcc 7.4:

elc_naviframe.c:(.text+0x304d): undefined reference to `strclat'
This revision now requires changes to proceed.Oct 8 2019, 1:01 AM
vtorri requested changes to this revision.Oct 8 2019, 2:32 AM
vtorri added a subscriber: vtorri.

strlcat does not exist on Windows

Well, it does not exist in my Ubuntu 18.04 with gcc 7.4:

elc_naviframe.c:(.text+0x304d): undefined reference to `strclat'

to be very exact -> your standard libc - glibc - does not support it.

which is exactly my point.

zmike added a subscriber: zmike.Oct 8 2019, 6:59 AM

eina_strlcat and eina_strlcpy both exist.

brunobelo added inline comments.Oct 8 2019, 8:20 AM
src/bin/edje/edje_player.c
845

one of comments at https://stackoverflow.com/questions/6491038/strcat-vs-strncat-when-should-which-function-be-used said

If you are absolutely sure about source buffer's size and that the source buffer contains a NULL-character terminating the string, then you can safely use strcat when the destination buffer is large enough.

buf is a empty string, so it's not "dangerous" to be like it is, but if someday, that buffer change to something else, using - strlen() prevents the overflow.

src/lib/elementary/elc_naviframe.c
521

this was a typo, and it wasn't to be pushed. I was testing with others str* functions.

brunobelo planned changes to this revision.Oct 8 2019, 8:22 AM

change native strcat to its eina functions commented by @zmike

raster requested changes to this revision.Oct 9 2019, 10:54 AM
raster added a subscriber: raster.

none of this fixes any bugs - it just makes the code longer and wordier. warnings are there to encourage us to look at code that might be a problem... the code obviously was not in need of adding - strlen(buf).

brunobelo retitled this revision from c: possible buffer overflow with strncat. to c: possiblility of buffer overflow with strncat..Oct 9 2019, 11:08 AM
lauromoura added inline comments.Oct 15 2019, 9:56 AM
src/lib/elementary/elc_naviframe.c
517

As bufis initially empty, couldn't it be strncpy(buf, nit->title_label, sizeof(buf) - 1);?

so, what is the status of this diff ?

the status is the same. this doesn't fix any bug. it makes the code longer and more verbose. sure. a better fix would be to make these strncpy's as that at least involves a tiny bit less work for the cpy func as it doesn't have to first walk to end of string (which it will try and immediately stop as it's already at the end). i am not in favor of putting in code that just is more verbose that doesn't fix anything. as the code to modify the string buffer is pretty much right after the buffer is either zeroed out or initialized to be empty (at least start with a nul byte) i don't see an argument for "one day if something changes" as anything that changes this will see the strncat code and should realize it needs changes. i don't see there being any real chance of this though. i'd rather these just become strncpy's to be clear their intent is not to append but to initialize the buffer, which is what they do. it would also silence the warnings.

please - anyone doing this work. don't just make code more verbose. think about it carefully. if you are making changes that don't actually fix any real bug, then think about how to make the code better without making it more verbose and a strncpy is a far simpler change that actually indicates the real functionality of that piece of code. i'm just going to fix the code directly in git so this argument can end. i am sure there are possible bugs floating about. effort to try and find them is good. warnings are a way of pointing to possibly bad code. focus on where the bugs actually are. :) you have no idea how many times i've seen stupid patches like:

strcpy(buf, src); -> strncpy(buf, src, strlen(src));

or similar. code that has done nothing to fix anything but shut up a warning. it made the code less readable and it now HIDES a potential bug. the above doesn't actually do precisely this, but ... it does make it less readable and doesn't fix anything. it isn't hiding a bug though.

to mimic Shakespeare:

to close or not to close ?