Summary: | ImageDecoder setSize() should check for backing store allocation failure | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | noel gordon <noel.gordon> | ||||||||||||
Component: | New Bugs | Assignee: | noel gordon <noel.gordon> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, kling, mihaip, mrobinson, pkasting, webkit.review.bot, zimmermann | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 72245, 73812 | ||||||||||||||
Attachments: |
|
Description
noel gordon
2011-11-21 00:23:23 PST
Created attachment 116045 [details]
Patch
Created attachment 116066 [details]
Patch
Not sure we have a super-sized image test. Adding Mihai due to bug 48634, and folks from other ports. Comment on attachment 116066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review Good idea! > Source/WebCore/ChangeLog:20 > + No new tests. Covered by existing tests. Is it really? If not, it should be pretty easy to construct a gigantic image that still has a small file size. > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > + size_t backingStoreSize = newWidth * newHeight; This could overflow on 32-bit platforms. > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:194 > + if (!m_backingStore.tryReserveCapacity(backingStoreSize)) tryReserveCapacity() will not update Vector::m_size. You need to resize() it after tryReserveCapacity() succeeds. fast/images/size-failure.html should be testing a large image. (In reply to comment #4) > (From update of attachment 116066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > > + size_t backingStoreSize = newWidth * newHeight; > > This could overflow on 32-bit platforms. D'oh, I completely missed the comment in the ChangeLog about overflow being covered. (In reply to comment #5) > fast/images/size-failure.html should be testing a large image. Great, thanks! (In reply to comment #5) > fast/images/size-failure.html should be testing a large image. Added you for a reason :) will refer to that test to the ChangeLog. % find . -name Skipped | xargs grep size-failure ./gtk/Skipped:fast/images/size-failure.html ./mac-snowleopard/Skipped:fast/images/size-failure.html gtk covered by bug 72245, mac-snowleopard by a radar bug. (In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 116066 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:193 > > > + size_t backingStoreSize = newWidth * newHeight; > > > > This could overflow on 32-bit platforms. > > D'oh, I completely missed the comment in the ChangeLog about overflow being covered. No worries, bug 48634 was instructive and I thought it worth mentioning. (In reply to comment #4) > (From update of attachment 116066 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116066&action=review > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:194 > > + if (!m_backingStore.tryReserveCapacity(backingStoreSize)) > > tryReserveCapacity() will not update Vector::m_size. You need to resize() it after tryReserveCapacity() succeeds. Indeed you are correct. However, the image decoders don't use/require a Vector:m_size. They instead access the underlying Vector buffer() via the m_btyes member ... if (!m_backingStore.tryReserveCapacity(backingStoreSize)) return false; m_bytes = m_backingStore.data(); ... when writing decoded image pixel data into the buffer (m_backingStore). Still I don't mind adding the resize() if necessary. Your point did make me stare at the ASSERT(m_backingStore.isEmpty()) some more - it's wrong - changed it to ASSERT(!m_bytes). Created attachment 116172 [details]
Patch
Anything more I need to do here? I mean, you could add a comment about not calling resize. I think the difference is that you're not going to call the constructor on all the elements of the vector. Comment on attachment 116172 [details] Patch Clearing flags on attachment: 116172 Committed r101302: <http://trac.webkit.org/changeset/101302> All reviewed patches have been landed. Closing bug. The m_bytes assertion fires on lots of bots - rolling this out for now... Reopened since the patch was rolled out. (In reply to comment #15) > The m_bytes assertion fires on lots of bots - rolling this out for now... ImageFrame::ImageFrame() does not initialize m_size at construction time, seems easy to fix. Other ports ASSERT the setSize() contract - that it only be called once - using ASSERT(width() == 0 && height() == 0), so I'll use that. Add comment Adam asked for about not needing to resize the Vector backing store. Preparing a patch ... ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll use nots instead. Created attachment 117068 [details]
Patch
(In reply to comment #18) > ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll > use nots instead. Filed bug 73490 to remove these style nits elsewhere. Here we're just waiting on the gtk ews to return and catch-up. Question reviewers: is setting m_data to 0 correct? ImageFrame::operator() = is overloaded. It calls ImageFrame::copyBitmapData(const ImageFrame& other), which sets ... m_backingStore = other.m_backingStore; m_bytes = m_backingStore.data(); I would have expected "m_data = other.m_data" here, what am I missing? Specifically, should we write it like this ... m_backingStore = other.m_backingStore; m_bytes = other.m_data; if (m_bytes) m_bytes = m_backingStore.data(); /shrug/, maybe not touch m_data at all. Created attachment 117821 [details]
Patch for landing
Comment on attachment 117821 [details] Patch for landing Clearing flags on attachment: 117821 Committed r101975: <http://trac.webkit.org/changeset/101975> All reviewed patches have been landed. Closing bug. Following GIF tests were unhappy on GTK Release bots. Filed bug 73812. fast/images/dont-crash-with-null-gif-frames.html = CRASH fast/backgrounds/animated-gif-as-background.html = CRASH fast/images/gif-loop-count.html = CRASH |