513738 - crash (segfault) @ nsGIFDecoder2::GifWrite nsGIFDecoder2.cpp:1112 (memset)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: gif file that causes crash

Build Identifier: mozilla-central revision 6aab4d7bb538

Firefox crashes when viewing the attached gif file.

Reproducible: Always

Steps to Reproduce:
1. Load attached file.

Actual Results:  
firefox crashes

Expected Results:  
firefox doesn't crash

Note: doesn't appear to crash 3.5, but it still crashes m-c nightlies going back at least a month.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: stack trace of relevant thread

Comment 2

[Alfred Kayser]

Probably caused by bug 753:
When a gif has local colormaps in animated frames, mColormap is not allocated correctly.

Comment 3

[Alfred Kayser]

Attachment: Fix error handling for too large frames

Issue found: the attached image has a frame which is very large, actually too large for imgFrame, but BeginImageFrame doesn't return the error back to the state engine, which then happily tries to use the result of the failed BeginImageFrame.

And indeed this is caused by bug 753. Comparable to bug 505474 where the old code uses mImageData to check if BeginImageFrame was successful, this doesn't work anymore in the new code, so adding a check for nsresult will make this more error proof.

Comment 4

[Alfred Kayser]

Same blocking level as bug 505474.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 397834 [details] [diff] [review]
Fix error handling for too large frames

Remove the unrelated indendentation changes and we're all good!

Comment 6

[Alfred Kayser]

Attachment: Removed the whitespace fix.

Comment 7

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/94678be14027

Comment 8

[Samuel Sidler (old account; do not CC)]

Comment on attachment 398090 [details] [diff] [review]
Removed the whitespace fix.

As a security bug, per our new policy, this patch needs explicit superview from
a second person. It should *not* have landed on trunk without that
superview. Please be mindful of that in the future. (Reed, you should know better.)

http://www.mozilla.org/hacking/reviewers.html

Tagging vlad for superview.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 398090 [details] [diff] [review]
Removed the whitespace fix.

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz for release-drivers

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 398090 [details] [diff] [review]
Removed the whitespace fix.

a1.9.2=shaver

Comment 11

[Reed Loden [:reed]]

Patch doesn't apply on 1.9.1 or 1.9.0 due to code changes. Need new patch.

1.9.2 -- http://hg.mozilla.org/releases/mozilla-1.9.2/rev/860d8d674561

Comment 12

[Alfred Kayser]

This patch is not needed on 1.9.1 or older, as the missing error check that is fixed in this patch is not needed there, as the old code checks on mImageFrame!=null as the condition for successful BeginImageFrame.
This is not very clean, but 'it works'.
See comment #3. This issue was introduced by bug 753, where mImageFrame was removed, removing the error check as well. Bug 753 has not been applied to 191 and never will.