mprintf: Avoid integer overflow warning from clang #3184
Conversation
|
Sorry for closing and re-opening. I just registered for Hacktoberfest ;-) |
|
unsigned integer overflow? I don't understand this complaint. Doesn't it just "wrap around" from 0 to SIZE_T_MAX? |
|
@rockdaboot can you clarify how you get this warning and what version of clang? I've run curl and its tests quite a few times with both gcc and clang UB and address sanitizers and I don't get this warning. It is even a very well exercised line of code. |
Yes, the code does this obviously. The good thing is, it does it after checking, so there is no real bug as long as I did this with clang 6.0, |
|
Thanks. I don't think we benefit from warnings on wrap-arounds on unsigned integers. As seen in this and in #3185 we use them - without them being an error nor a mistake. I would rather suggest that you better change your clang options to use |
|
This is not a false positive. Clang detects this real happening during runtime. But it (currently) has no effect. This can change when code changes or if compilers optimize differently. So the best measurement is to avoid this behavior in code. At least a general suppression of such UB is not a good advice. That possibly suppresses reports of real bad effects in other places as well. One option is to suppress the warning at that certain place (but still less future proof than changing the code). |
True, that's not the correct word for this. It warns for an "overflow" that is truly happening, but one that is controlled, expected and isn't an undefined behavior. What benefit does this warning bring for us? It seems to be a warning for people and code bases where they avoid or ban wrap-arounds for some reason. Just because clang provides a warning (for a well-defined C behavior) doesn't mean we must approve of its use.
It is not UB. It is on the contrary a defined B. |
Unsigned, in contrary to signed, overflow is not UB - N1570 6.2.5 [Types] / 9 says:
|
True, sorry. It is still a possible dangerous behavior that is worth reporting.
Unexpected wrap-around is possibly dangerous, especially when such a variable is used for bounds-checking. Expected warp-around like in typical hash functions is of course not worth mentioning. That's why clang offers a function attribute to suppress the warning. I am not sure if you can say that the whole curl codebase is safe on unsigned wrap-arounds. For this you first have to detect and manually check all the possible places. But even after being aware of all such code, you might be interested if commits or MRs introduces new wrap-around with bad effects. So there are good reasons to fix or silence the harmless/expected warnings and then switch on the extra checks during CI runs. Especially when the fixes are trivial. |
|
Not a fan of #3185 but this one I think is fine, though I agree with the general sentiment. You could call this "more correct" since it would only prevent a bug if the code were modified to evaluate len later on. It's good for best practice (and getting a free t-shirt apparently). |
rockdaboot
force-pushed
the
tmp-mprintf-integer-overflow
branch
from
56b6add
to
87d1fd9
Compare
rockdaboot
changed the title
|
Amended the issue title and the commit message to make clear that there is no real impact. |
|
The impact is that it converts the code into a weird-looking line that now will trigger people's minds as it doesn't make much sense to evaluate the same variable twice like that. I would argue that this would require a comment blob explaining why it looks odd. And when we need a comment to explain why we need odd-looking code just to silence a warning that doesn't buy us anything, I think we should reconsider. I'm a 👎 on merging this. I don't think warning for unsigned integer "overflows" is useful for us. |
|
Another option is to decrement |
|
Would the following be sufficiently succinct and non-weird? for(; len > 0 && *str; len--)
OUTCHAR(*str++); |
|
I like that. Since 'len' is unsigned it could even be simplified one step further: |
The overflow has no real world impact. Just avoid it for "best practice". Code change suggested by "The Infinnovation Team" and Daniel Stenberg.
rockdaboot
force-pushed
the
tmp-mprintf-integer-overflow
branch
from
87d1fd9
to
3bee58c
Compare
|
Much better :-) PR amended. |
|
Thanks! |
A simple curl_easy_init() triggers an unsigned integer overflow in dprintf_formatf():
mprintf.c:838:19: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x6d9dcf in dprintf_formatf /home/tim/src/curl/lib/mprintf.c:838:19
#1 0x6d71d7 in curl_mvsnprintf /home/tim/src/curl/lib/mprintf.c:1006:13
#2 0x6dcf29 in curl_msnprintf /home/tim/src/curl/lib/mprintf.c:1023:13
#3 0x81d17b in Curl_ossl_version /home/tim/src/curl/lib/vtls/openssl.c:3739:10
#4 0x6d4cd8 in curl_version /home/tim/src/curl/lib/version.c:119:11
#5 0x6d4bf4 in Curl_version_init /home/tim/src/curl/lib/version.c:86:3
#6 0x51f245 in global_init /home/tim/src/curl/lib/easy.c:271:3
#7 0x51f52b in curl_easy_init /home/tim/src/curl/lib/easy.c:361:14
While this likely has no real world impact in normal operation, it is annoying when doing UBSAN / fuzzing tests. Good programming practice would be to avoid it.
There are certain possible fixes to it. I chose the most obviously and any decent compiler optimizes this into a single CPU opcode, so speed impact should be near zero.