Bug 72864 - ImageDecoder setSize() should check for backing store allocation failure
Summary: ImageDecoder setSize() should check for backing store allocation failure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on:
Blocks: 72245 73812
  Show dependency treegraph
 
Reported: 2011-11-21 00:23 PST by noel gordon
Modified: 2011-12-05 03:01 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.26 KB, patch)
2011-11-21 00:29 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2011-11-21 04:25 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2011-11-21 19:51 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (2.97 KB, patch)
2011-11-29 16:19 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch for landing (2.54 KB, patch)
2011-12-04 18:32 PST, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2011-11-21 00:23:23 PST
ImageDecoder setSize() should check for backing store allocation failure.
Comment 1 noel gordon 2011-11-21 00:29:12 PST
Created attachment 116045 [details]
Patch
Comment 2 noel gordon 2011-11-21 04:25:09 PST
Created attachment 116066 [details]
Patch
Comment 3 noel gordon 2011-11-21 04:51:38 PST
Not sure we have a super-sized image test.  Adding Mihai due to bug 48634, and folks from other ports.
Comment 4 Andreas Kling 2011-11-21 08:09:25 PST
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.
Comment 5 Mihai Parparita 2011-11-21 08:12:19 PST
fast/images/size-failure.html should be testing a large image.
Comment 6 Andreas Kling 2011-11-21 08:16:47 PST
(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!
Comment 7 noel gordon 2011-11-21 19:12:45 PST
(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.
Comment 8 noel gordon 2011-11-21 19:16:19 PST
(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.
Comment 9 noel gordon 2011-11-21 19:47:56 PST
(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).
Comment 10 noel gordon 2011-11-21 19:51:19 PST
Created attachment 116172 [details]
Patch
Comment 11 noel gordon 2011-11-28 14:13:14 PST
Anything more I need to do here?
Comment 12 Adam Barth 2011-11-28 14:34:48 PST
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 13 WebKit Review Bot 2011-11-28 16:23:15 PST
Comment on attachment 116172 [details]
Patch

Clearing flags on attachment: 116172

Committed r101302: <http://trac.webkit.org/changeset/101302>
Comment 14 WebKit Review Bot 2011-11-28 16:23:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Nikolas Zimmermann 2011-11-29 01:18:01 PST
The m_bytes assertion fires on lots of bots - rolling this out for now...
Comment 16 Peter Kasting 2011-11-29 11:01:45 PST
Reopened since the patch was rolled out.
Comment 17 noel gordon 2011-11-29 16:01:46 PST
(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 ...
Comment 18 noel gordon 2011-11-29 16:16:32 PST
ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll
use nots instead.
Comment 19 noel gordon 2011-11-29 16:19:48 PST
Created attachment 117068 [details]
Patch
Comment 20 noel gordon 2011-11-30 14:20:09 PST
(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.
Comment 21 noel gordon 2011-12-01 14:36:37 PST
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?
Comment 22 noel gordon 2011-12-01 14:53:17 PST
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();
Comment 23 noel gordon 2011-12-04 18:31:58 PST
/shrug/, maybe not touch m_data at all.
Comment 24 noel gordon 2011-12-04 18:32:22 PST
Created attachment 117821 [details]
Patch for landing
Comment 25 WebKit Review Bot 2011-12-04 23:58:35 PST
Comment on attachment 117821 [details]
Patch for landing

Clearing flags on attachment: 117821

Committed r101975: <http://trac.webkit.org/changeset/101975>
Comment 26 WebKit Review Bot 2011-12-04 23:58:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 noel gordon 2011-12-05 03:00:59 PST
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