image/gif: panics for several GIFs

[CSEMike]

I've observed several panics in image/draw when working with real-world GIFs that work
with other libraries (e.g., they display in Chrome.)

Attached are two representative images that induce different failures and a program to
repro.

I've tested with two versions:
Version:  devel +62bf913b4f40 Wed Feb 27 20:55:01 2013 -0800
Version:  devel +1d079908dd84 Thu Apr 25 18:47:12 2013 +0200

Stack traces are below. Please let me know if there's anything else you need. Thanks!

From 62bf913b4f40 --

For crash1.gif --
$ go run imagecrash.go crash1.gif 
Version:  devel +62bf913b4f40 Wed Feb 27 20:55:01 2013 -0800
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x20 pc=0x44786f]

goroutine 1 [running]:
image/draw.drawRGBA(0x4c20006f0c0, 0x0, 0x0, 0x1, 0x1, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:473 +0x42f
image/draw.DrawMask(0x4c20006f100, 0x4c20006f0c0, 0x0, 0x0, 0x1, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:114 +0x35d
image/draw.Draw(0x4c20006f100, 0x4c20006f0c0, 0x0, 0x0, 0x1, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:37 +0xbb
main.main()
    /usr/local/google/home/piatek/speed/piatek-pss-git2/google3/experimental/users/piatek/imagecrash.go:30 +0x447
exit status 2

For crash2.gif --
$ go run imagecrash.go crash2.gif 
Version:  devel +62bf913b4f40 Wed Feb 27 20:55:01 2013 -0800
panic: runtime error: index out of range

goroutine 1 [running]:
image.(*Paletted).At(0x4c2000700c0, 0x0, 0x23, 0x4c200059480, 0xfffbfbfb, ...)
    /usr/lib/google-golang/src/pkg/image/image.go:822 +0x15e
image/draw.drawRGBA(0x4c20006f0c0, 0x0, 0x0, 0x12c, 0x24, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:473 +0x421
image/draw.DrawMask(0x4c20006f100, 0x4c20006f0c0, 0x0, 0x0, 0x12c, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:114 +0x35d
image/draw.Draw(0x4c20006f100, 0x4c20006f0c0, 0x0, 0x0, 0x12c, ...)
    /usr/lib/google-golang/src/pkg/image/draw/draw.go:37 +0xbb
main.main()
    /usr/local/google/home/piatek/speed/piatek-pss-git2/google3/experimental/users/piatek/imagecrash.go:30 +0x447
exit status 2

Attachments:

1. imagecrash.go (498 bytes)
2. crash1.gif (29 bytes)
3. crash2.gif (2533 bytes)

[vdobler]

Comment 2:

These are actually two different issues:
A)
crash1.gif has an no palette information and thus the decoded
image has a zero length Palette and image.Palletted.At returns
a nil color.Color in this case. See 
http://tip.golang.org/src/pkg/image/image.go?s=22798:22839#L814
The final panic happens in 
http://tip.golang.org/src/pkg/image/draw/draw.go?s=14011:14050#L473
which does not check on this nil value.
image.Paletted is the only image type which returns a nil
color.Color in a call to At(x,y).
Possible fixes: Either check for nil Color during draw or have
image.Paletted.At return a color.RGBA{} default value like in
the case of (x,y) not in the image bounds.
B)
chrash2.gif has a palette with 128 entries but pixel (0,35)
references color no 255 and thus panics in
http://tip.golang.org/src/pkg/image/image.go?s=22929:22948#L814
It could be argued, that the gif is malformed and decoding
should fail.
It seems that other tools (checked GIMP only) handle both cases
gracefully by substituting black in both cases above.

[robpike]

Comment 3:

Labels changed: added priority-soon, removed priority-triage.

Owner changed to @nigeltao.

Status changed to Accepted.

[gopherbot]

Comment 4 by jeff.allen:

I'd be willing to work on this (especially with the great diagnosis from dr.volker,
thanks!)
The last time we were looking at fixing a problem like this (issue #3565) I decided it
was better to not fix the bug and continue rejecting the GIF because the breakage was in
its LZW encoding.
This time, it seems reasonable to accept these images because while they are clearly
invalid, the fix is within image/gif/*.go and the behavior of adding a black pixel in is
not very controversial.
Nigel, please give a "pre-LGTM" before I start on this. :)

[nigeltao]

Comment 5:

I'd prefer to reject invalid images (as an error) instead of implying black pixels. I
think both cases should be fixed in image/gif instead of image or image/draw.
Specifically, gif.DecodeXxx should return an error where it currently allows returning
an image.Paletted with (A) an empty Palette or (B) a Pix element >= len(Palette). The
second check (B) can be elided if len(Palette) == 255.
The spec (http://www.w3.org/Graphics/GIF/spec-gif89a.txt) section 22 "Table Based Image
Data" says that "Each index must be within the range of the size of the active color
table, starting at 0."

[gopherbot]

Comment 6 by jeff.allen:

OK, CL coming.

[nigeltao]

Comment 7:

This issue was closed by revision 8192017.

Status changed to Fixed.