68753 2011-09-24 02:13:54 -0700 [WinCairo] BitmapImage::drawFrameMatchingSourceSize causes access violation if BitmapImage::frameAtIndex() returns NULL 2012-11-24 15:05:48 -0800 1 1 1 Unclassified WebKit WebKit API 528+ (Nightly build) PC Unspecified RESOLVED FIXED P2 Normal --- 1 david.delaune webkit-unassigned aroben bfulgham oldest_to_newest 472745 0 david.delaune 2011-09-24 02:13:54 -0700 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 472747 1 108581 david.delaune 2011-09-24 02:53:48 -0700 Created attachment 108581 Check for zero cairo_surface_t * pointer to avoid null pointer exception 473054 2 108581 aroben 2011-09-26 05:01:16 -0700 Comment on attachment 108581 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; 476636 3 108581 bfulgham 2011-10-01 22:12:17 -0700 Comment on attachment 108581 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? 476637 4 bfulgham 2011-10-01 22:14:00 -0700 Nice catch! (In reply to comment #2) > (From update of attachment 108581 > 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 534932 5 122266 bfulgham 2012-01-12 10:06:30 -0800 Created attachment 122266 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. 535054 6 aroben 2012-01-12 12:22:36 -0800 (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? 553266 7 david.delaune 2012-02-09 10:12:20 -0800 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 560315 8 122266 aroben 2012-02-20 12:14:59 -0800 Comment on attachment 122266 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. 624472 9 david.delaune 2012-05-15 13:23:20 -0700 What automated test is Apple using to test for Bug 61684? 624491 10 122266 simon.fraser 2012-05-15 13:34:22 -0700 Comment on attachment 122266 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. 774891 11 bfulgham 2012-11-24 15:05:48 -0800 Committed r135659: <http://trac.webkit.org/changeset/135659> 108581 2011-09-24 02:53:48 -0700 2012-01-12 10:06:30 -0800 Check for zero cairo_surface_t * pointer to avoid null pointer exception patch.diff text/plain 1873 david.delaune SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk1OTA3KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDktMjQgIERhdmlkIERl bGF1bmUgIDxkYXZpZC5kZWxhdW5lQGdvb2dsZW1haWwuY29tPgorCisgICAgICAgIFtXaW5DYWly b10gQml0bWFwSW1hZ2U6OmRyYXdGcmFtZU1hdGNoaW5nU291cmNlU2l6ZSBjYXVzZXMgYWNjZXNz IHZpb2xhdGlvbiBpZiBCaXRtYXBJbWFnZTo6ZnJhbWVBdEluZGV4KCkgcmV0dXJucyBOVUxMCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02ODc1MworCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyBmdW5j dGlvbmFsaXR5IGFkZGVkIGluIHRoaXMgY2hhbmdlLgorCisgICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3Mvd2luL0ltYWdlQ2Fpcm9XaW4uY3BwOgorICAgICAgICAoV2ViQ29yZTo6Qml0bWFwSW1h Z2U6OmRyYXdGcmFtZU1hdGNoaW5nU291cmNlU2l6ZSk6CisgICAgICAgIEFkZGVkIGNoZWNrIGZv ciB6ZXJvIGNhaXJvX3N1cmZhY2VfdCAqIHBvaW50ZXIgdG8gYXZvaWQgbnVsbCBwb2ludGVyIGV4 Y2VwdGlvbi4KKwogMjAxMS0wOS0yNCAgWW91bmcgSGFuIExlZSAgPGpveWJyb0Bjb21wYW55MTAw Lm5ldD4KIAogICAgICAgICBTVkdBbmltYXRpb24gZG9lcyBub3Qgc3VwcG9ydCAndmFsdWVzJyBm b3IgZnJvbS10byBhbmltYXRpb25zCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy93aW4vSW1hZ2VDYWlyb1dpbi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3Mvd2luL0ltYWdlQ2Fpcm9XaW4uY3BwCShyZXZpc2lvbiA5NTkwNikK KysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3dpbi9JbWFnZUNhaXJvV2luLmNw cAkod29ya2luZyBjb3B5KQpAQCAtOTcsNyArOTcsNyBAQAogICAgIHNpemVfdCBmcmFtZXMgPSBm cmFtZUNvdW50KCk7CiAgICAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCBmcmFtZXM7ICsraSkgewog ICAgICAgICBjYWlyb19zdXJmYWNlX3QqIGltYWdlID0gZnJhbWVBdEluZGV4KGkpOwotICAgICAg ICBpZiAoY2Fpcm9faW1hZ2Vfc3VyZmFjZV9nZXRfaGVpZ2h0KGltYWdlKSA9PSBzdGF0aWNfY2Fz dDxzaXplX3Q+KHNyY1NpemUuaGVpZ2h0KCkpICYmIGNhaXJvX2ltYWdlX3N1cmZhY2VfZ2V0X3dp ZHRoKGltYWdlKSA9PSBzdGF0aWNfY2FzdDxzaXplX3Q+KHNyY1NpemUud2lkdGgoKSkpIHsKKyAg ICAgICAgaWYgKGltYWdlICYmIGNhaXJvX2ltYWdlX3N1cmZhY2VfZ2V0X2hlaWdodChpbWFnZSkg PT0gc3RhdGljX2Nhc3Q8c2l6ZV90PihzcmNTaXplLmhlaWdodCgpKSAmJiBjYWlyb19pbWFnZV9z dXJmYWNlX2dldF93aWR0aChpbWFnZSkgPT0gc3RhdGljX2Nhc3Q8c2l6ZV90PihzcmNTaXplLndp ZHRoKCkpKSB7CiAgICAgICAgICAgICBzaXplX3QgY3VycmVudEZyYW1lID0gbV9jdXJyZW50RnJh bWU7CiAgICAgICAgICAgICBtX2N1cnJlbnRGcmFtZSA9IGk7CiAgICAgICAgICAgICBkcmF3KGN0 eHQsIGRzdFJlY3QsIEZsb2F0UmVjdCgwLjBmLCAwLjBmLCBzcmNTaXplLndpZHRoKCksIHNyY1Np emUuaGVpZ2h0KCkpLCBDb2xvclNwYWNlRGV2aWNlUkdCLCBjb21wb3NpdGVPcCk7Cg== 122266 2012-01-12 10:06:30 -0800 2012-05-15 13:34:21 -0700 Avoid passing a null pointer to the Cairo library. bitmapImage.patch text/plain 1691 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwNDgzMikKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDEyLTAxLTEyICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1Ad2Via2l0Lm9yZz4KKworICAgICAgICBbV2luQ2Fpcm9dIEF2b2lk IGFjY2VzcyB2aW9sYXRpb24gd2hlbiBmcmFtZSBpcyBOVUxMLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Njg3NTMKKworICAgICAgICBCaXRtYXBJbWFn ZTo6ZHJhd0ZyYW1lTWF0Y2hpbmdTb3VyY2VTaXplIGNhdXNlcyBhbiBhY2Nlc3MgdmlvbGF0aW9u CisgICAgICAgIGlmIEJpdG1hcEltYWdlOjpmcmFtZUF0SW5kZXggcmV0dXJucyBOVUxMLiAoRm91 bmQgYnkgRGF2aWQgRGVsYXVuZSkuCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiAoT09QUyEpCisKKyAgICAgICAgKiBwbGF0Zm9y bS9ncmFwaGljcy93aW4vSW1hZ2VDYWlyb1dpbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpCaXRt YXBJbWFnZTo6ZHJhd0ZyYW1lTWF0Y2hpbmdTb3VyY2VTaXplKTogQ2hlY2sgZm9yIG51bGwKKyAg ICAgICAgY2Fpcm9fc3VyZmFjZV90IHBvaW50ZXIgYW5kIGF2b2lkIGRlcmVmZXJlbmNpbmcuCisK IDIwMTItMDEtMTIgIFBhdmVsIFBvZGl2aWxvdiAgPHBvZGl2aWxvdkBjaHJvbWl1bS5vcmc+CiAK ICAgICAgICAgV2ViIEluc3BlY3RvcjogW0pTQ10gLy9AIHNvdXJjZVVSTCBpcyBub3QgcmVzcGVj dGVkLgpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvd2luL0ltYWdlQ2Fp cm9XaW4uY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L3dpbi9JbWFnZUNhaXJvV2luLmNwcAkocmV2aXNpb24gMTA0NzMxKQorKysgU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3Mvd2luL0ltYWdlQ2Fpcm9XaW4uY3BwCSh3b3JraW5nIGNvcHkp CkBAIC05Nyw2ICs5Nyw5IEBAIHZvaWQgQml0bWFwSW1hZ2U6OmRyYXdGcmFtZU1hdGNoaW5nU291 cmMKICAgICBzaXplX3QgZnJhbWVzID0gZnJhbWVDb3VudCgpOwogICAgIGZvciAoc2l6ZV90IGkg PSAwOyBpIDwgZnJhbWVzOyArK2kpIHsKICAgICAgICAgY2Fpcm9fc3VyZmFjZV90KiBpbWFnZSA9 IGZyYW1lQXRJbmRleChpKTsKKyAgICAgICAgaWYgKCFpbWFnZSkKKyAgICAgICAgICAgIGNvbnRp bnVlOworCiAgICAgICAgIGlmIChjYWlyb19pbWFnZV9zdXJmYWNlX2dldF9oZWlnaHQoaW1hZ2Up ID09IHN0YXRpY19jYXN0PHNpemVfdD4oc3JjU2l6ZS5oZWlnaHQoKSkgJiYgY2Fpcm9faW1hZ2Vf c3VyZmFjZV9nZXRfd2lkdGgoaW1hZ2UpID09IHN0YXRpY19jYXN0PHNpemVfdD4oc3JjU2l6ZS53 aWR0aCgpKSkgewogICAgICAgICAgICAgc2l6ZV90IGN1cnJlbnRGcmFtZSA9IG1fY3VycmVudEZy YW1lOwogICAgICAgICAgICAgbV9jdXJyZW50RnJhbWUgPSBpOwo=