[GPU Process] Implement the ClipToDrawingCommands item by using begin and end markers

[Said Abou-Hallawa]

Instead of encoding and decoding the clipping commands as a separate DisplayList, the recorder will insert a "begin" and "end" markers before and after the clipping commands. When replaying the "begin" mark, the relpayer will create a mask ImageBuffer and force using its context for the following items . When replaying the "end" mark, it will clip the original context to the mask ImageBuffer.

[Said Abou-Hallawa]

Created attachment 421096 [details]
Patch

[Tim Horton]

Comment on attachment 421096 [details]
Patch

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

This does not seem outrageous

> Source/WebCore/ChangeLog:12
> +        When replaying the "begin" mark, the relpayer will create a mask ImageBuffer

sp. `relpayer`

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160
> +        if (!m_maskImageBuffer)
> +            return { StopReplayReason::InvalidItem, WTF::nullopt };

Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this?

Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context?

[Simon Fraser (smfr)]

Comment on attachment 421096 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> +    if (item.is<EndClipToDrawingCommands>()) {

What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?

[Said Abou-Hallawa]

Created attachment 421216 [details]
Patch

[Said Abou-Hallawa]

Created attachment 421218 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 421096 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        When replaying the "begin" mark, the relpayer will create a mask ImageBuffer
> 
> sp. `relpayer`

Fixed.

>> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160
>> +            return { StopReplayReason::InvalidItem, WTF::nullopt };
> 
> Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this?
> 
> Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context?

I added StopReplayReason::OutOfMemory to handle this case.

I added also the following check in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists()

MESSAGE_CHECK_WITH_RETURN_VALUE(result.reasonForStopping != DisplayList::StopReplayReason::OutOfMemory, nullptr, "Cound not allocate memory");

This will terminate the WebProcess:

#define MESSAGE_CHECK_WITH_RETURN_VALUE(assertion, returnValue, message) do { \
    if (UNLIKELY(!(assertion))) { \
        TERMINATE_WEB_PROCESS_WITH_MESSAGE(message); \
        return (returnValue); \
    } \
} while (0)

>> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
>> +    if (item.is<EndClipToDrawingCommands>()) {
> 
> What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?

clipToDrawingCommands() can be called internally. Currently it is only called form one place which is CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to have nested calls from clipToDrawingCommands() because the drawing commands create a mask image. Creating the mask should not involve drawing with gradient. And the drawing happens only in the lambda function which we control.

[Simon Fraser (smfr)]

(In reply to Said Abou-Hallawa from comment #6)

> >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> >> +    if (item.is<EndClipToDrawingCommands>()) {
> > 
> > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?
> 
> clipToDrawingCommands() can be called internally. Currently it is only
> called form one place which is
> CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to
> have nested calls from clipToDrawingCommands() because the drawing commands
> create a mask image. Creating the mask should not involve drawing with
> gradient. And the drawing happens only in the lambda function which we
> control.

Let me ask it this way. What happens when an IPC fuzzer generates display list IPC with lots of "begin clip to drawing command" but no "end"?

[Said Abou-Hallawa]

(In reply to Simon Fraser (smfr) from comment #7)
> (In reply to Said Abou-Hallawa from comment #6)
> 
> > >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164
> > >> +    if (item.is<EndClipToDrawingCommands>()) {
> > > 
> > > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?
> > 
> > clipToDrawingCommands() can be called internally. Currently it is only
> > called form one place which is
> > CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to
> > have nested calls from clipToDrawingCommands() because the drawing commands
> > create a mask image. Creating the mask should not involve drawing with
> > gradient. And the drawing happens only in the lambda function which we
> > control.
> 
> Let me ask it this way. What happens when an IPC fuzzer generates display
> list IPC with lots of "begin clip to drawing command" but no "end"?

Replayer::applyItem() will terminate the web process once it detects a "begin clip to drawing command" item while m_maskImageBuffer is not null.

    if (item.is<BeginClipToDrawingCommands>()) {
        if (m_maskImageBuffer)
            return { StopReplayReason::InvalidItem, WTF::nullopt };
        ...
        return { WTF::nullopt, WTF::nullopt };
    }

[Simon Fraser (smfr)]

Comment on attachment 421218 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55
> +    BeginClipToDrawingCommands,
> +    EndClipToDrawingCommands,

At some point we should make this more generic (for filters etc).

[Said Abou-Hallawa]

Comment on attachment 421218 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55
>> +    EndClipToDrawingCommands,
> 
> At some point we should make this more generic (for filters etc).

I wish we could encode/decode the DisplayList instead of these two markers. But the current DisplayList implementation makes it a little bit hard unless we do a significant code re-factoring.

[EWS]

Committed r273291: <https://commits.webkit.org/r273291>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421218 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74622997>

[Said Abou-Hallawa]

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