Eliminating sprintf deprecation warnings (maybe others as well)
Hi! First time here, so my apologies if I'm violating some spoken or unspoken rules :) I'm developing/maintaining two specialized storage engines(*) and I have one issue: 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. 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). 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. Yes, 10000 is a ridiculously high number that makes it look like we're introducing a huge security risk, but but the current sprintf() calls have an implicit N = infinity. In some instances, the compiler sniffs out the actual size of the buffer and complains - in that case I would change the code to an appropriate regular snprintf() call. Would doing this be worthwile? I.e., is there a good chance that such a patch would actually be accepted into the codebase? If so, I might also do the same with other warnings (I believe there are some type-punning warnings at least). Sincerely, Stein Haugan *) The engines are for internal use so far, but might make them public at some point
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.
Ok, thanks for the advice, Kristian. I guess I'll live with it (as I haven't found any way to turn off that particular warning, even though I'm not the only one wanting to :).
On 27 Oct 2023, at 19:21, Kristian Nielsen <knielsen@knielsen-hq.org> wrote:
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.
participants (2)
-
Kristian Nielsen
-
Stein Vidar Hagfors Haugan