Do Not Detect Overflow With Overflow

Credits to a gweilo for the sharing below.

Integer overflow and underflow manifest themselves as vulnerabilities. Here is an overflow bug fired by Sir BugFinder. I assigned our fictional developer Sir FastFix ownership of the bug, and he jumped into the code straight.

First, look at this problematic pseudo-code snippet below :

SWORD param = 0;

while ( flag )
{
	param ++ ;
	//
	//manipulate the flag value...
	//
}

buffer = malloc(sizeof(BYTE) * param);
...

The param can increase definitely. No good. Sir FastFix quickly identifies the problem and sends me this code review below.

// sirfastfix: now uses unsigned.
UWORD param = 0;

while ( flag )
{
	// sirfastfix: code fix for overflow bug.
	if ( param > param + 1 )
	{
		TRACE_ERROR("Overflow occurred at param\n");
		return E_UNEXPECTED;
	}
	param ++ ;
	//
	//manipulate the flag value...
	//
}

buffer = malloc(sizeof(BYTE) * param);
...

Now, I have to review it. Let’s look at the changes.

  1. param is now checked with the condition (param > param + 1). Since it must be false, an overflow must have occurred if it is true. Intuitive.
  2. param is now unsigned using UWORD, and not signed SWORD. I find no reasons for negative buffers. A good move.

But, something smells stinky. Let’s think again.

  1. Why not use well-defined constants like MAX_INT, MAX_SHORT or MAX_LONG constants to check before incrementing param? Like MAX_INT – a < b ?
  2. Why the code to detect overflow is using yet another overflow to check?

Sir FastFix, I am not approving this code check-in. This fix is not going in anywhere into the source tree. Who knows what this overflow to check overflow can result in? Let’s write more solid and not college quality code, and not rushing to resolve the bug.

About these ads

7 Responses to “Do Not Detect Overflow With Overflow”


  1. 1 Tom Chan October 14, 2008 at 10:07 am

    This reminds me of an issue about signed integers:
    int x;
    // …
    if ( x<0 ) x = -x;
    // assume x positive here (WRONG!)

    Reason:
    When x is -(2^31), -x is still -(2^31)… boom!

  2. 2 log0 October 24, 2008 at 11:19 am

    >>Tom

    Sorry Tom, the email for comments didn’t work for some reason. and I never got to see the reply. =( And I’ve been so busy for nothing, too.

    Yep, since positive integers have one less representation due to a “zero”.

    This also happens for division and multiplication overflow as well.

  3. 3 pakming October 25, 2008 at 4:33 am

    I can only say that it looks good in the original fix.
    (param > param + 1)
    will be false only when overflow occurs.

    Although you may say that you can use something like:
    (param > MAX_UWORD – 1).
    Yes, it also suits the same purpose. This is kind of matter of taste – i believe. But this may not be so straight forward than the first one (you have to change operator+ into operator-). Also, enlarging the type from UWORD to UDWORD later would need to change from MAX_UWORD to MAX_UDWORD (of couse, you can argue that we need to carefully review the code for such a big change!!)

    Changing from signed to unsigned is risky and should be code reviewed carefully. There are usually some non-trivial behavior change around the comparison, casting, primitive arithematic operations, etc. Careful code review is needed for this “big” change!!!

    In this fix, i would like to spend more time in reviewing the “signed->unsigned” rather than spending time on arguing which comparison looks more professional!!

    Just my opinion. (I admit I used a lot comparisons of the first form. I may not be so professional from your point of view :-) )

  4. 4 log0 October 25, 2008 at 4:49 am

    The professional term here is a by-product only. You should understand it is not important as it is an empty term.

    I don’t understand why would such code be there, but let me know if you can tell me why negative buffers are good and overflowing is good, too. Since you never know what happens for those undefined behaviour in runtime and in compiler specification, why risk?

    So, unless you can justify why must you detect overflow with overflow and allocating negative buffers. If you are submitting such code, it is going to get rejected 100%.

  5. 5 log0 October 25, 2008 at 4:58 am

    I agree with you careful code review is required due to change in signedness, and this is really code-dependent. If it is a global variable of some sort, then it is a great danger to change.

    But I would suggest change in general. Leave it alone, or better than it was.

  6. 6 pakming October 25, 2008 at 7:08 am

    No, allocating a buffer of negative size is obviously a bad bug… no question here. I meant we need to review the code carefully no matter whether it is local or global. There are too much tricky stuff in changing the sign, although most of us may think that it is a piece of cake. But I never meant we don’t need to change it.

    I agree that different compiler may have different behavior – i only have only worked on one compiler for this kind of issues so far. (But if the compiler changes its existing behavior, file a bug!!)

    After discuss with Andrew, I found that C# may simply throw an exception at the comparison. Therefore, I agree that using (param > MAX_UWORD – 1) may be, in general, a better solution.

  7. 7 log0 October 25, 2008 at 9:08 am

    Ok sorry I mistakened that one. We have agreement on the negative buffer.

    Yep. I agree to you the changing of sign is tricky, especially if the variable is not trivial. You’re right, with care you change.

    I see. I do not know about C#. If it throws an exception, it seems good to let it throw and fail gracefully at least, moreover it is best if compiler or libraries provide the solution.


Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s





Follow

Get every new post delivered to your Inbox.

%d bloggers like this: