After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796174 - strcat() considered unsafe for buffer overflow
strcat() considered unsafe for buffer overflow
Product: evolution
Classification: Applications
Component: Calendar
Other Linux
: Normal normal
: ---
Assigned To: evolution-calendar-maintainers
Evolution QA team
Depends on:
Reported: 2018-05-16 16:05 UTC by Team w00t
Modified: 2018-06-19 08:37 UTC
See Also:
GNOME target: ---
GNOME version: ---

Description Team w00t 2018-05-16 16:05:32 UTC
Hi Team,


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
Comment 1 Milan Crha 2018-05-18 07:03:37 UTC
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.
Comment 2 Team w00t 2018-05-19 08:56:48 UTC
Thank you Milan, for looking into this we have PR for same.
Comment 3 André Klapper 2018-05-20 15:10:05 UTC
Please see for how to contribute patches. Thank you a lot!
Comment 4 Milan Crha 2018-05-21 10:53:18 UTC
(In reply to Team w00t from comment #2)
> Thank you Milan, for looking into this we have PR for same.

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().
Comment 5 Milan Crha 2018-06-14 12:27:31 UTC
Created commit_34bad6173 in eds master (3.29.3+) [1]
Created commit 8f974afad in evo master (3.29.3+)

Comment 6 Milan Crha 2018-06-19 08:37:40 UTC
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+)