A New Way to Detect Integer Overflows?


David LeBlanc and I have written a good deal about Integer Overflow issues, including the following:

A couple of days ago I saw some code from someone outside of Microsoft claiming they had found a new (read: cheap) way to detect integer overflow errors, here’s the code snippet:

void *p= NULL;
size_t cb  = z + (x * y);
if ((int)cb > 0 && cb < MAX)
   p=malloc(cb);

Basically, you cast the result to signed, and if it’s negative, then there must be an overflow… right?

I had no spare cycles, so I asked David to look at it. He shot the code down in about 15secs. So what’s wrong with the code?

Comments (10)

  1. Dean Harding says:

    Well, it’s possible that the value could actually wrap around "twice" (i.e. back into positive), as in:

    unsigned int x = MAX – 1;

    unsigned int y = MAX – 1;

    unsigned int z = MAX – 1;

    size_t cb = z + (x * y);

    Will yield cb = 2…

  2. Ilya says:

    if ((cb > z) && (cb < MAX))

  3. oshkosh says:

    In a recent security conference (http://esorics04.eurecom.fr/program.html), there was a paper which addressed integer overflow attacks in their own way. Here is the URL (post-googling) of the paper:

    http://www.cse.buffalo.edu/~rc27/publications/chinchani-ESORICS04-final.pdf

    There are some nice ideas, which you might find useful for your work.

  4. Michael Howard says:

    Dean hit it on the head! x*y could result in a value >2^31, which would be treated as negative, then adding z would swing the result back into the positive column. oops!

  5. Thomas says:

    The code as we see it here is ok.

    size_t is unsigned int, malloc() takes an unsigned int argument.

    (multiple wrap arounds doesnt matter, BTW)

    but the interessting code part is missing: the copying to the buffer.

    my suggested test code:

    size_t tmp;

    if( x >= UINT_MAX / y)

    exit()

    if( (x * y) >= (UINT_MAX – z) )

    exit()

  6. Michael Howard says:

    Personally, i would not use a divide as it’s very expensive; if i can avoid using it i will. You should look at how LeBlanc does this without doing divides in SafeInt.hpp

  7. Thomas says:

    But LeBlanc also wrotes:

    "For a 64-bit integer, you have no other choice than to perform the division, …"

    And 64 bit systems become more and more common. Espacially in the Unix sector.

    size_t will become 64 bit there, and in the open source community you have to make the checks bulletproof on every architecture because the code is often widely used… from x86_32, over 31 bit s390, to ppc64.

    But thanks for the hint and for the links to the papers. :)

    Thomas

  8. Michael Howard says:

    I totally agree with your comments about 64-bit. It also turns out a div is sometimes not so expensive, in the case of, say n/2, most compilers will turn this into n >>= 2.

  9. 厚重之刀 says:

    I hope to see more new methods!

  10. Thomas… Should you make sure that y isn’t zero? (divide by zero error)