Bug 48634 – fast/images/size-failure.html results in malloc of 2 Gb after switching to WebKit image decoders

• Component: Images
• Status: RESOLVED FIXED

[Mikhail Naganov]

I tried rebaselining tests, and found the discrepancy (see the screenshot). This appeared on Mac after you had fixed Chromium build.

[Mikhail Naganov]

Created attachment 72331 [details]
screenshot from rebaseline

[Dimitri Glazkov (Google)]

It's the same issue I've seen the first time I rolled out the patch.

[Mikhail Naganov]

Looks like these 4 WebGL tests that check color values were also affected by this change. At least, they started failing right after compilation was fixed, and only on Mac.

  fast/canvas/webgl/gl-teximage.html = TEXT
  fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html = TEXT
  fast/canvas/webgl/tex-image-with-format-and-type.html = TEXT
  fast/canvas/webgl/texture-transparent-pixels-initialized.html = TEXT

[Adam Barth]

I'll investigate some more.

[Adam Barth]

A number of these tests don't use images at all.  Did you actually observe these tests fail, or just that they were flaky?

Also notice that the shadow seems to correspond to where the scroll bars aren't.

[Mikhail Naganov]

Three reasons I attributed those failures to your change:
 - tests were passing before compilation on mac broke, and started failing after you fixed it;
 - they fail only on mac;
 - I was needed to make a roll having ~100 tests failing, so I had to do something with them.

If I can somehow help you resolving this problem, please tell.

[Adam Barth]

> Three reasons I attributed those failures to your change:

Oh, I definitely think its related.  I'm trying to reproduce locally.

> If I can somehow help you resolving this problem, please tell.

Thanks.

[Mihai Parparita]

Strangely, this seems to happen with even with tests that have scrollbars, but only in release mode:

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_5/results/layout-test-results/fast/lists/001-vertical-actual.png (release build)
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_5__dbg__2_/results/layout-test-results/fast/lists/001-vertical-actual.png (debug build)

[Adam Barth]

Ah, I was testing in Debug.  I'll try again in Release.  Maybe initialized memory that inited to zero in Debug?

[Mihai Parparita]

James has a Leopard laptop, I built test_shell on it in Release mode and was able to reproduce by running it:

./webkit/tools/layout_tests/run_webkit_tests.sh --verbose --release fast/images/size-failure.html fast/images/svg-width-100p-as-background.html

fast/images/size-failure.html  claims to pass, but has this output:

2010-11-11 17:21:49,387 dump_render_tree_thread.py:432  DEBUG Thread-1 fast/images/size-failure.html passed
2010-11-11 17:21:49,504 dump_render_tree_thread.py:118  DEBUG Previous test output stderr lines:
TestShell(54495,0xa0177720) malloc: *** mmap(size=2147483648) failed (error code=12)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug
2010-11-11 17:21:49.375 TestShell[54495:10b] Attempt to allocate -2147483648 bytes for NS/CFData failed

fast/images/svg-width-100p-as-background.html then fails with this output:

expected: http://trac.webkit.org/export/71871/trunk/LayoutTests/platform/mac/fast/images/svg-width-100p-as-background-expected.png
actual: http://build.chromium.org/f/chromium/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/images/svg-width-100p-as-background-actual.png
diff: http://build.chromium.org/f/chromium/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/images/svg-width-100p-as-background-diff.png

