code review 7486051: image/gif: handle invalid GIFs more cautiously

image/gif: handle invalid GIFs more cautiously

GIFs with huge bounds and not enough pixels to
match were causing huge allocations before giving
an error. We now load the pixels and compare
how many are really available before allocating
the image.

Fixes issue 5050.

[jeff.allen, 2013-03-14 15:26:47]

Hello nigeltao@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
http://code.google.com/p/go

[r, 2013-03-14 15:48:41]

There may be something to do here, but I don't believe you've done it.

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go
File src/pkg/image/decode_test.go (right):

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go...
src/pkg/image/decode_test.go:35: {"", "testdata/issue5050.gif", 64 << 8},
the "" thing is ugly. plus you are looking for a specific error. please just
make this one file a special Test that checks exactly what you're looking for.

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go#...
src/pkg/image/gif/reader.go:183: if pix, err = ioutil.ReadAll(lzwr); err != nil
{
you're still reading in the whole file. i guess that's unavoidable, but it means
the CL description misled me a bit.  change it to something like "after reading
the file, if the size does not match the bounds, don't allocate an Image".

in any case, i don't really see the difference here. instead of reading into
m.pix, you're reading into a local variable. what have you saved? in fact, isn't
it worse? in the original, you read no more than the bounds; here you decode the
whole file.

[bradfitz, 2013-03-14 16:36:59]

The problem was that you could make a GIF file that said:

[ header ] [ image is 99999999 x 99999999 pixels! ] [ lie. only 400 pixels
] [ EOF ]

And we'd read it in that order, and allocate 99999999 * 99999999 pixels
ahead of time, only to fail later when we found it was short.

With his change, he reads the dimensions, notes it, and then slurps all the
pixels and verifies they match, having only allocated 400 pixels worth of
memory.


On Thu, Mar 14, 2013 at 8:48 AM, <r@golang.org> wrote:

> There may be something to do here, but I don't believe you've done it.
>
>
> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>
image/decode_test.go<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go>
> File src/pkg/image/decode_test.go (right):
>
> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>
image/decode_test.go#newcode35<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go#newcode35>
> src/pkg/image/decode_test.go:**35: {"", "testdata/issue5050.gif", 64 <<
> 8},
> the "" thing is ugly. plus you are looking for a specific error. please
> just make this one file a special Test that checks exactly what you're
> looking for.
>
> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>
image/gif/reader.go<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go>
> File src/pkg/image/gif/reader.go (right):
>
> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>
image/gif/reader.go#newcode183<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go#newcode183>
> src/pkg/image/gif/reader.go:**183: if pix, err = ioutil.ReadAll(lzwr); err
> != nil {
> you're still reading in the whole file. i guess that's unavoidable, but
> it means the CL description misled me a bit.  change it to something
> like "after reading the file, if the size does not match the bounds,
> don't allocate an Image".
>
> in any case, i don't really see the difference here. instead of reading
> into m.pix, you're reading into a local variable. what have you saved?
> in fact, isn't it worse? in the original, you read no more than the
> bounds; here you decode the whole file.
>
>
https://codereview.appspot.**com/7486051/<https://codereview.appspot.com/7486...
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[mtj1, 2013-03-14 17:06:40]

precisely... the "GIF of death"

much larger than:
https://commons.wikimedia.org/wiki/Category:Very_large_GIF_files

You might want to think of it like file reading, where you have "read the
whole thing" ("by file") or a more timid approach, not a buffer at a time
like the io library, but with a pre-defined limit.


On Thu, Mar 14, 2013 at 9:36 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> The problem was that you could make a GIF file that said:
>
> [ header ] [ image is 99999999 x 99999999 pixels! ] [ lie. only 400 pixels
> ] [ EOF ]
>
> And we'd read it in that order, and allocate 99999999 * 99999999 pixels
> ahead of time, only to fail later when we found it was short.
>
> With his change, he reads the dimensions, notes it, and then slurps all
> the pixels and verifies they match, having only allocated 400 pixels worth
> of memory.
>
>
> On Thu, Mar 14, 2013 at 8:48 AM, <r@golang.org> wrote:
>
>> There may be something to do here, but I don't believe you've done it.
>>
>>
>> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>>
image/decode_test.go<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go>
>> File src/pkg/image/decode_test.go (right):
>>
>> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>>
image/decode_test.go#newcode35<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/decode_test.go#newcode35>
>> src/pkg/image/decode_test.go:**35: {"", "testdata/issue5050.gif", 64 <<
>> 8},
>> the "" thing is ugly. plus you are looking for a specific error. please
>> just make this one file a special Test that checks exactly what you're
>> looking for.
>>
>> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>>
image/gif/reader.go<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go>
>> File src/pkg/image/gif/reader.go (right):
>>
>> https://codereview.appspot.**com/7486051/diff/2001/src/pkg/**
>>
image/gif/reader.go#newcode183<https://codereview.appspot.com/7486051/diff/2001/src/pkg/image/gif/reader.go#newcode183>
>> src/pkg/image/gif/reader.go:**183: if pix, err = ioutil.ReadAll(lzwr);
>> err
>> != nil {
>> you're still reading in the whole file. i guess that's unavoidable, but
>> it means the CL description misled me a bit.  change it to something
>> like "after reading the file, if the size does not match the bounds,
>> don't allocate an Image".
>>
>> in any case, i don't really see the difference here. instead of reading
>> into m.pix, you're reading into a local variable. what have you saved?
>> in fact, isn't it worse? in the original, you read no more than the
>> bounds; here you decode the whole file.
>>
>>
https://codereview.appspot.**com/7486051/<https://codereview.appspot.com/7486...
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>> .
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>
>  --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>



-- 
Michael T. Jones | Chief Technology Advocate  | mtj@google.com |  +1
650-335-5765

[jeff.allen, 2013-03-14 17:22:48]

Hello nigeltao@golang.org, r@golang.org, bradfitz@golang.org, mtj@google.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2013-03-14 17:44:20]

i had the problem backwards. looks good but please fix these two nits.

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/decode_test.go
File src/pkg/image/decode_test.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/decode_test.g...
src/pkg/image/decode_test.go:125: t.Errorf("Got error %v", err)
t.Errorf("expected 'not enough pixels' error; got %v", err)

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go...
src/pkg/image/gif/reader.go:189: return fmt.Errorf("gif: not enough pixels")
return errors.New("gif: not enough pixels")

[r, 2013-03-14 17:45:40]

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go
File src/pkg/image/gif/reader.go (right):

https://codereview.appspot.com/7486051/diff/10001/src/pkg/image/gif/reader.go...
src/pkg/image/gif/reader.go:189: return fmt.Errorf("gif: not enough pixels")
actually this isn't a very good message. how about
return errors.New("gif: file too short for rectangle defined in header")

[jeff.allen, 2013-03-15 08:30:37]

Hello nigeltao@golang.org, r@golang.org, bradfitz@golang.org, mtj@google.com
(cc: golang-dev@googlegroups.com),

Please take another look.

[minux1, 2013-03-15 08:37:59]

could we use a smaller gif for the test?
i think we can just take a minimal gif
and manually corrupt the bounding box
size in its header.

[nigeltao, 2013-03-15 08:42:41]

On 2013/03/14 16:36:59, bradfitz wrote:
> [ header ] [ image is 99999999 x 99999999 pixels! ] [ lie. only 400 pixels
> ] [ EOF ]
> 
> And we'd read it in that order, and allocate 99999999 * 99999999 pixels
> ahead of time, only to fail later when we found it was short.
> 
> With his change, he reads the dimensions, notes it, and then slurps all the
> pixels and verifies they match, having only allocated 400 pixels worth of
> memory.

Well, if your program has memory problems with this bad image, would it also
have memory problems with a legit GIF that was actually 99999999 * 99999999?
Even if that's a lot of black pixels, a long stream of zeroes still compresses
very well. This CL might be fixing a particular symptom without addressing a
bigger problem of being able to decode untrusted images without allowing denial
of service. That problem isn't specific to GIF either.

Some more thought is required.

[nigeltao, 2013-03-15 11:17:41]

On Fri, Mar 15, 2013 at 7:42 PM,  <nigeltao@golang.org> wrote:
> Some more thought is required.

I dropped some more thoughts at
https://code.google.com/p/go/issues/detail?id=5050#c1

[nigeltao, 2013-03-15 11:18:00]

On Fri, Mar 15, 2013 at 7:42 PM,  <nigeltao@golang.org> wrote:
> Some more thought is required.

I dropped some more thoughts at
https://code.google.com/p/go/issues/detail?id=5050#c1

[jeff.allen, 2013-03-15 16:19:23]

Please see https://codereview.appspot.com/7602045/ for a better solution to
issue 5050.