Summary: | Crash in Image Decoder due to large gifs | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | gordon <gosun> | ||||||||||||||||||
Component: | Images | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | UNCONFIRMED --- | ||||||||||||||||||||
Severity: | Normal | CC: | benjamin, bfulgham, cand, cgarcia, charles.wei, kling, laszlo.gombos, ossy, ostap73, rwlbuis, staikos | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
gordon
2013-05-05 23:11:02 PDT
Created attachment 200606 [details]
The decoded GIF size > 5M, reserve the initialized frame, when the current frame is the last frame
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. Created attachment 200867 [details]
The decoded GIF size > 5M, reserve the initialized frame
Created attachment 201016 [details]
The decoded GIF size > 5M, reserve the initialized frame
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? 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. 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? (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. Thank you. PR:328689 Created attachment 201317 [details]
If the decoded GIF size > 5M, preserve the frame data.
Crash in Image Decoder on Master_41 due to gifs.
Created attachment 201331 [details]
If the decoded GIF size > 5M, preserve the frame data.
Crash in Image Decoder on Master_41 due to gifs.
Created attachment 201332 [details]
If the decoded GIF size > 5M, preserve the frame data.
Crash in Image Decoder on Master_41 due to gifs.
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.
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-" 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. +Benjamin, Brent to raise the awareness for review. (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. 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. 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.
|