Bug 64802 – Custom cursors cause the WebProcess to crash

• Component: WebKit2
• Status: RESOLVED FIXED

[Amruth Raj]

With https://bugs.webkit.org/show_bug.cgi?id=64321 there can be a custom Cursor object with an empty Image.
Encoding them would lead to a crash since a ShareableBitmap of size 0 would be requested. This returns a NULL and causes a crash.

#0  0x00007f141d5ee05a in WebCore::IntSize::width (this=0x8) at ../../Source/WebCore/platform/graphics/IntSize.h:67
#1  0x00007f141d6051ca in WebKit::ShareableBitmap::createCairoSurface (this=0x0) at ../../Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:69
#2  0x00007f141d604ec9 in WebKit::ShareableBitmap::createGraphicsContext (this=0x0) at ../../Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:43
#3  0x00007f141d622f51 in CoreIPC::encodeImage (encoder=0x23a8650, image=0x23c3c50) at ../../Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:294

[Amruth Raj]

Created attachment 101340 [details]
Handle the case of a custom cursor with a null image

[Martin Robinson]

Nate, what are your thoughts here?

[Nate Chapin]

(In reply to comment #2)
> Nate, what are your thoughts here?

It looks fine to me.  I think this can be fixed in WebCore to ensure we don't send custom cursors with null Image*s to the embedder, but the patch will be much uglier than this one due to the #if(LAZY_NATIVE_CURSOR) flag.

[Darin Adler]

Comment on attachment 101340 [details]
Handle the case of a custom cursor with a null image

This is wrong. It will encode something that the decoder will fail to decode. We need to encode something that indicates there is no image following, perhaps a boolean.

[Nate Chapin]

(In reply to comment #4)
> (From update of attachment 101340 [details])
> This is wrong. It will encode something that the decoder will fail to decode. We need to encode something that indicates there is no image following, perhaps a boolean.

Darin, would prefer this just fixed in WebCore to ensure we don't send null images?  I'd be happy to write that patch, but it wouldn't get fixed today.

[Darin Adler]

(In reply to comment #5)
> Darin, would you prefer this just fixed in WebCore to ensure we don't send null images?  I'd be happy to write that patch, but it wouldn't get fixed today.

I think the right question is what behavior we want when such a cursor is specified. Once we know what behavior we want, then we can figure out how to get that behavior. We could start by figuring out what behavior we have here in WebKit1. I’m thinking that a custom cursor with an empty image should probably result in an invisible cursor. Is that right?

As far as implementation is concerned, it’s OK with me to fix the encoding machinery so it can successfully send an empty custom cursor across the process boundary. I object to the incorrect patch, not the very notion of what the patch is doing. The patch is wrong because if we encode one of these, the decoder will then try to decode fields that aren’t present and consume additional bytes and thus be unable to decode the rest of the stream. That’s a relatively simple mechanical thing to fix.

[Amruth Raj]

(In reply to comment #6)
> (In reply to comment #5)
> > Darin, would you prefer this just fixed in WebCore to ensure we don't send null images?  I'd be happy to write that patch, but it wouldn't get fixed today.
> 
> I think the right question is what behavior we want when such a cursor is specified. Once we know what behavior we want, then we can figure out how to get that behavior. We could start by figuring out what behavior we have here in WebKit1. I’m thinking that a custom cursor with an empty image should probably result in an invisible cursor. Is that right?
In WebKit1, GTK handles this case. In case a NULL image is sent to gdk_window_set_cursor, it uses cursor of the parent window to display.
> 
> As far as implementation is concerned, it’s OK with me to fix the encoding machinery so it can successfully send an empty custom cursor across the process boundary. I object to the incorrect patch, not the very notion of what the patch is doing. The patch is wrong because if we encode one of these, the decoder will then try to decode fields that aren’t present and consume additional bytes and thus be unable to decode the rest of the stream. That’s a relatively simple mechanical thing to fix.
I will attach a patch shortly addressing this.

[Amruth Raj]

Created attachment 101474 [details]
Indicate the presence of a null image while encoding

[Amruth Raj]

It looks like individual ports needs to additionally handle this. QT port still crashes with my patch.

[Darin Adler]

*** Bug 65420 has been marked as a duplicate of this bug. ***

[Oleg Romashin (:romaxa)]

Qt port crash with this patch:
#0  0xb2e4d8c4 in operator! (this=0x0) at ../../include/QtCore/../../src/corelib/tools/qshareddata.h:198
#1  QPixmap::isNull (this=0x0) at image/qpixmap.cpp:545
#2  0xb2e4d90a in QPixmap::toImage (this=0x0) at image/qpixmap.cpp:484
#3  0xb2d78784 in QCursor::QCursor (this=0x82f1870, pixmap=..., hotX=0, hotY=0) at kernel/qcursor.cpp:313
#4  0xb5d22e2e in WebCore::createCustomCursor (image=0x82f1a08, hotSpot=...)
    at ../../../Source/WebCore/platform/qt/CursorQt.cpp:79
#5  0xb5d238de in WebCore::Cursor::ensurePlatformCursor (this=0xbfc484bc)
    at ../../../Source/WebCore/platform/qt/CursorQt.cpp:199
#6  0xb5d1ccf3 in WebCore::Cursor::platformCursor (this=0xbfc484bc)
    at ../../../Source/WebCore/platform/Cursor.cpp:263
#7  0xb54ae51f in QtWebPageProxy::setCursor (this=0x8283ec0, cursor=...)
    at ../../../Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:239
#8  0xb546f945 in WebKit::WebPageProxy::setCursor (this=0xaf702068, cursor=...)
    at ../../../Source/WebKit2/UIProcess/WebPageProxy.cpp:2666
#9  0xb5505535 in CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&), WebCore::Cursor> (args=..., object=0xaf702068, function=
    (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:19
#10 0xb55010e8 in CoreIPC::handleMessage<Messages::WebPageProxy::SetCursor, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&)> (argumentDecoder=0xaf7034c0, object=0xaf702068, function=
    (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:277
#11 0xb54fee14 in WebKit::WebPageProxy::didReceiveWebPageProxyMessage (this=0xaf702068, messageID=...,

[Amruth Raj]

Like I mentioned in comment #9, QT port needs to handle this case at the UI process. In case a null image is passed to GTK, it displays the cursor of parent window.
(In reply to comment #11)
> Qt port crash with this patch:
> #0  0xb2e4d8c4 in operator! (this=0x0) at ../../include/QtCore/../../src/corelib/tools/qshareddata.h:198
> #1  QPixmap::isNull (this=0x0) at image/qpixmap.cpp:545
> #2  0xb2e4d90a in QPixmap::toImage (this=0x0) at image/qpixmap.cpp:484
> #3  0xb2d78784 in QCursor::QCursor (this=0x82f1870, pixmap=..., hotX=0, hotY=0) at kernel/qcursor.cpp:313
> #4  0xb5d22e2e in WebCore::createCustomCursor (image=0x82f1a08, hotSpot=...)
>     at ../../../Source/WebCore/platform/qt/CursorQt.cpp:79
> #5  0xb5d238de in WebCore::Cursor::ensurePlatformCursor (this=0xbfc484bc)
>     at ../../../Source/WebCore/platform/qt/CursorQt.cpp:199
> #6  0xb5d1ccf3 in WebCore::Cursor::platformCursor (this=0xbfc484bc)
>     at ../../../Source/WebCore/platform/Cursor.cpp:263
> #7  0xb54ae51f in QtWebPageProxy::setCursor (this=0x8283ec0, cursor=...)
>     at ../../../Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:239
> #8  0xb546f945 in WebKit::WebPageProxy::setCursor (this=0xaf702068, cursor=...)
>     at ../../../Source/WebKit2/UIProcess/WebPageProxy.cpp:2666
> #9  0xb5505535 in CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&), WebCore::Cursor> (args=..., object=0xaf702068, function=
>     (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:19
> #10 0xb55010e8 in CoreIPC::handleMessage<Messages::WebPageProxy::SetCursor, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebCore::Cursor const&)> (argumentDecoder=0xaf7034c0, object=0xaf702068, function=
>     (void (WebKit::WebPageProxy::*)(WebKit::WebPageProxy *, const WebCore::Cursor &)) 0xb546f920 <WebKit::WebPageProxy::setCursor(WebCore::Cursor const&)>) at ../../../Source/WebKit2/Platform/CoreIPC/HandleMessage.h:277
> #11 0xb54fee14 in WebKit::WebPageProxy::didReceiveWebPageProxyMessage (this=0xaf702068, messageID=...,

[Oleg Romashin (:romaxa)]

I would suggest to add image null check:
 static QCursor* createCustomCursor(Image* image, const IntPoint& hotSpot)
 {
+    if (image->isNull())
+        return NULL;

[WebKit Review Bot]

Comment on attachment 101474 [details]
Indicate the presence of a null image while encoding

Clearing flags on attachment: 101474

Committed r92108: <http://trac.webkit.org/changeset/92108>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This seems to have been rolled out in r92113.

[Amruth Raj]

I think the problem here is that nullImage() function is not part of the export's file. I will add this and put up a new patch.
(In reply to comment #16)
> This seems to have been rolled out in r92113.

[Amruth Raj]

Created attachment 102631 [details]
Add Image::nullImage() into exports to fix compilation on SL

[Amruth Raj]

Created attachment 102635 [details]
Add Image::nullImage() into exports to fix compilation on SL

[Martin Robinson]

Comment on attachment 102635 [details]
Add Image::nullImage() into exports to fix compilation on SL

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

> Source/WebKit2/ChangeLog:11
> +        * Shared/WebCoreArgumentCoders.cpp:
> +        (CoreIPC::::encode):
> +        (CoreIPC::::decode):
> +

In your previous patch you had filled these in. Why remove them?

[Amruth Raj]

Created attachment 102637 [details]
Add comments in ChangeLog

[Amruth Raj]

(In reply to comment #20)
> (From update of attachment 102635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102635&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        * Shared/WebCoreArgumentCoders.cpp:
> > +        (CoreIPC::::encode):
> > +        (CoreIPC::::decode):
> > +
> 
> In your previous patch you had filled these in. Why remove them?
Done. Added these back.

[Martin Robinson]

Comment on attachment 102637 [details]
Add comments in ChangeLog

Seems reasonable.

[WebKit Review Bot]

Comment on attachment 102637 [details]
Add comments in ChangeLog

Clearing flags on attachment: 102637

Committed r92183: <http://trac.webkit.org/changeset/92183>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 65029 has been marked as a duplicate of this bug. ***