Bug 189154 – The width of an empty or nullptr TextRun should be zero

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

A default initialized or null string wraps a nullptr StringImpl. Most accessors in WTFString.cpp, such as isAllASCII(), hash(), etc., perform a nullptr check before using m_impl, but is8Bit() does not.

This patch adds a check in the is8Bit() implementation to be consistent with other methods, and to address a small number of crashes observed in testing.

[Brent Fulgham]

Created attachment 348501 [details]
Patch

[Brent Fulgham]

Another possible fix could be this:

diff --git a/Source/WebCore/platform/graphics/FontCascade.cpp b/Source/WebCore/platform/graphics/FontCascade.cpp
index 7648f5bf269..245a418731b 100644
--- a/Source/WebCore/platform/graphics/FontCascade.cpp
+++ b/Source/WebCore/platform/graphics/FontCascade.cpp
@@ -604,6 +604,9 @@ FontCascade::CodePath FontCascade::codePath(const TextRun& run, std::optional<un
     if (s_codePath != Auto)
         return s_codePath;
 
+    if (!run.length())
+        return Simple;
+
 #if !USE(FREETYPE)
     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
     if ((enableKerning() || requiresShaping()) && (from.value_or(0) || to.value_or(run.length()) != run.length()))

However, I think there are enough unchecked 'is8Bit()' calls in WebKit that it makes sense to handle it in WTFString.

But perhaps I should do both changes?

[Brent Fulgham]

<rdar://problem/43685926>

[Anders Carlsson]

Asking whether a null string is 8-bit or not doesn't make sense - it's neither!

I think checking for a null run.length() is better.

[Brent Fulgham]

(In reply to Anders Carlsson from comment #4)
> Asking whether a null string is 8-bit or not doesn't make sense - it's
> neither!
> 
> I think checking for a null run.length() is better.

Sounds good. I do worry that other calls (such as isAllASCII()) have similar weirdness issues.

[alan]

(In reply to Anders Carlsson from comment #4)
> Asking whether a null string is 8-bit or not doesn't make sense - it's
> neither!
> 
> I think checking for a null run.length() is better.
I agree. This check would probably just mask flawed code on the caller side.
-and that begs the question of why you ask for the code path on an empty run.

[Brent Fulgham]

It looks like there are a lot of places where is8Bit() is not nullptr checked, even in files where it is checked in other methods.

Should I take a pass through all of these and add the necessary checks to make them consistent?

[Brent Fulgham]

Looking into this a bit further, it's a simpler/less risky change to just recognize that the width of an empty/null TextRun is always zero, rather than attempt to compute this from font data.

[Brent Fulgham]

Created attachment 348509 [details]
Patch

[alan]

Comment on attachment 348509 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:157
> +    bool is8Bit() const { return m_impl && m_impl->is8Bit(); }

Is this just a leftover from the previous candidate?

[Brent Fulgham]

Created attachment 348518 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 348518 [details]
Patch

Clearing flags on attachment: 348518

Committed r235516: <https://trac.webkit.org/changeset/235516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Reverted r235516 for reason:

Caused 50 Crashes on Sierra

Committed r235523: <https://trac.webkit.org/changeset/235523>

[Darin Adler]

I’m annoyed having String::is8Bit be a "don’t touch this if null" function; it makes it really inconvenient to write code with a special case for 8-bit and another special case for 16-bit; you always have to special case the empty string, which seems like overkill for me. Requiring we always write a special case for the empty string is super-annoying.

If we want to take the position that a null string is neither 8-bit nor 16-bit, then I suggest we consider adding an is16Bit function as well as the is8Bit function and have both return false for null strings. Or, since processing with 8-bit is perhaps more efficient, maybe the function should be something with semantics more like canUse8Bit and then it can return true for the null string or mayRequire16Bit or whatever?

I really don’t want to have WTF::String functions that you can’t call on a null string. I think this is currently the *only* one with this behavior and it bothers me quite a bit.

[Brent Fulgham]

Created attachment 348655 [details]
Patch

[Brent Fulgham]

This patch goes back to my initial nullptr check, as Darin suggested. However, it looks like bypassing work when we have a nullptr string is useful, so I've fixed the three places where my assertion could fire during layout tests.

[alan]

Comment on attachment 348655 [details]
Patch

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

> Source/WebCore/rendering/RenderText.cpp:1246
> +    if (!m_text.isEmpty()) {

computeCanUseSimplifiedTextMeasuring could actually early return with true if it has no content.

    auto& font = style().fontCascade();
    if (font.wordSpacing() || font.letterSpacing())
        return false;

    if (m_text.isEmpty()
        return true;

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:108
> +    if (text.length()) {
> +        m_isComplexText = scaledFont.codePath(m_run) == FontCascade::Complex;
>  
> -    if (m_isComplexText)
> -        m_simpleWidthIterator = nullptr;
> -    else
> -        m_simpleWidthIterator = std::make_unique<WidthIterator>(&scaledFont, m_run);
> +        if (m_isComplexText)
> +            m_simpleWidthIterator = nullptr;
> +        else
> +            m_simpleWidthIterator = std::make_unique<WidthIterator>(&scaledFont, m_run);
> +    }

In here you need to re-create the m_simpleWidthIterator with the new m_run (to avoid UAF)

[Daniel Bates]

Comment on attachment 348655 [details]
Patch

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

> Source/WebCore/platform/graphics/ComplexTextController.cpp:73
> +        if (!text.length())

Does this handle combined text?

[Daniel Bates]

(In reply to Daniel Bates from comment #19)
> Comment on attachment 348655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348655&action=review
> 
> > Source/WebCore/platform/graphics/ComplexTextController.cpp:73
> > +        if (!text.length())
> 
> Does this handle combined text?

Disregard this remark.

[Daniel Bates]

According to the inline comment for RenderText::text(), <https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderText.h?rev=229147#L65>, it can never hold a null string. Is this comment still correct? Unless the empty string is common and has performance implications, can we make use of the fact that RenderText::text() is never a null string to reduce the need to add checks for the empty string. In particular, can we make use of this fact to either change to checking for a null string, which is the problematic case from my understanding, or remove some checks entirely from the patch?

[Brent Fulgham]

(In reply to Daniel Bates from comment #21)
> According to the inline comment for RenderText::text(),
> <https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderText.
> h?rev=229147#L65>, it can never hold a null string. Is this comment still
> correct? Unless the empty string is common and has performance implications,
> can we make use of the fact that RenderText::text() is never a null string
> to reduce the need to add checks for the empty string. In particular, can we
> make use of this fact to either change to checking for a null string, which
> is the problematic case from my understanding, or remove some checks
> entirely from the patch?

I do not think that comment in RenderText is correct. My new assertion fired in this code while running our layout tests, indicating that this invariant is not true.

[Brent Fulgham]

Comment on attachment 348655 [details]
Patch

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

>> Source/WebCore/rendering/RenderText.cpp:1246
>> +    if (!m_text.isEmpty()) {
> 
> computeCanUseSimplifiedTextMeasuring could actually early return with true if it has no content.
> 
>     auto& font = style().fontCascade();
>     if (font.wordSpacing() || font.letterSpacing())
>         return false;
> 
>     if (m_text.isEmpty()
>         return true;

Done.

>> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:108
>> +    }
> 
> In here you need to re-create the m_simpleWidthIterator with the new m_run (to avoid UAF)

Do you mean in the case of a nullptr RenderSVGInlineText string? I'll post a new patch for you to give final approval to.

[Brent Fulgham]

Created attachment 348743 [details]
Patch

[Darin Adler]

Comment on attachment 348743 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        This patch recognizes that an empty TextRun should always produce a zero width, rather than
> +        attempt to compute this value from font data.

I am a bit puzzled by our decision making here.

This kind of change, adding checks for empty text runs with early exits, is fine with me if we think that empty text runs are common and deserve to be optimized. If empty text runs should almost never occur, and they only occasionally do, and the performance in that case is not important to carefully optimize, then I think it would be fine to omit empty string and zero length special cases and let general purpose code run even though it will do extra work. I suspect the only reason the general case code didn’t run successfully before was the unguarded is8Bit checks.

> Source/WTF/wtf/text/WTFString.h:157
> -    bool is8Bit() const { return m_impl->is8Bit(); }
> +    bool is8Bit() const { return !m_impl || m_impl->is8Bit(); }

I suggest landing this separately. I do want this change but I don’t think it’s needed to resolve the problem we are solving here if we are also going to add empty string and zero length checks in all these places.

Alternatively we could do *just* this one and not add all those special cases to the text code like the original proposal here.

But doing both seems a little strange to me.

I’d also like to hear at least a little more about what Anders thinks about the design of WTF::String and this function. I don’t want my opinion to override his judgement.

Also, if we do remove this, then we can follow up by removing quite a few null checks if we add this one here; should audit for null, empty, and zero length checks that aren’t beneficial any more once we change this. Like the null check in String::sizeInBytes just below this. Maybe the compiler will remove them for us, but it also seems worthwhile to have the code be less complex and repetitive.

[Daniel Bates]

(In reply to Brent Fulgham from comment #22)
> (In reply to Daniel Bates from comment #21)
> > According to the inline comment for RenderText::text(),
> > <https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderText.
> > h?rev=229147#L65>, it can never hold a null string. Is this comment still
> > correct? Unless the empty string is common and has performance implications,
> > can we make use of the fact that RenderText::text() is never a null string
> > to reduce the need to add checks for the empty string. In particular, can we
> > make use of this fact to either change to checking for a null string, which
> > is the problematic case from my understanding, or remove some checks
> > entirely from the patch?
> 
> I do not think that comment in RenderText is correct. My new assertion fired
> in this code while running our layout tests, indicating that this invariant
> is not true.


Are you saying you added an ASSERT(m_text.impl()) to RenderText::text() and it fired when you ran the layout tests? If not, can you please elaborate on the assertion you added and where you added it? If the comment in RenderText.h for text() is incorrect then can we please fix text() up and/or update the comment. Leaving the comment is confusing in the face of your proposed change.

[Brent Fulgham]

(In reply to Daniel Bates from comment #26)
> Are you saying you added an ASSERT(m_text.impl()) to RenderText::text() and
> it fired when you ran the layout tests? If not, can you please elaborate on
> the assertion you added and where you added it? If the comment in
> RenderText.h for text() is incorrect then can we please fix text() up and/or
> update the comment. Leaving the comment is confusing in the face of your
> proposed change.

No.

I added the assertion ASSERT(run.length()) to FontCascade::codePath, ran layout tests, and found that assertions fired for the following methods:

RenderText::computeCanUseSimplifiedTextMeasuring
SVGTextMetricsBuilder::initializeMeasurementWithTextRenderer
TextLayout::isNeeded

I don't think the claim that RenderText's m_text member can never be null is correct. It is checked for null in the constructor, but the m_text member can be changed dynamically through various method calls, so it's not at all clear that this claim is true.

I'll revise the patch to remove that comment.

Alternatively, we could assert for non-null String in the setter method, and fix the places where a null value is being added.

[Brent Fulgham]

Created attachment 348825 [details]
Patch

[Brent Fulgham]

(In reply to Brent Fulgham from comment #27)
> I added the assertion ASSERT(run.length()) to FontCascade::codePath, ran
> layout tests, and found that assertions fired for the following methods:

I spoke with this in more detail with Dan, who pointed out that a zero-length TextRun is not the same thing as a TextRun with a nullptr String, and that the assertion firing in the empty string case did not constitute a violation of the non-nullability of the RenderText.

I agree.

I have revised the patch to remove the assertion from FontCascade::codePath, since it is not useful.

I have left the checks for empty strings because I do think empty strings occur frequently enough that avoiding the work of constructing objects, making assessments of style and other characteristics, and performing calculations that always yield zero is a waste of energy.

[Daniel Bates]

Comment on attachment 348825 [details]
Patch

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

> Source/WebCore/platform/graphics/ComplexTextController.cpp:75
> +        if (!text.length())
> +            return false;
> +

This is unnecessary because RenderText::text() can never be the null string, RenderBlock::constructTextRun() knows how to handle the empty string and do not believe this function is hot enough to warrant special casing the empty string.

> Source/WebCore/platform/graphics/FontCascade.cpp:345
> +    if (!run.length())
> +        return 0;

I do not get the impression that a run with a zero length is the root problem.

> Source/WebCore/platform/graphics/FontCascade.cpp:393
> +    if (!run.length())
> +        return 0;
> +

I do not get the impression that a run with a zero length is the root problem.

> Source/WebCore/rendering/RenderText.cpp:1246
> +    if (m_text.isEmpty())
> +        return false;

This is not necessary because m_text cannot be the null string and TextRun knows how to handle empty and non-empty strings.

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:101
> +    m_isComplexText = text.length() && scaledFont.codePath(m_run) == FontCascade::Complex;

Ditto.

(Notice that RenderSVGInlineText extends RenderText).

[Daniel Bates]

I am unclear how it makes sense for TextRun to ever hold a null string. I suggest we fix this issue to fix this bug.

[Brent Fulgham]

Created attachment 348925 [details]
Patch

[Brent Fulgham]

I added some logging, and determined that on common web pages we hit the zero-length string code path about 1:50 times through the width measurement. Seems common enough to warrant special casing.

[WebKit Commit Bot]

Comment on attachment 348925 [details]
Patch

Clearing flags on attachment: 348925

Committed r235721: <https://trac.webkit.org/changeset/235721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.