Null deref in InlineBox::height()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Cris Neckar]

Created attachment 66801 [details]
repro

When firstline is true the renderers style() method may return null and then be derefed. 

I hav been hitting this a lot with a fuzzer I am working on and will submit a patch shortly.

the attached repro will trigger this.

[Cris Neckar]

Created attachment 66812 [details]
Patch

[Cris Neckar]

+jamesr

James can you review this when you get a chance. Thanks!

[James Robinson]

Comment on attachment 66812 [details]
Patch

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

> WebCore/rendering/InlineBox.cpp:99
> +    if (!renderer()->style(m_firstLine))
> +        return 0;
>      if (renderer()->isText())
>          return m_isText ? renderer()->style(m_firstLine)->font().height() : 0;
>      if (renderer()->isBox() && parent())
There's one very slight behavior change here - if renderer()->isBox() && parent() == true, old code wouldn't call renderer()->style(m_firstLine) at all whereas this patch will.  Can you reoranize this so that's still true?  I think moving the isBox() && ... branch above the NULL check will suffice.

[Cris Neckar]

Created attachment 66917 [details]
Patch

[Cris Neckar]

Good catch. Thank you!

[Eric Seidel (no email)]

Comment on attachment 66917 [details]
Patch

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

> WebCore/rendering/InlineBox.cpp:-97
> +    if (!renderer()->style(m_firstLine))
> +        return 0;
>      if (renderer()->isText())
>          return m_isText ? renderer()->style(m_firstLine)->font().height() : 0;
> -    if (renderer()->isBox() && parent())
I believe style(m_firstLine) is non-trivial.  Shouldn't we cache it in a local variable?

    RenderStyle* firstLineStyle() const { return document()->usesFirstLineRules() ? firstLineStyleSlowCase() : style(); }
    RenderStyle* style(bool firstLine) const { return firstLine ? firstLineStyle() : style(); }

Does this still happen with the fix for bug 45574 (r67255 or later)? style() should never return null and if it is, then that should be fixed, not papered over.

[Ryosuke Niwa]

The repro now causes a crash at the line 96 of InlineBox.cpp:

    if (renderer()->isText())
        return m_isText ? renderer()->style(m_firstLine)->font().height() : 0;

font() is 0 and crashes inside RefPtr.

(In reply to comment #8)
> The repro now causes a crash at the line 96 of InlineBox.cpp:
> 
>     if (renderer()->isText())
>         return m_isText ? renderer()->style(m_firstLine)->font().height() : 0;
> 
> font() is 0 and crashes inside RefPtr.

Seems like style(true) is still null.

I think the issue is that RenderObject::firstLineStyleSlowCase() calls setHasPseudoStyle() on the wrong RenderStyle, namely the RenderText’s instead of its parent’s.

<rdar://problem/8478160>

Created attachment 68814 [details]
Set the pseudo style bit on the RenderStyle which is going to supply the first-line style

Fixed in <http://trac.webkit.org/projects/webkit/changeset/68335>.