Do Not Detect Overflow With Overflow

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.

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.

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.

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 ?

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.

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

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

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.