After that (and I'm guessing till the DRT gets restarted), scrollbar rendering is screwed up. I assume we shouldn't be trying to malloc that many bytes in the first place, and the failure leaves everything in a screwed up state.

[Adam Barth]

Can we get a backtrace of the failed allocation?  It would be interesting to see purported width and height of image we're trying to allocate.

[Mihai Parparita]

(In reply to comment #11)
> Can we get a backtrace of the failed allocation?  It would be interesting to see purported width and height of image we're trying to allocate.

As it turns out, the malloc failure happens on Snow Leopard with test_shell, so this is easier to reproduce:


#0  0x976ead12 in malloc_error_break ()
#1  0x976ebeb8 in szone_error ()
#2  0x975f6ec0 in allocate_pages ()
#3  0x976077b2 in large_malloc ()
#4  0x975f8f90 in szone_malloc_should_clear ()
#5  0x975fa08b in malloc_zone_calloc ()
#6  0x9974fe48 in __CFDataAllocate ()
#7  0x997523f5 in __CFDataGrow ()
#8  0x99796c29 in CFDataSetLength ()
#9  0x010515b8 in WebCore::RGBA32Buffer::setSize (this=0x6824600, newWidth=14489, newHeight=34615) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/cg/ImageDecoderCG.cpp:64
#10 0x01051be2 in WebCore::GIFImageDecoder::initFrameBuffer (this=0x5e17c30, frameIndex=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageDecoder.cpp:343
#11 0x01052593 in WebCore::GIFImageDecoder::haveDecodedRow (this=0x5e17c30, frameIndex=0, rowBuffer=0x681a600 "\313\314\313\314\320\314\314\314\332\314\314\314\314\314\314\314\314#\314\314\314##\351#\321\324\b\b\325\b\325\325\3308\3308\3308\330l88l", rowEnd=0x681a699 "", rowNumber=0, repeatCount=1, writeTransparentPixels=false) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageDecoder.cpp:223
#12 0x01053aff in GIFImageReader::output_row (this=0x5e20930) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageReader.cpp:161
#13 0x010540a0 in GIFImageReader::do_lzw (this=0x5e20930, q=0x6841b21 "") at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageReader.cpp:356
#14 0x01054396 in GIFImageReader::read (this=0x5e20930, buf=0x6841c20 "\377V\275\235v^\2335\367\"\320\255\363\341\303\351a\a\023>\234t\370\323\342\215{\215O\316u9\346\257\316A\a\235t\264\035dPg\026a\207\035i\v\266\365\253\203\342}w\027m\tRd\323\205\236\261\027\3155\351/\243\020BB\t5\224Q\210\325\307\324}\214Q\265\037\177\375Q6\220\210!\372\246\315`\003%5\320\215c\351d\221^\025\365h\351i\032\241\024$J\336M(\223\205z]\230\336z\032\246\343\244O\004\306'\242a$\036\325\214}\3675\346\330\212,b\265\234? .\363\314P1\n\365\"\2158\342h\226g<\266\351#m8T\223\321Ek\305V\333D\350\211ySn\032r\332\016Y1\212)fpI"..., len=33476, query=WebCore::GIFImageDecoder::GIFFullQuery, haltAtFrame=1) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageReader.cpp:446
#15 0x0105202d in WebCore::GIFImageDecoder::decode (this=0x5e17c30, haltAtFrame=1, query=WebCore::GIFImageDecoder::GIFFullQuery) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageDecoder.cpp:318
#16 0x010520e7 in WebCore::GIFImageDecoder::frameBufferAtIndex (this=0x5e17c30, index=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/image-decoders/gif/GIFImageDecoder.cpp:127
#17 0x0103f13e in WebCore::ImageSource::createFrameAtIndex (this=0x5e17924, index=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/ImageSource.cpp:132
#18 0x00fe4183 in WebCore::BitmapImage::cacheFrame (this=0x5e17910, index=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/BitmapImage.cpp:121
#19 0x00fe4960 in WebCore::BitmapImage::frameAtIndex (this=0x5e17910, index=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/BitmapImage.cpp:213
#20 0x00f921f6 in WebCore::BitmapImage::draw (this=0x5e17910, ctxt=0xbfffbf48, destRect=@0xbfffad1c, srcRect=@0xbfffad0c, styleColorSpace=WebCore::ColorSpaceDeviceRGB, compositeOp=WebCore::CompositeSourceOver) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/cg/ImageCG.cpp:162
#21 0x01001231 in WebCore::GraphicsContext::drawImage (this=0xbfffbf48, image=0x5e17910, styleColorSpace=WebCore::ColorSpaceDeviceRGB, dest=@0xbfffadf0, src=@0xbfffade0, op=WebCore::CompositeSourceOver, useLowQualityScale=false) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/GraphicsContext.cpp:413
#22 0x010012a3 in WebCore::GraphicsContext::drawImage (this=0xbfffbf48, image=0x5e17910, styleColorSpace=WebCore::ColorSpaceDeviceRGB, dest=@0xbfffaf24, srcRect=@0xbfffae40, op=WebCore::CompositeSourceOver, useLowQualityScale=false) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/GraphicsContext.cpp:329
#23 0x010013e2 in WebCore::GraphicsContext::drawImage (this=0xbfffbf48, image=0x5e17910, styleColorSpace=WebCore::ColorSpaceDeviceRGB, r=@0xbfffaf24, op=WebCore::CompositeSourceOver, useLowQualityScale=false) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/graphics/GraphicsContext.cpp:319
#24 0x015c6e38 in WebCore::RenderImage::paintIntoRect (this=0x5e1c78c, context=0xbfffbf48, rect=@0xbfffaf24) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderImage.cpp:355
#25 0x015c787f in WebCore::RenderImage::paintReplaced (this=0x5e1c78c, paintInfo=@0xbfffb0fc, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderImage.cpp:291
#26 0x01620030 in WebCore::RenderReplaced::paint (this=0x5e1c78c, paintInfo=@0xbfffb0fc, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderReplaced.cpp:146
#27 0x015c70b8 in WebCore::RenderImage::paint (this=0x5e1c78c, paintInfo=@0xbfffb0fc, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderImage.cpp:297
#28 0x01541d1d in WebCore::InlineBox::paint (this=0x5e1d62c, paintInfo=@0xbfffb164, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/InlineBox.cpp:184
#29 0x01544545 in WebCore::InlineFlowBox::paint (this=0x5e172dc, paintInfo=@0xbfffb224, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/InlineFlowBox.cpp:749
#30 0x0166cd3a in WebCore::RootInlineBox::paint (this=0x5e172dc, paintInfo=@0xbfffb224, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RootInlineBox.cpp:178
#31 0x015f3e31 in WebCore::RenderLineBoxList::paint (this=0x5e1c088, renderer=0x5e1c01c, paintInfo=@0xbfffb420, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderLineBoxList.cpp:256
#32 0x015690a3 in WebCore::RenderBlock::paintContents (this=0x5e1c01c, paintInfo=@0xbfffb420, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2224
#33 0x01569891 in WebCore::RenderBlock::paintObject (this=0x5e1c01c, paintInfo=@0xbfffb420, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2334
#34 0x01569cff in WebCore::RenderBlock::paint (this=0x5e1c01c, paintInfo=@0xbfffb420, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2115
#35 0x01568ebd in WebCore::RenderBlock::paintChildren (this=0x5e1c90c, paintInfo=@0xbfffb5b0, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2267
#36 0x015690c5 in WebCore::RenderBlock::paintContents (this=0x5e1c90c, paintInfo=@0xbfffb5b0, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2226
#37 0x01569891 in WebCore::RenderBlock::paintObject (this=0x5e1c90c, paintInfo=@0xbfffb5b0, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2334
#38 0x01569cff in WebCore::RenderBlock::paint (this=0x5e1c90c, paintInfo=@0xbfffb5b0, tx=8, ty=8) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2115
#39 0x01568ebd in WebCore::RenderBlock::paintChildren (this=0x5e1a23c, paintInfo=@0xbfffb934, tx=0, ty=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2267
#40 0x015690c5 in WebCore::RenderBlock::paintContents (this=0x5e1a23c, paintInfo=@0xbfffb934, tx=0, ty=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2226
#41 0x01569891 in WebCore::RenderBlock::paintObject (this=0x5e1a23c, paintInfo=@0xbfffb934, tx=0, ty=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2334
#42 0x01569cff in WebCore::RenderBlock::paint (this=0x5e1a23c, paintInfo=@0xbfffb934, tx=0, ty=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderBlock.cpp:2115
#43 0x015d8fe4 in WebCore::RenderLayer::paintLayer (this=0x5e1b3dc, rootLayer=0x5e0fbec, p=0xbfffbf48, paintDirtyRect=@0xbfffbea0, paintBehavior=0, paintingRoot=0x0, overlapTestRequests=0xbfffbd48, paintFlags=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderLayer.cpp:2489
#44 0x015d9aad in WebCore::RenderLayer::paintList (this=0x5e0fbec, list=0x5c00130, rootLayer=0x5e0fbec, p=0xbfffbf48, paintDirtyRect=@0xbfffbea0, paintBehavior=0, paintingRoot=0x0, overlapTestRequests=0xbfffbd48, paintFlags=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderLayer.cpp:2542
#45 0x015d91b7 in WebCore::RenderLayer::paintLayer (this=0x5e0fbec, rootLayer=0x5e0fbec, p=0xbfffbf48, paintDirtyRect=@0xbfffbea0, paintBehavior=0, paintingRoot=0x0, overlapTestRequests=0xbfffbd48, paintFlags=0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderLayer.cpp:2510
#46 0x015d9b9b in WebCore::RenderLayer::paint (this=0x5e0fbec, p=0xbfffbf48, damageRect=@0xbfffbea0, paintBehavior=0, paintingRoot=0x0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../rendering/RenderLayer.cpp:2295
#47 0x014dfed5 in WebCore::FrameView::paintContents (this=0x7009c00, p=0xbfffbf48, rect=@0xbfffbea0) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../page/FrameView.cpp:2122
#48 0x01099cf5 in WebCore::ScrollView::paint (this=0x7009c00, context=0xbfffbf48, rect=@0xbfffbf08) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebCore/WebCore.gyp/../platform/ScrollView.cpp:851
#49 0x00aba103 in WebKit::WebFrameImpl::paintWithContext (this=0x5c11670, gc=@0xbfffbf48, rect=@0xbfffc16c) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp:1891
#50 0x00aba1d4 in WebKit::WebFrameImpl::paint (this=0x5c11670, canvas=0x15815bc0, rect=@0xbfffc16c) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebKit/chromium/src/WebFrameImpl.cpp:1913
#51 0x00af5084 in WebKit::WebViewImpl::paint (this=0x5c101f0, canvas=0x15815bc0, rect=@0xbfffc16c) at /Users/mihaip/Developer/source/chromium1/src/third_party/WebKit/WebKit/chromium/src/WebViewImpl.cpp:1027
#52 0x000211d8 in WebWidgetHost::PaintRect (this=0x5c0f220, rect=@0xbfffc284) at /Users/mihaip/Developer/source/chromium1/src/webkit/tools/test_shell/mac/webwidget_host.mm:281
#53 0x00021ac7 in WebWidgetHost::Paint (this=0x5c0f220) at /Users/mihaip/Developer/source/chromium1/src/webkit/tools/test_shell/mac/webwidget_host.mm:207
#54 0x0001ed57 in -[TestShellWebView drawRect:] (self=0x5c0f280, _cmd=0x92c589e8, rect={origin = {x = 0, y = 0}, size = {width = 800, height = 600}}) at /Users/mihaip/Developer/source/chromium1/src/webkit/tools/test_shell/mac/test_shell_webview.mm:59
#55 0x92598a36 in -[NSView _drawRect:clip:] ()
#56 0x925976d4 in -[NSView _recursiveDisplayAllDirtyWithLockFocus:visRect:] ()
#57 0x92595bf3 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#58 0x92596b68 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#59 0x92596b68 in -[NSView _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#60 0x92595767 in -[NSThemeFrame _recursiveDisplayRectIfNeededIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:topView:] ()
#61 0x925920ae in -[NSView _displayRectIgnoringOpacity:isVisibleRect:rectIsVisibleRectForView:] ()
#62 0x924f2d3f in -[NSView displayIfNeeded] ()
#63 0x924bc050 in -[NSWindow displayIfNeeded] ()
#64 0x924ed572 in _handleWindowNeedsDisplay ()
#65 0x97968968 in __NSFireTimer ()
#66 0x9976870b in __CFRunLoopRun ()
#67 0x99766094 in CFRunLoopRunSpecific ()
#68 0x99765ec1 in CFRunLoopRunInMode ()
#69 0x906d3f9c in RunCurrentEventLoopInMode ()
#70 0x906d3d51 in ReceiveNextEventCommon ()
#71 0x906d3bd6 in BlockUntilNextEventMatchingListInMode ()
#72 0x924c3a89 in _DPSNextEvent ()
#73 0x924c32ca in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#74 0x9248555b in -[NSApplication run] ()
#75 0x001a66ba in base::MessagePumpNSApplication::DoRun (this=0x8f08a00, delegate=0xbffff4d0) at /Users/mihaip/Developer/source/chromium1/src/base/message_pump_mac.mm:683
#76 0x001a6ca9 in base::MessagePumpCFRunLoopBase::Run (this=0x8f08a00, delegate=0xbffff4d0) at /Users/mihaip/Developer/source/chromium1/src/base/message_pump_mac.mm:213
#77 0x0013893b in MessageLoop::RunInternal (this=0xbffff4d0) at /Users/mihaip/Developer/source/chromium1/src/base/message_loop.cc:266
#78 0x00138955 in MessageLoop::RunHandler (this=0xbffff4d0) at /Users/mihaip/Developer/source/chromium1/src/base/message_loop.cc:238
#79 0x001389b9 in MessageLoop::Run (this=0xbffff4d0) at /Users/mihaip/Developer/source/chromium1/src/base/message_loop.cc:216
#80 0x0001d3f1 in main (argc=1, argv=0xbffff684) at /Users/mihaip/Developer/source/chromium1/src/webkit/tools/test_shell/test_shell_main.cc:442

In theory, based on http://trac.webkit.org/changeset/52833 and http://trac.webkit.org/changeset/52102 we should be rejecting invalid sizes and malloc failures, but I'm not seeing that here.

[Adam Barth]

Hum...  Yeah.  Thanks for investigating this bug.  I should be able to drive this to completion from here.  (Of course, you're welcome to continue working on it if you're finding it interesting.)

[Mihai Parparita]

Retitling bug for clarity, adding Peter since he might have ideas about where the image decoders go wrong. Peter: see comment 10 and onward for background.

[Peter Kasting]

For reference, see the difference between RGBA32Buffer::setSize() implementations on Skia (where it checks m_bitmap.allocPixels() and returns false if it fails) versus in the "base" class (where there's a comment saying "this has no way to check for allocation failure...").

[Mihai Parparita]

Do we ever impose a maximum image size (i.e. should we be trying to display a 14489x34615 image in the first place)?

[Peter Kasting]

(In reply to comment #16)
> Do we ever impose a maximum image size (i.e. should we be trying to display a 14489x34615 image in the first place)?

Yes, see ImageDecoder::isOverSize().

[Mihai Parparita]

Created attachment 73810 [details]
Patch

[Adam Barth]

Comment on attachment 73810 [details]
Patch

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

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:63
> +    int backingStoreSize = newWidth * newHeight * sizeof(PixelData);

Is "int" the right type here?  I would have expected size_t.  Do we need to check for overflow?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:67
> +    if (!backingStoreRef)
> +        return false;
> +    m_backingStore.adoptCF(backingStoreRef);

Are these the right failure semantics?  I guess setSize is called exactly once.  Maybe we should ASSERT something about the state of m_backingStore at the top of the function (e.g., that it's NULL).

[Peter Kasting]

(In reply to comment #19)
> (From update of attachment 73810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73810&action=review
> 
> > WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:63
> > +    int backingStoreSize = newWidth * newHeight * sizeof(PixelData);
> 
> Is "int" the right type here?  I would have expected size_t.  Do we need to check for overflow?

Adam is correct that this should be a size_t.  As for checking for overflow, I forget off the top of my head whether it's possible to get arbitrary dimensions here without tripping the master isOverSize() check.

> > WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:67
> > +    if (!backingStoreRef)
> > +        return false;
> > +    m_backingStore.adoptCF(backingStoreRef);
> 
> Are these the right failure semantics?  I guess setSize is called exactly once.  Maybe we should ASSERT something about the state of m_backingStore at the top of the function (e.g., that it's NULL).

These are the right failure semantics.  ASSERT()ing would be fine to add or leave off as desired -- the other platforms don't bother to ASSERT this and we have sufficient checks to prevent it in the callers.

[Mihai Parparita]

Created attachment 73859 [details]
Patch

[Mihai Parparita]

(In reply to comment #20)
> > Is "int" the right type here?  I would have expected size_t.  Do we need to check for overflow?
> 
> Adam is correct that this should be a size_t.  As for checking for overflow, I forget off the top of my head whether it's possible to get arbitrary dimensions here without tripping the master isOverSize() check.

Changed to size_t; I'm quite sure we only get here if we pass the isOverSize check.

> > > WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:67
> > > +    if (!backingStoreRef)
> > > +        return false;
> > > +    m_backingStore.adoptCF(backingStoreRef);
> > 
> > Are these the right failure semantics?  I guess setSize is called exactly once.  Maybe we should ASSERT something about the state of m_backingStore at the top of the function (e.g., that it's NULL).
> 
> These are the right failure semantics.  ASSERT()ing would be fine to add or leave off as desired -- the other platforms don't bother to ASSERT this and we have sufficient checks to prevent it in the callers.

Added the ASSERT, just to be safe.

[WebKit Commit Bot]

Comment on attachment 73859 [details]
Patch

Rejecting patch 73859 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 73859]" exit_code: 2
Last 500 characters of output:
ayoutTests/platform/chromium/test_expectations.txt
Hunk #1 succeeded at 3140 (offset -3 lines).
Hunk #2 FAILED at 3204.
1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6089003

[Mihai Parparita]

Created attachment 73953 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 73953 [details]
Patch for landing

Clearing flags on attachment: 73953

Committed r72066: <http://trac.webkit.org/changeset/72066>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 73859 [details]
Patch

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

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:64
> +    size_t backingStoreSize = newWidth * newHeight * sizeof(PixelData);

This code needs a check for overflow. Doing multiplication like this without a check for overflow can lead to security problems.

[Mihai Parparita]

(In reply to comment #27)
> This code needs a check for overflow. Doing multiplication like this without a check for overflow can lead to security problems.

ImageDecoder::isOveSize will reject image sizes that can lead to overflow issues, so we would never get here in that case. Still want me to add a check, just to be safe?

[Peter Kasting]

(In reply to comment #28)
> (In reply to comment #27)
> > This code needs a check for overflow. Doing multiplication like this without a check for overflow can lead to security problems.
> 
> ImageDecoder::isOveSize will reject image sizes that can lead to overflow issues, so we would never get here in that case.

In fact, that's its explicit purpose in life.  It is there to be an overflow check for all future "w * h * bpp" calculations because not all of them are explicitly under our control.  (For example, Skia does calculations like this internally in some cases.)

I would prefer not to have code that implies that overflow should be possible here.  A comment about isOverSize() checking this would be fine.  If you're nervous, you can couple it with an ASSERT().  An actual conditional that returns if there's overflow would be misleading, though.