prevent GPU process from rendering sbix glyphs

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Cameron McCormack (:heycam)]

When GPUP canvas is enabled, the display list recorder handles sbix glyphs by recording drawImage calls.  This means that during display list replay, we should not have any sbix glyphs in a DrawGlyphs display list item.  Let's check that this is indeed the case.

[Cameron McCormack (:heycam)]

<rdar://77231959>

[Cameron McCormack (:heycam)]

Created attachment 429489 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 429491 [details]
Patch

[Darin Adler]

Comment on attachment 429491 [details]
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.

[Cameron McCormack (:heycam)]

(In reply to Darin Adler from comment #4)
> > 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?

Yes, one option would be to have some API to tell CoreText "here are the allowed glyph formats", and rely on it checking that state for each glyph that is drawn.

Though it is only done for fonts that have SVG or sbix glyphs in them.  After the first drawGlyphs call, when the BitVector is initialized, it devolves to a loop and a bitcheck for each glyph ID in the array.  My intuition is that compared with the amount of work that goes into the actual drawing of the glyphs, this is small.  (OK yes, never trust intuition when it comes to performance. :-))

> > 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?

Advice I received was that the new behavior is preferred so that, if an attacker did find a way to include disallowed glyph types in the display list items, that it would not allow them to DoS the browser by killing the GPU process.

> > 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.

I guess Variant is not quite what we want, in that while you could mint some enum types to represent the two "no BitVector" states, it's not as ergonomic as you'd like:

  enum ComplexColorGlyphsState { Unknown, None };
  Variant<ComplexColorGlyphsState, BitVector> m_glyphsWithComplexColorFormat;

or:

  enum ComplexColorGlyphsUnknown { State };
  enum ComplexColorGlyphsNone { State };
  Variant<ComplexColorGlyphsUnknown, ComplexColorGlyphsNone, BitVector> m_glyphs...;

Really I'm working around the fact that BitVector doesn't record whether all its bits are empty.  It has an isEmpty() which works slowly by counting bits, when it's spilled its inline storage.

