Bug 125329 – Define m_hasBadParent in InlineBox if defined(ADDRESS_SANITIZER)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Drew Yao]

Source/WebCore/rendering/InlineBox.{h,cpp} have the concept of m_hasBadParent which is used to detect use after free issues.  It's only enabled #if !ASSERT_DISABLED.

If we're running fuzzing with Address Sanitizer in a release build, we're not getting the benefit of this checking.  It should also be enabled when building with Address Sanitizer.

The simple fix is to convert all the
#if !ASSERT_DISABLED
in these files to
#if !ASSERT_DISABLED || defined(ADDRESS_SANITIZER)

[David Kilzer (:ddkilzer)]

A slightly better approach may be to introduce an ASSERT_WITH_SECURITY_IMPLICATIONS_DISABLED macro and change:

#if !ASSERT_DISABLED

to:

#if !ASSERT_WITH_SECURITY_IMPLICATIONS_DISABLED

This would let us separate the two types of asserts and the code needed to implement them.

[David Kilzer (:ddkilzer)]

I'm working on this.  Pulling the thread has unraveled a few uses of #ifndef NDEBUG that should have been ASSERT_DISABLED or now ASSERT_WITH_SECURITY_IMPLICATION_DISABLED.

[David Kilzer (:ddkilzer)]

Created attachment 218697 [details]
Patch v1 for initial comments, not landing

[Darin Adler]

Comment on attachment 218697 [details]
Patch v1 for initial comments, not landing

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

> Source/WebCore/rendering/InlineBox.cpp:56
> -    if (!m_hasBadParent && m_parent)
> +    if (m_hasBadParent && m_parent)
>          m_parent->setHasBadChildList();

This change seems wrong. The point here is to tell the parent that this child is gone, and so the parent’s child list is now bad. It’s not safe to do that if the parent is bad. So we should do this only if the parent is *not* bad. Removing the ! breaks the code.

> Source/WebCore/rendering/InlineFlowBox.cpp:58
> -    if (!m_hasBadChildList)
> +    if (m_hasBadChildList) {
>          for (InlineBox* child = firstChild(); child; child = child->nextOnLine())
>              child->setHasBadParent();
> +    }

Same mistake here. The parent is going away and so any child that is still around now has a bad parent. But if the child list is already bad, then it’s not safe to walk the child list so we don’t even try in that case. Removing the ! breaks the code.

[David Kilzer (:ddkilzer)]

Created attachment 218927 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Created an attachment (id=218927) [details]
> Patch v2

Removed the changes to the m_hasBad* ivar checks.

[WebKit Commit Bot]

Comment on attachment 218927 [details]
Patch v2

Clearing flags on attachment: 218927

Committed r160462: <http://trac.webkit.org/changeset/160462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Farler]

*** Bug 124387 has been marked as a duplicate of this bug. ***