Fix ignored snprintf return value in StatusArg.cpp#8967
Fix ignored snprintf return value in StatusArg.cpp#8967annafilimonova1 wants to merge 4 commits intoFirebirdSQL:masterfrom
Conversation
| } | ||
|
|
||
| Quad::Quad(const ISC_QUAD* quad) noexcept : | ||
| Str(text) | ||
| { | ||
| snprintf(text, sizeof(text), "%x:%x", quad->gds_quad_high, quad->gds_quad_low); | ||
| [[maybe_unused]] auto result = snprintf(text, sizeof(text), "%x:%x", | ||
| static_cast<unsigned int>(quad->gds_quad_high), |
There was a problem hiding this comment.
Format is hexadecimal. What's wrong with signed integer?
There was a problem hiding this comment.
See page 332 of https://open-std.org/JTC1/SC22/WG14/www/docs/n3220.pdf
b,B,o,u,x,X The unsigned int argument is converted to unsigned binary (b or B), unsigned octal
(o), unsigned decimal (u), or unsigned hexadecimal notation (x or X)
Looks like it does not care about sign. Not sure what changes when you pass signed integer instead, probably nothing? Anyway, static_cast makes it more explicit
There was a problem hiding this comment.
Not sure what changes when you pass signed integer instead, probably nothing?
Exactly as written in your quote: for x specifier corresponding parameter is considered to be unsigned int. No explicit cast is needed as long as parameter's size matches. That's the way variadic functions work: they use reinterpret_cast inside.
There was a problem hiding this comment.
Static analyzer was triggered just because of different types. I agree, in this case there are not any problems just because of differing types, but there is a reason to consider this cast as a good practice:
va_arg call with unsigned int argument while original type is int will be valid only if original int is non-negative.
If int is negative for some reason, we will face UB without this fix. With fix, value will just wrap around without UB.
page 289 for va_arg:
If type is not compatible with the type of the actual next argument (as promoted according to
the default argument promotions), the behavior is undefined, except for the following cases:
...
- one type is compatible with a signed integer type, the other type is compatible with the
corresponding unsigned integer type, and the value is representable in both types
...
| @@ -407,19 +407,25 @@ Num::Num(ISC_STATUS s) noexcept : | |||
| Int64::Int64(SINT64 val) noexcept : | |||
| Str(text) | |||
| { | |||
| snprintf(text, sizeof(text), "%" SQUADFORMAT, val); | |||
| [[maybe_unused]] auto result = snprintf(text, sizeof(text), "%" SQUADFORMAT, val); | |||
| fb_assert(result >= 0); | |||
| } | |||
There was a problem hiding this comment.
Adding asserts is good idea - but they should be complete. Look here:
From man:
The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.
I.e. sizeof(text) should also be taken into an account.
There was a problem hiding this comment.
Done. Please, see changes
4 participants
Part of closed #8938