(BitVector has some constructors for values to represent "empty" and "deleted", for hash table usage.  These aren't real BitVector values, and calling most member functions of a BitVector in this state would crash.  So we could use the "empty" value here to saving having the separate state which records whether the BitVector is empty.  But it feels slightly hacky.)

(Option could be taught to use these hash traits to save space too, when there's a distinct "empty" value that could be used instead of a separate boolean.)

> 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.

I'll look into doing that.

[Cameron McCormack (:heycam)]

And Optional<Optional<BitVector>> prevents being able to represent impossible states, at the cost of more storage.

[Darin Adler]

(In reply to Cameron McCormack (:heycam) from comment #6)
> And Optional<Optional<BitVector>> prevents being able to represent
> impossible states, at the cost of more storage.

And to be fair, also at the cost of more subtlety and confusion. We end up using Optional<Optional> when decoding optionals, and I think it’s pretty confusing. The various hasValue checks look so similar. Like x.hasValue and x->hasValue meaning totally different things, although I suppose !x and !*x are less easily confusable.

Because of issues like these, Sam Weinig has discouraged us from using Optional<bool>.

The search for something clear, straightforward, and unambiguous continues, I suppose. I think what you chose for this patch is fine, and it would also be nice to strive for better and reflect even after landing it.

The fact that BitVector doesn’t record when all its bits are empty makes sense to me when you think of it as a packed implementation of a vector of booleans. One would not expect a vector to keep track of whether all its values are false any more than a vector of integers keeps track of whether all its values are 0.

But I totally agree that we could make something like that for purposes like this one. The abstraction of a vector that knows how to not allocate any storage at all when it’s all false and that also can efficiently answer the "are these all false" question makes a lot of sense, and a lazily-computed one of those, using Optional, might be great for this use. In fact no reason we can’t just create that, built on top of BitVector. So maybe that’s the way to go to make an incremental improvement?

I often encourage people to make these classes "in place" at first, sharing the header with the class that uses them, then move them to WTF when the urge to use them elsewhere arises.

Anyway, really don’t want to slow you down; this patch is already excellent.

[Myles C. Maxfield]

Comment on attachment 429491 [details]
Patch

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

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:820
>> +    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.

I was actually going to say this, but it looks like Darin got here first :)

[Cameron McCormack (:heycam)]

I've split that function out (patch up in a bit), and it reads better now.  Thanks for the suggestion.  But I'll leave it in FontCoreText.cpp since it looks like all other similar "CoreText but not Windows" code is in this file, and I'd like to keep this new function near the function that uses it.

[Cameron McCormack (:heycam)]

Created attachment 429505 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 429505 [details]
Patch

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

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:811
> +    unsigned glyphCount = CTFontGetGlyphCount(getCTFont());

CTFontGetGlyphCount returns a CFIndex (and it's possible it might return -1 in some error case). It think you should use CFIndex here.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:814
> +    for (unsigned glyphID = 0; glyphID < glyphCount; ++glyphID) {

This goes through every glyph in the font? That sounds expensive. Did you measure how long this takes?

For short strings, wouldn't it be more efficient to check each glyph in the string, rather than building up the bit vector?

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 429505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429505&action=review
> 
> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:811
> > +    unsigned glyphCount = CTFontGetGlyphCount(getCTFont());
> 
> CTFontGetGlyphCount returns a CFIndex (and it's possible it might return -1
> in some error case). It think you should use CFIndex here.
> 
> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:814
> > +    for (unsigned glyphID = 0; glyphID < glyphCount; ++glyphID) {
> 
> This goes through every glyph in the font? That sounds expensive. Did you
> measure how long this takes?
> 
> For short strings, wouldn't it be more efficient to check each glyph in the
> string, rather than building up the bit vector?

Myles tells me this is probably OK. I think there are two bits of info that didn't make it into the changelog:
1. Most fonts don't have SBIX tables.
2. CTFontGetSbixImageSizeForGlyphAndContentsScale does internal caching.

[Cameron McCormack (:heycam)]

(In reply to Simon Fraser (smfr) from comment #11)
> This goes through every glyph in the font? That sounds expensive. Did you
> measure how long this takes?

Thanks for the prompt to measure.  I think in the end is a little slow -- on Apple Color Emoji (realistically where we'll run into this) -- which has ~3,400 glyphs, the loop to initialize the BitVector takes ~24 ms.  And that's on my fast desktop machine.

I will switch to caching individual bits, rather than eagerly computing them for all glyphs.

[Cameron McCormack (:heycam)]

Created attachment 429628 [details]
Patch

[Cameron McCormack (:heycam)]

I took the opportunity to prevent some of the "invalid state combinations" Darin mentioned too.

[Cameron McCormack (:heycam)]

Created attachment 429629 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 429628 [details]
Patch

r? for Myles to take a look.

[Simon Fraser (smfr)]

Comment on attachment 429629 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.h:276
> +        static constexpr size_t validBit(Glyph glyphID) { return static_cast<size_t>(glyphID) * 2; }
> +        static constexpr size_t valueBit(Glyph glyphID) { return static_cast<size_t>(glyphID) * 2 + 1; }

"valid" and "value" are so close they make this code hard to read.

> Source/WebCore/platform/graphics/Font.h:284
> +        BitVector m_bits;

Maybe a comment here saying that there are 2 bits per glyph, ordered [valid, value, ...]

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

Should this be a RELEASE_ASSERT?

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:797
> +    return { true, glyphCount * 2 };

Not immediately clear where the 2 comes from.

[Darin Adler]

Comment on attachment 429629 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:276
>> +        static constexpr size_t valueBit(Glyph glyphID) { return static_cast<size_t>(glyphID) * 2 + 1; }
> 
> "valid" and "value" are so close they make this code hard to read.

Possible replacement names:

bitForHasValue
bitForValue

>> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:194
>> +        ASSERT_NOT_REACHED();
> 
> Should this be a RELEASE_ASSERT?

I asked this when reviewing an earlier version and Cameron explained why we are making this change. People discussed and decided that we don’t this to be a way to crash the GPU Process.

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:797
>> +    return { true, glyphCount * 2 };
> 
> Not immediately clear where the 2 comes from.

Maybe the *2 belongs inside the ComplexColorFormatGlyphs constructor where it’s closer to the other *2.

[Darin Adler]

Comment on attachment 429629 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:276
>> +        static constexpr size_t valueBit(Glyph glyphID) { return static_cast<size_t>(glyphID) * 2 + 1; }
> 
> "valid" and "value" are so close they make this code hard to read.

Possible replacement names:

bitForHasValue
bitForValue

>> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:194
>> +        ASSERT_NOT_REACHED();
> 
> Should this be a RELEASE_ASSERT?

I asked this when reviewing an earlier version and Cameron explained why we are making this change. People discussed and decided that we don’t this to be a way to crash the GPU Process.

>> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:797
>> +    return { true, glyphCount * 2 };
> 
> Not immediately clear where the 2 comes from.

Maybe the *2 belongs inside the ComplexColorFormatGlyphs constructor where it’s closer to the other *2.

[Darin Adler]

Comment on attachment 429629 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.h:266
> +        static ComplexColorFormatGlyphs withNoRelevantTables();
> +        static ComplexColorFormatGlyphs withRelevantTables(unsigned glyphCount);

Naming here is creative, but a bit peculiar. I am OK with it, but might prefer more straight ahead names that include the word "create".

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:828
> +    auto sbixTableData = adoptCF(CTFontCopyTable(platformData().ctFont(), kCTFontTableSbix, kCTFontTableOptionNoOptions));
> +    if (sbixTableData)
> +        return true;

Put it inside the if please?

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:839
> +            CFIndex glyphCount = CTFontGetGlyphCount(getCTFont());
> +            m_glyphsWithComplexColorFormat = ComplexColorFormatGlyphs::withRelevantTables(std::max(glyphCount, 0L));

Not sure that calling std::max does what we want here if the glyph count is negative. Understand the desire to not pass a negative number, but passing 0 may not be OK.

Not great to have this code depend on "0L" matching CFIndex. If we do want to clamp to zero, then more elegant better idiom would be to use clampTo<destination-integer-type>.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:848
> +    if (OTSVGTableRef svgTable = otSVGTable().table) {

I like auto for a case like this.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:866
> +    OTSVGTableRef table = otSVGTable().table;

I like auto in a place like this.

[Cameron McCormack (:heycam)]

(In reply to Simon Fraser (smfr) from comment #18)
> > Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:194
> > +        ASSERT_NOT_REACHED();
> 
> Should this be a RELEASE_ASSERT?

Security folks advised me to make this a debug ASSERT, so as to avoid giving Web processes a chance to DoS the browser by killing the GPU process.

[Cameron McCormack (:heycam)]

(In reply to Darin Adler from comment #21)
> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:839
> > +            CFIndex glyphCount = CTFontGetGlyphCount(getCTFont());
> > +            m_glyphsWithComplexColorFormat = ComplexColorFormatGlyphs::withRelevantTables(std::max(glyphCount, 0L));
> 
> Not sure that calling std::max does what we want here if the glyph count is
> negative. Understand the desire to not pass a negative number, but passing 0
> may not be OK.

You're probably right.  Returning an error from CTFontGetGlyphCount would be an odd situation (and might not even be possible), and it'd be better to consider this like the "we didn't find any relevant tables" situation.

[Cameron McCormack (:heycam)]

Created attachment 429699 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 429735 [details]
Patch

[Darin Adler]

Comment on attachment 429735 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:206
> +CGFloat CTFontGetSbixImageSizeForGlyphAndContentsScale(CTFontRef, const CGGlyph, CGFloat contentsScale);

Peculiar use of const here. I assume this is just how the function is declared, but that const has no effect.

> Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:813
> +    ASSERT(!m_hasRelevantTables);

This assertion is backwards! This should only be called on a font that *does* have relevant tables. I didn’t notice that on my own; the EWS testing caught it.

[Cameron McCormack (:heycam)]

(In reply to Darin Adler from comment #26)
> > Source/WebCore/PAL/pal/spi/cf/CoreTextSPI.h:206
> > +CGFloat CTFontGetSbixImageSizeForGlyphAndContentsScale(CTFontRef, const CGGlyph, CGFloat contentsScale);
> 
> Peculiar use of const here. I assume this is just how the function is
> declared, but that const has no effect.

Indeed, it has no effect, but that's how it's declared in the place I'm copying it from.

> > Source/WebCore/platform/graphics/coretext/FontCoreText.cpp:813
> > +    ASSERT(!m_hasRelevantTables);
> 
> This assertion is backwards! This should only be called on a font that
> *does* have relevant tables. I didn’t notice that on my own; the EWS testing
> caught it.

Will fix!

[Cameron McCormack (:heycam)]

Created attachment 429802 [details]
Patch

[EWS]

Committed r278151 (238195@main): <https://commits.webkit.org/238195@main>

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