GNOME Bugzilla – Bug 796174
strcat() considered unsafe for buffer overflow
Last modified: 2018-06-19 08:37:40 UTC
Hi Team, https://github.com/GNOME/evolution/blob/master/src/calendar/gui/e-calendar-view.c#L2346 i.e strcat (buffer, " - "); Does not check for buffer overflows when concatenating to destination which is [MS-banned] (CWE-120). Consider using strcat_s, strncat, strlcat, or snprintf (warning: strncat is easily misused) Request team to please have a look. Thank you Team w00t
Thanks for a bug report. The buffer in that place is stack-allocated and has 1024 bytes. While it's theoretically possible to overflow that buffer it's highly unlikely. Do not understand me wrong, I agree with code safety, even it can be sometimes overpedantic (see the latest commits around static analysers warnings in evolution and evolution-data-server), thus I do not close this bug report. Feel free to provide a patch for this, in which case you receive also credits for the work. There's a glib variant of snprintf() called g_snprintf(), which I'd prefer to use, but I'm afraid it doesn't allow reading and writing from/to the same buffer, thus its use requires other (more or less) ugly constructions in the code.
Thank you Milan, for looking into this we have PR for same. https://github.com/GNOME/evolution/pull/3
Please see https://wiki.gnome.org/Git/Developers for how to contribute patches. Thank you a lot!
(In reply to Team w00t from comment #2) > Thank you Milan, for looking into this we have PR for same. > https://github.com/GNOME/evolution/pull/3 Thanks. I looked there and to be honest I do not agree with that approach: a) doubling the buffer size when it is already quite large will not fix, but only workaround/postpone, the real issue there b) you changed only one single strcat() call, while the file itself has there multiple such calls, even the next line of your change uses it c) I'd expect a change for the whole source tree, not a separate patch for each single call d) the change itself won't build, strlcat() has three arguments, not two e) for some reason, `man strlcat` doesn't produce any information, while `man strcat` does, which is one more reason to use something portable, like above (comment #1) mentioned g_snprintf().
Created commit_34bad6173 in eds master (3.29.3+) [1] Created commit 8f974afad in evo master (3.29.3+) [1] https://gitlab.gnome.org/GNOME/evolution-data-server/commit/34bad6173
I finally committed these also into the stable branches, but I do not think it had been really necessary. Created commit_c52a659c8 in eds gnome-3-28 (3.28.4+) [2] Created commit a87ce36cd in evo gnome-3-28 (3.28.4+) [2] https://gitlab.gnome.org/GNOME/evolution-data-server/commit/c52a659c8