Bug 15778 - Public GIF image decoder can corrupt memory on malformed GIFs
Summary: Public GIF image decoder can corrupt memory on malformed GIFs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-31 14:16 PDT by Peter Kasting
Modified: 2007-11-01 02:41 PDT (History)
0 users

See Also:


Attachments
patch v1 (3.76 KB, patch)
2007-10-31 14:24 PDT, Peter Kasting
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2007-10-31 14:16:06 PDT
This bug report is about the code in WebCore/platform/image-decoders/gif, so it affects the Qt/Cairo versions of WebCore, but not Safari (which uses its own decoders).

When a malformed GIF specifies a frame (after the first) that is larger than the overall image, the GIF decoder does not properly check to avoid writing past the end of the memory buffer, and corrupts memory.

Patch coming shortly.
Comment 1 Peter Kasting 2007-10-31 14:24:23 PDT
Created attachment 16968 [details]
patch v1

This patch does a couple of things:
* Explicitly ignores rows past the end of the image in GIFImageDecoder::haveDecodedRow().  This is similar to the behavior of the Mozilla image decoder sources, which allow a frame decoder to provide whatever data it wants, and then simply ignore the excess data when compositing/drawing the image.
* Removes some broken code in GIFImageReader::output_row() that looks like it was trying to prevent this case, but didn't really succeed.  This code isn't present in the Mozilla GIF decoder this file is based on, doesn't work right, and is significantly more complex than the simple added conditional described above.
* Fixes some code in GIFImageDecoder::read() that would be hit in this case to set local variables more correctly.  This doesn't make much difference, but it's much closer to the original Mozilla code -- it looks like when the file was ported to WebKit, someone originally misplaced a close brace in here, so e.g. screen_height would only get reset if screen_width also needed to be reset.

The current Mozilla sources avoid the "rowbuf" struct entirely, but updating to that change is both a large patch and not really related to this bug.
Comment 2 Dave Hyatt 2007-10-31 14:25:19 PDT
Comment on attachment 16968 [details]
patch v1

r=me
Comment 3 Mark Rowe (bdash) 2007-11-01 02:41:10 PDT
Landed in r27346.