[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