Stein Vidar Hagfors Haugan via developers <developers@lists.mariadb.org> writes:
So I am considering to contribute with a patch that will eliminate all sprintf() instances by converting them to snprintf(destination, N, ...). Of course, hunting down the correct N for each instance can be a *lot* of work (I presume this is why this hasn't been done already).
Yes. It's very important not to do changes like this (or any changes to the code base) without fully understanding the code in each instance.
So I'm proposing to make a macro UNSAFE_SNPRINTF(destination, ...) which would expand to snprintf(destination, 10000, ...) and replace all current sprintf() calls with that.
That sounds like a bad idea. The snprintf() silently truncates the destination string. This could lead to serious corruption bugs. And due to the silent truncation, the bug will be hidden from tools like valgrind and asan, which would otherwise (using sprintf) be able to detect the bug when the destination buffer is too small. Remember, buffer overflows are not the only bugs in the world! Nor often the most serious.
There are numerous uses of sprintf() in the general mariadb codebase, and each one generates a warning during compilation when doing a plain cmake/make build (on MacOS & RHEL7 at least).
I am a bit fanatical about hunting down and eliminating warnings if at all possible, no matter how small and insignificant - if you become accustomed to routinely getting warnings during compilation, you can easily overlook an important one.
A patch to disable the warning about use of sprintf() would be a good alternative, and would solve your problem. It's good to disable unwanted compiler warnings. - Kristian.