Bug 115623 – Crash in Image Decoder due to large gifs

• Component: Images
• Status: UNCONFIRMED

[gordon]

It crashes while playing with the browser for a while, with the following back
trace:

#0  0x7a61730c in setRGBA (a=255, b=<optimized out>, g=<optimized out>, r=255,
dest=<optimized out>, this=<optimized out>)
    at
/home/cswei/project/playbook/webkit/Source/WebCore/platform/image-decoders/ImageDecoder.h:197
#1  WebCore::GIFImageDecoder::haveDecodedRow (this=0x7dd6d940,
frameIndex=<optimized out>, rowBuffer=..., width=<optimized out>, rowNumber=0, 
    repeatCount=1, writeTransparentPixels=false)
    at
webkit/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:236

It is because the decoded GIF size > 5M.

[gordon]

Created attachment 200606 [details]
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame

[Andreas Kling]

Comment on attachment 200606 [details]
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame

View in context: https://bugs.webkit.org/attachment.cgi?id=200606&action=review

> Source/WebCore/ChangeLog:8
> +        Crash in Image Decoder due to large gifs
> +        https://bugs.webkit.org/show_bug.cgi?id=115623
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/images/animated-large-image-crash.html

You need to explain what the bug is, and why you are fixing it this way.

[gordon]

Created attachment 200867 [details]
The decoded GIF size > 5M, reserve the initialized frame

[gordon]

Created attachment 201016 [details]
The decoded GIF size > 5M, reserve the initialized frame

[Benjamin Poulain]

Comment on attachment 201016 [details]
The decoded GIF size > 5M, reserve the initialized frame

View in context: https://bugs.webkit.org/attachment.cgi?id=201016&action=review

> LayoutTests/ChangeLog:12
> +        If the decoded GIF is larger than 5M,
> +        we clear the the frame buffer except the current and after frame.
> +        However, if the current frame is the last frame of the GIF,
> +        and the first frame has been re-initialized for next time,
> +        then should not clear first frame, it should be reserved.

This does not explain the test.

Especially since it will decode either only the first frame or no frame at all.

> LayoutTests/ChangeLog:16
> +        * fast/images/resources/gif-large.gif: Added.

Do you own the rights on this?
Better create a new test image yourself.

> Source/WebCore/ChangeLog:13
> +        If the decoded GIF is larger than 5M,
> +        we clear the the frame buffer except the current and after frame.
> +        However, if the current frame is the last frame of the GIF,
> +        and the first frame has been re-initialized for next time,
> +        then should not clear first frame, it should be reserved.
> +        Test: fast/images/animated-large-image-crash.html

This does not explain the reason of the crash.

> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:270
> +
> +    size_t currentFrame() const
> +    {
> +        return m_currentDecodingFrame;
> +    }
> +

Why do you suddenly expose this publicly?

[gordon]

Created attachment 201051 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

Thank you for your comments。
> + Why do you suddenly expose this publicly?
I use it to calculate how many frames have been re-initialized, so it should be public.

[Rob Buis]

Comment on attachment 201051 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

View in context: https://bugs.webkit.org/attachment.cgi?id=201051&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=115623

Is there a PR? Can you try to get an internal review first?

[Rob Buis]

(In reply to comment #7)
> (From update of attachment 201051 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201051&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=115623
> 
> Is there a PR? Can you try to get an internal review first?

Nm, I saw that Benjamin already reviewed it, so you still have to address his issues. I'd also still like to know if there is a PR since that is valuable information if we look at he landed commit a few months later.

[gordon]

Thank you.
PR:328689

[gordon]

Created attachment 201317 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.

[gordon]

Created attachment 201331 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.

[gordon]

Created attachment 201332 [details]
If the decoded GIF size > 5M, preserve the frame data.

Crash in Image Decoder on Master_41 due to gifs.

[Benjamin Poulain]

Comment on attachment 201332 [details]
If the decoded GIF size > 5M, preserve the frame data.

You should put back the test, this should be tested.
My concern about your previous test was just about the rights. You can just create your own animated gif to solve that.

[gordon]

Sorry, I can't upload this test. Because it will block other build, such as QT.

This patch just fix the BlackBerry, because the previous patch has been "r-"

[gordon]

Created attachment 201519 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

> + Why do you suddenly expose this publicly? 
I use it to calculate how many frames have been re-initialized, so it should be public.

[Charles Wei]

+Benjamin, Brent to raise the awareness for review.

[Alexey Proskuryakov]

(In reply to comment #14)
> Sorry, I can't upload this test. Because it will block other build, such as QT.

A test can be skipped on other ports by adding an entry to TestExpectations file.

[cand]

I confirm this bug on r169702, on Google's homepage no less!
I also confirm that gosun's last patch (I only applied the gif/* parts, as the assert change in ImageDecoder would conflict with a cairo patch) fixes it.

This is just sad that a serious bug like this can stay open for over a year.

[Brady Eidson]

Comment on attachment 201519 [details]
If the decoded GIF size > 5M, preserve the re-initialized frame

Patch no longer applies to trunk.
If this is still an issue, needs a new patch.