[webkit-reviews] review granted: [Bug 226159] prevent GPU process from rendering sbix glyphs : [Attachment 429491] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 23 18:41:34 PDT 2021


Darin Adler <darin at apple.com> has granted Cameron McCormack (:heycam)
<heycam at apple.com>'s request for review:
Bug 226159: prevent GPU process from rendering sbix glyphs
https://bugs.webkit.org/show_bug.cgi?id=226159

Attachment 429491: Patch

https://bugs.webkit.org/attachment.cgi?id=429491&action=review




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 429491
  --> https://bugs.webkit.org/attachment.cgi?id=429491
Patch

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

> Source/WebCore/ChangeLog:13
> +	   item. This patch checks for such glyphs (like we already do for SVG
> +	   glyphs) and returns early if they're found.

Maybe some day we can find a way to turn this into an allow list style
mechanism rather than a deny list one. It’s a better security design to have a
list of what is safe to use rather than a list of reasons glyphs are unsafe.

> Source/WebCore/ChangeLog:24
> +	   assertion to avoid unnecessarily crashing teh GPUP, and instead
return

Typo: "teh"

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:193
> +    if (isInGPUProcess() && font.hasAnyComplexColorFormatGlyphs(glyphs,
numGlyphs)) {

A shame we have to do so much work just to check if it’s safe to use each
glyph. A little bit scary from a performance point of view. I wonder if there’s
a way to do this in future that is more efficient, perhaps with some help from
CoreText?

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:194
> +	   ASSERT_NOT_REACHED();

The old code did RELEASE_ASSERT, so intentionally crashed in when encountering
an unexpected font/glyph combination. The new code instead quietly does nothing
in production builds. Was the old behavior better or is the new behavior
better?

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:820
> +    if (m_hasNoComplexColorFormatGlyphs)
> +	   return false;
> +
> +    if (!m_glyphsWithComplexColorFormat) {

This is probably one of the best ways to organize this for relative simplicity.
But it’s sort of funny to use Optional and also use a bitfield. Optional stores
a boolean in a way that’s not very space-efficient. And the bitfield in turn
trades in cleaner coding idioms (like initializing in the class definition) for
greater efficiency.

If possible I would slightly prefer a representation where there are no invalid
states. The logical equivalent of a Variant with one value that says "don't
know", another that says "no complex glyphs", and a third state that indicates
"here is the BitVector". Not really suggesting we change anything. But this one
does have the state where there is a bit vector, but
m_hasNoComplexColorFormatGlyphs is true; a state we will never get into of
course.

Would be nice to break out the one-time code for computing
m_glyphsWithComplexColorFormat into its own function. For one thing it could go
into a separate FontCocoa.mm file. And could make the logic of the function
here easier to read. I understand that by having in this function we can do a
"m_hasNoComplexColorFormatGlyphs = true, return false" right in the middle of
it, but I think the separation of concerns would still be a plus.


More information about the webkit-reviews mailing list