Bug 57756 – chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (two crashes)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Berend-Jan Wever]

Created attachment 88060 [details]
Repro 34b28124d3d1bdb69ec42adc292c8e77

Repro:
<script>
  document.writeln("<v>");
  document.body.innerHTML="<style>*{border-radius:5ex;}</style>";
  document.write("<title>x");
</script>

id:             chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (34b28124d3d1bdb69ec42adc292c8e77)
description:    Attempt to read from unallocated NULL pointer+0x30 in chrome.dll!WebCore::RenderStyle::fontMetrics
application:    Chromium 12.0.725.0
stack:          chrome.dll!WebCore::RenderStyle::fontMetrics
                chrome.dll!WebCore::CSSPrimitiveValue::computeLengthDouble
                chrome.dll!WebCore::CSSPrimitiveValue::computeLengthInt
                chrome.dll!WebCore::CSSStyleSelector::applyProperty
                chrome.dll!WebCore::CSSStyleSelector::applyDeclarations<...>
                chrome.dll!WebCore::CSSStyleSelector::styleForElement
                chrome.dll!WebCore::Node::styleForRenderer
                chrome.dll!WebCore::HTMLTitleElement::textWithDirection
                chrome.dll!WebCore::HTMLTitleElement::childrenChanged
                chrome.dll!WebCore::ContainerNode::parserAddChild
                chrome.dll!WebCore::HTMLConstructionSite::attachAtSite
                chrome.dll!WebCore::HTMLConstructionSite::insertTextNode
                chrome.dll!WebCore::HTMLTreeBuilder::processCharacterBuffer
                chrome.dll!WebCore::HTMLTreeBuilder::processCharacter
                chrome.dll!WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken
                chrome.dll!WebCore::HTMLTreeBuilder::constructTreeFromToken
                chrome.dll!WebCore::HTMLDocumentParser::pumpTokenizer
                chrome.dll!WebCore::HTMLDocumentParser::insert
                chrome.dll!WebCore::Document::write
                chrome.dll!WebCore::Document::write
                chrome.dll!WebCore::V8HTMLDocument::writeCallback
                chrome.dll!v8::internal::HandleApiCallHelper<...>
                chrome.dll!v8::internal::Builtin_HandleApiCall
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                chrome.dll!v8::Script::Run

- or -
<script>
  document.writeln("<v>");
  document.body.innerHTML="<style>*{-webkit-border-end-width:5ex;}</style>";
  document.write("<title>x");
</script>

id:             chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (164e5193a19e7700092c2c9f88ca066e)
(ever so slightly different stack)

Both repros trigger similar NULL ptr crashes, so I assume they are the same issue.

[Berend-Jan Wever]

Created attachment 88061 [details]
Repro 164e5193a19e7700092c2c9f88ca066e

[Berend-Jan Wever]

This is probably related to bug 51466 or a variation thereof.

Crash id for debug builds:
chrome.dll!WebCore::Font::primaryFont ReadAV@NULL (8f0571b4327014e218145b45bae3bada)

Simplified repro:
<script>
  document.documentElement.innerHTML = '<title style="border:0ex;">x';
</script>

m_fontList is NULL in:
webkit\source\webcore\platform\graphics\font.h
inline const SimpleFontData* Font::primaryFont() const
{
    ASSERT(m_fontList);
    return m_fontList->primarySimpleFontData(this);
}

Which is called by:
webkit\source\webcore\platform\graphics\font.h
  const FontMetrics& fontMetrics() const { return primaryFont()->fontMetrics(); }

Which is called by:
webkit\source\webcore\rendering\style\renderstyle.h
    const FontMetrics& fontMetrics() const { return inherited->font.fontMetrics(); }

Which is called by:
webkit\source\webcore\css\cssprimitivevalue.cpp
double CSSPrimitiveValue::computeLengthDouble(RenderStyle* style, RenderStyle* rootStyle, double multiplier, bool computingFontSize)
{
    unsigned short type = primitiveType();
<<<snip>>>
    switch (type) {
<<<snip>>>
        case CSS_EXS:
<<<snip>>>
            factor = style->fontMetrics().xHeight();

A few calls up the stack we see this:
webkit\source\webcore\css\cssstyleselector.cpp
PassRefPtr<RenderStyle> CSSStyleSelector::styleForElement(Element* e, RenderStyle* defaultParent, bool allowSharing, bool resolveForRootDefault, bool matchVisitedPseudoClass)
{
<<<snip>>>
    // Now do the author and user normal priority properties and all the !important properties.
    if (!resolveForRootDefault) {
        applyDeclarations<false>(false, lastUARule + 1, m_matchedDecls.size() - 1);
        applyDeclarations<false>(true, firstAuthorRule, lastAuthorRule);

I believe the problem is that Font::update should have been called before we hit this code. It set m_fontList for the font:
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/Font.cpp&q=Font::update&exact_package=chromium&sa=N&cd=1&ct=rc&l=115

void Font::update(PassRefPtr<FontSelector> fontSelector) const
{
    // FIXME: It is pretty crazy that we are willing to just poke into a RefPtr, but it ends up
    // being reasonably safe (because inherited fonts in the render tree pick up the new
    // style anyway. Other copies are transient, e.g., the state in the GraphicsContext, and
    // won't stick around long enough to get you in trouble). Still, this is pretty disgusting,
    // and could eventually be rectified by using RefPtrs for Fonts themselves.
    if (!m_fontList)
        m_fontList = FontFallbackList::create();
    m_fontList->invalidate(fontSelector);
}

@hyatt: You wrote the above comment in Font::update, can you have a look?

[Berend-Jan Wever]

Chromium: http://code.google.com/p/chromium/issues/detail?id=78321

[Julien Chaffraix]

Created attachment 93034 [details]
tentative fix

I had a look at this bug yesterday and I came to the same conclusion.

In one of the branch inside styleForElement, the new RenderStyle does not inherits its FontFallbackList from its parents and thus it never gets created. Attached is the fix I came with (I am not marking it for review as I would also like to hear from dhyatt), feel free to re-use the test cases if this is not the right way.

[Eric Seidel (no email)]

A null pointer crash is not a security bug, right?

[Julien Chaffraix]

(In reply to comment #5)
> A null pointer crash is not a security bug, right?

This bug is not marked as security (ie public). My limited understanding is that null pointer dereferences are usually considered a non-exploitable DoS (though there has been some ways to leverage them in some specific contexts).

[Eric Seidel (no email)]

Sorry, my mis-read.

[Julien Chaffraix]

Comment on attachment 93034 [details]
tentative fix

After a quick discussion with dhyatt, this looks like the right approach. Putting the patch up for review.

[Hajime Morrita]

Comment on attachment 93034 [details]
tentative fix

Why not let Font class create m_fontList lazily? This fix looks fragile for me...

[Julien Chaffraix]

> Why not let Font class create m_fontList lazily?

I think that's what Font::update is about. My understanding is that we don't want to allocate m_fontList unless we really need it. Also lazy allocation would have a cost on font handling.

> This fix looks fragile for me...

I don't deny that this is fragile and I don't mind refactoring the code (now or later) to make the code less fragile if that's what people prefer.

[Dave Hyatt]

Comment on attachment 93034 [details]
tentative fix

r=me

I think this is ok for now, since this particular case is not common.

[WebKit Review Bot]

Comment on attachment 93034 [details]
tentative fix

Clearing flags on attachment: 93034

Committed r88472: <http://trac.webkit.org/changeset/88472>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

FYI, filed bug 62390 for the font handling refactoring.