Bug 72864 – ImageDecoder setSize() should check for backing store allocation failure

• Component: New Bugs
• Status: RESOLVED FIXED

[noel gordon]

ImageDecoder setSize() should check for backing store allocation failure.

[noel gordon]

Created attachment 116045 [details]
Patch

[noel gordon]

Created attachment 116066 [details]
Patch

[noel gordon]

Not sure we have a super-sized image test.  Adding Mihai due to bug 48634, and folks from other ports.

[Andreas Kling]

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.

[Mihai Parparita]

fast/images/size-failure.html should be testing a large image.

[Andreas Kling]

(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!

[noel gordon]

(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.

[noel gordon]

(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.

[noel gordon]

(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).

[noel gordon]

Created attachment 116172 [details]
Patch

[noel gordon]

Anything more I need to do here?

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 116172 [details]
Patch

Clearing flags on attachment: 116172

Committed r101302: <http://trac.webkit.org/changeset/101302>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

The m_bytes assertion fires on lots of bots - rolling this out for now...

[Peter Kasting]

Reopened since the patch was rolled out.

[noel gordon]

(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 ...

[noel gordon]

ASSERT(width() == 0 && height() == 0) fails to pass webkit check style, so I'll
use nots instead.

[noel gordon]

Created attachment 117068 [details]
Patch

[noel gordon]

(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.

[noel gordon]

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?

[noel gordon]

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();

[noel gordon]

/shrug/, maybe not touch m_data at all.

[noel gordon]

Created attachment 117821 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 117821 [details]
Patch for landing

Clearing flags on attachment: 117821

Committed r101975: <http://trac.webkit.org/changeset/101975>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

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