[poppler] 13 commits - fofi/FoFiType1C.cc glib/poppler-action.cc glib/poppler-document.cc glib/poppler-page.cc makefile.vc poppler/ArthurOutputDev.cc poppler/CairoOutputDev.cc poppler/CharCodeToUnicode.cc poppler/Error.cc poppler/Gfx.cc poppler/G

[Krzysztof Kowalczyk]

Reverted. Sorry about that and thanks for noticing and explanation.

I have to say that I still don't get that code. Before making this
change I wrote a test program (see below) that ran the expression in
question (a * (int) b / b != a) on every integer and would say if for
any int a the result is any different than just testing for a < 0.

On my Ubuntu 7.04 with gcc 4.1.2, those are the exact equivalents.

I get your explanation but it seems that in those 3 specific cases
I've replaced, where a is an int and b is small unsigned value, those
tests are really equivalent to testing for a < 0, because an int
overflow of multiplying a postive value shows as negative number. Even
if you use a large b (like 16000), I couldn't find a single int a for
which the tests are not equivalent.

That being said, better be safe than sorry, so change reverted.

-- kjk

#include <stdio.h>
#include <limits.h>
#include <stdint.h>

#define MIN_INT     0x80000000
#define MAX_INT     (MIN_INT - 1)

bool t(int a)
{
    size_t b = sizeof(int);
    if (a * (int)b / b != a)
    {
        if (! (a < 0) )
            printf("t(%d) failed; a * (int)b / b = %d\n", a, (int)(a *
(int)b / b));
    }
}

int main(int argc, char **argv)
{
    for (int a=MIN_INT; a++; a < MAX_INT)
    {
        t(a);
    }
}

On 9/25/07, Kristian Høgsberg <krh at bitplanet.net> wrote:
> On 9/24/07, Krzysztof Kowalczyk <kjk at kemper.freedesktop.org> wrote:
> 5a1f670a4d16affeed86cdf643ab22f481caa3a5)
> > Author: Krzysztof Kowalczyk <kkowalczyk at tlapx60ubu.(none)>
> > Date:   Sun Sep 23 22:28:16 2007 -0700
> >
> >     replace extremely confusing 'a*(int)sizeof(foo)/sizeof(foo) != a' which, due to type promotions, if a is int, is equivalent to a < 0; fix problems revealed by the change
>
> NO! Revert this commit right away.  Those checks are integer overflow
> checks and prevent a class of security bugs.  The problem is that when
> a pdf document specifies two (or more) numbers that poppler has to
> multiply to determine how much memory to use, integer overflows can
> happen.  For example, suppose the document specifies the width and
> height of an image.  Both width and height can be positive, innocent
> looking numbers, but when multiplied together, the results overflow
> 32bits.  The result may be a small positive integer, say 17 bytes,
> which poppler then successfully allocates and then starts reading the
> (huge) image data into that 17 byte allocation.  What you have is an
> exploitable security bug, and if you check the security history of
> xpdf and poppler, you'll see that we've fixed a great many of those
> over time.
>
> A nice patch would be to introduce a macro such as CHECK_OVERFLOW(a,b)
> that implements the above check, which makes the code more readable
> and isolates the implementation of this tricky check in place.  But
> the checks have to stay.
>
> cheers,
> Kristian
>