New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mprintf: Avoid integer overflow warning from clang #3184

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@rockdaboot
Contributor

rockdaboot commented Oct 28, 2018

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.

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 28, 2018

Sorry for closing and re-opening. I just registered for Hacktoberfest ;-)

@bagder

This comment has been minimized.

Member

bagder commented Oct 28, 2018

unsigned integer overflow?

I don't understand this complaint. Doesn't it just "wrap around" from 0 to SIZE_T_MAX?

@bagder

This comment has been minimized.

Member

bagder commented Oct 28, 2018

@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.

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 29, 2018

Doesn't it just "wrap around" from 0 to SIZE_T_MAX?

Yes, the code does this obviously. The good thing is, it does it after checking, so there is no real bug as long as len isn't used thereafter. So 'fix' is not a correct wording, 'silence' or 'suppress' would be better.

I did this with clang 6.0, CFLAGS="-O1 -g -fno-omit-frame-pointer -gline-tables-only -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=undefined,integer,nullability -fsanitize=address -fsanitize-address-use-after-scope -fsanitize-coverage=trace-pc-guard,trace-cmp"

@bagder

This comment has been minimized.

Member

bagder commented Oct 29, 2018

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 usesanitize=signed-integer-overflow instead of sanitize=integer to avoid this false positive.

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 29, 2018

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).

@bagder

This comment has been minimized.

Member

bagder commented Oct 29, 2018

This is not a false positive

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.

general suppression of such UB is not a good advice

It is not UB. It is on the contrary a defined B.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Oct 29, 2018

At least a general suppression of such UB is not a good advice.

Unsigned, in contrary to signed, overflow is not UB - N1570 6.2.5 [Types] / 9 says:

A computation involving unsigned operands can never overflow,
because a result that cannot be represented by the resulting unsigned integer type is
reduced modulo the number that is one greater than the largest value that can be
represented by the resulting type.

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 29, 2018

It is not UB. It is on the contrary a defined B.

True, sorry. It is still a possible dangerous behavior that is worth reporting.

It seems to be a warning for people and code bases where they avoid or ban wrap-arounds for some reason.

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.

@jay

This comment has been minimized.

Member

jay commented Oct 29, 2018

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 rockdaboot force-pushed the rockdaboot:tmp-mprintf-integer-overflow branch from 56b6add to 87d1fd9 Oct 30, 2018

@rockdaboot rockdaboot changed the title from mprintf: Fix integer overflow to mprintf: Avoid integer overflow warning from clang Oct 30, 2018

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 30, 2018

Amended the issue title and the commit message to make clear that there is no real impact.

@bagder

This comment has been minimized.

Member

bagder commented Oct 31, 2018

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.

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Oct 31, 2018

Another option is to decrement len in the if body. That doesn't look weird, even makes the code easier to read. Though it introduces two extra lines.

@infinnovation-dev

This comment has been minimized.

infinnovation-dev commented Nov 1, 2018

Would the following be sufficiently succinct and non-weird?

        for(; len > 0 && *str; len--)
          OUTCHAR(*str++);
@bagder

This comment has been minimized.

Member

bagder commented Nov 1, 2018

I like that. Since 'len' is unsigned it could even be simplified one step further:

for(; len && *str; len--)
  OUTCHAR(*str++);
mprintf: Avoid integer overflow warning from clang
The overflow has no real world impact.
Just avoid it for "best practice".

Code change suggested by "The Infinnovation Team" and Daniel Stenberg.

@rockdaboot rockdaboot force-pushed the rockdaboot:tmp-mprintf-integer-overflow branch from 87d1fd9 to 3bee58c Nov 2, 2018

@rockdaboot

This comment has been minimized.

Contributor

rockdaboot commented Nov 2, 2018

Much better :-) PR amended.

@MarcelRaad

Looks good to me now!

@bagder

This comment has been minimized.

Member

bagder commented Nov 2, 2018

Thanks!

@bagder bagder closed this in e4f2a5b Nov 2, 2018

@rockdaboot rockdaboot deleted the rockdaboot:tmp-mprintf-integer-overflow branch Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment