Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image/gif: EncodeAll panics when a palette is too large to encode #20249

Closed
topherbullock opened this issue May 4, 2017 · 5 comments
Closed

image/gif: EncodeAll panics when a palette is too large to encode #20249

topherbullock opened this issue May 4, 2017 · 5 comments

Comments

@topherbullock
Copy link

@topherbullock topherbullock commented May 4, 2017

What version of Go are you using (go version)?

1.8.1

What operating system and processor architecture are you using (go env)?

darwin / amd64

What did you do?

Created a gif image with a palette ( either global or local color table for a frame ) containing > 256 colors.

https://play.golang.org/p/xfDZCcSIFF

What did you expect to see?

The EncodeAll function returns errors when image blocks are too large, on a zero-length gif, etc. As such, in this case since we know the gif can't be encoded due to the palette length being > 256, I would expect a similar error, along the lines of errors.New("gif: image palette is too large to encode") to match the style here

What did you see instead?

It panics here trying to read log2Lookup[-1] defined here

I have a change-set ready with the proposed functionality and accompanying tests.

@bradfitz bradfitz added the NeedsFix label May 4, 2017
@bradfitz bradfitz added this to the Go1.10 milestone May 4, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 4, 2017

Go 1.9 is fine too if trivial/safe.

/cc @nigeltao

@gopherbot
Copy link

@gopherbot gopherbot commented May 5, 2017

CL https://golang.org/cl/42790 mentions this issue.

@topherbullock
Copy link
Author

@topherbullock topherbullock commented May 5, 2017

@bradfitz + @nigeltao Ah, okay.. Welp, that looks a lot like what I had ready to submit, so LGTM :D

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented May 6, 2017

Ah, I didn't read your final line that said you had a change-set ready. Sorry.

@gopherbot gopherbot closed this in 6d9b900 May 6, 2017
@topherbullock
Copy link
Author

@topherbullock topherbullock commented May 9, 2017

@nigeltao No worries 😄

@golang golang locked and limited conversation to collaborators May 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.