[WinCairo] BitmapImage::drawFrameMatchingSourceSize causes access violation if BitmapImage::frameAtIndex() returns NULL

[David Delaune]

Hi,

I encountered an access violation in one of my unit tests at BitmapImage::drawFrameMatchingSourceSize. Below is the call stack:

>	WebKit.dll!WebCore::BitmapImage::drawFrameMatchingSourceSize(WebCore::GraphicsContext * ctxt=0x0023edb0, const WebCore::FloatRect & dstRect={...}, const WebCore::IntSize & srcSize={...}, WebCore::ColorSpace styleColorSpace=ColorSpaceDeviceRGB, WebCore::CompositeOperator compositeOp=CompositeCopy)  Line 100 + 0x10 bytes
 	WebKit.dll!WebCore::BitmapImage::getHBITMAPOfSize(HBITMAP__ * bmp=0xee0510de, tagSIZE * size=0x0023ef74)  Line 90
 	WebKit.dll!WebIconDatabase::iconForURL(wchar_t * url=0x77f34618, tagSIZE * size=0x0023ef74, int __formal=1, unsigned int * bitmap=0x0023ef84)  Line 179 + 0xb bytes

After an investigation it appears that in some circumstances frameAtIndex() can return a NULL NativeImagePtr pointer. The WinCairo branch calls cairo_image_surface_get_height and cairo_image_surface_get_width and these functions will throw an exception if passed a NULL pointer. I noticed that the ImageCGWin.cpp implementation is nearly identical and based the comments in the Bug 61684 patch I wonder if it should also be reviewed.

Best Wishes,
-David Delaune

[David Delaune]

Created attachment 108581 [details]
Check for zero cairo_surface_t * pointer to avoid null pointer exception

[Adam Roben (:aroben)]

Comment on attachment 108581 [details]
Check for zero cairo_surface_t * pointer to avoid null pointer exception

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

Is it possible to add a regression test for this? Perhaps via TestWebKitAPI?

> Source/WebCore/platform/graphics/win/ImageCairoWin.cpp:100
>      for (size_t i = 0; i < frames; ++i) {
>          cairo_surface_t* image = frameAtIndex(i);
> -        if (cairo_image_surface_get_height(image) == static_cast<size_t>(srcSize.height()) && cairo_image_surface_get_width(image) == static_cast<size_t>(srcSize.width())) {
> +        if (image && cairo_image_surface_get_height(image) == static_cast<size_t>(srcSize.height()) && cairo_image_surface_get_width(image) == static_cast<size_t>(srcSize.width())) {

I think this would be a little clearer using an early continue:

cairo_surface_t* image = frameAtIndex(i);
if (!image)
    continue;

[Brent Fulgham]

Comment on attachment 108581 [details]
Check for zero cairo_surface_t * pointer to avoid null pointer exception

You mentioned you found this during unit test. Is this a test we could integrate into WebKit's own test infrastructure?

[Brent Fulgham]

Nice catch!

(In reply to comment #2)
> (From update of attachment 108581 [details] 
> I think this would be a little clearer using an early continue:
> 
> cairo_surface_t* image = frameAtIndex(i);
> if (!image)
>     continue;

I agree. Please make this change, and let me know about the test case.

Thanks!

-Brent

[Brent Fulgham]

Created attachment 122266 [details]
Avoid passing a null pointer to the Cairo library.

Since David never responded to my comments, I've revised the source change per Adam's comment.  No test case provided since exercising this logic requires calling WebKit API calls that are not used in the existing test infrastructure.

[Adam Roben (:aroben)]

(In reply to comment #5)
> Created an attachment (id=122266) [details]
> Avoid passing a null pointer to the Cairo library.
> 
> Since David never responded to my comments, I've revised the source change per Adam's comment.  No test case provided since exercising this logic requires calling WebKit API calls that are not used in the existing test infrastructure.

Brent, did you consider add a test to TestWebKitAPI for this?

[David Delaune]

Hi,

I have not tried this... I am just guessing that the manual test located at:

\WebKit\ManualTests\resources\favicon-loads-for-local-files.html

could potentially be modified to demonstrate this bug on WinCairo.

You could probably generate a corrupt icon by doing something like:

head -c 1024 /dev/urandom > corrupt_favicon.ico

Best Wishes,
-David Delaune

[Adam Roben (:aroben)]

Comment on attachment 122266 [details]
Avoid passing a null pointer to the Cairo library.

Discussion in this bug makes it sound like it should be possible to write an automated test. So let's wait until we have one.

[David Delaune]

What automated test is Apple using to test for Bug 61684?

[Simon Fraser (smfr)]

Comment on attachment 122266 [details]
Avoid passing a null pointer to the Cairo library.

It would be nice to have a test for this if you can make one (with a corrupted image file or something). I don't think it's necessary though.

[Brent Fulgham]

Committed r135659: <http://trac.webkit.org/changeset/135659>