Bug 56301 – chrome.dll!WebCore::RenderBlock::removeFloatingObjectsBelow ReadAV@NULL (928f227631041a7b4b71dd15efeae337)

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Berend-Jan Wever]

Created attachment 85664 [details]
Repro

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

Repro:
xxxxxxxxxxxxxxxxx--><img border>+++++++++++++++++++++xxxxxxxxxxxxxxxxxxxxxxxxxxxx><
<style>
  *{
    -webkit-columns:auto 3;
    margin-bottom:.87%;
    border-left-width:thick;
    float:right;
  }
</style>

The code hits this assert:
    template<typename T, size_t inlineCapacity, typename U>
    inline T& ListHashSet<T, inlineCapacity, U>::last()
    {
        ASSERT(!isEmpty());
        return m_tail->m_value;
    }
When executing "curr = floatingObjectSet.last()" because "floatingObjectSet" is empty in:
void RenderBlock::removeFloatingObjectsBelow(FloatingObject* lastFloat, int logicalOffset)
{
    if (!m_floatingObjects)
        return;
    
    FloatingObjectSet& floatingObjectSet = m_floatingObjects->set();
    FloatingObject* curr = floatingObjectSet.last();
    while (curr != lastFloat && (!curr->isPlaced() || logicalTopForFloat(curr) >= logicalOffset)) {
        m_floatingObjects->decreaseObjectsCount(curr->type());
        floatingObjectSet.removeLast();
        delete curr;
        curr = floatingObjectSet.last();
    }
}

This code was called with "lastFloat" == 0, from "void RenderBlock::layoutInlineChildren(bool relayoutChildren, int& repaintLogicalTop, int& repaintLogicalBottom)", because the set of floating objects is empty, which is probably where the issue originates. The function is a bit too complex to do a quick analysis, but it's safe to say this is a NULL ptr crash and not a security issue.

id:             chrome.dll!WebCore::RenderBlock::removeFloatingObjectsBelow ReadAV@NULL (928f227631041a7b4b71dd15efeae337)
description:    Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::RenderBlock::removeFloatingObjectsBelow
application:    Chromium 12.0.704.0
stack:          chrome.dll!WebCore::RenderBlock::removeFloatingObjectsBelow
                chrome.dll!WebCore::RenderBlock::layoutInlineChildren
                chrome.dll!WebCore::RenderBlock::layoutBlock
                chrome.dll!WebCore::RenderBlock::layoutColumns
                chrome.dll!WebCore::RenderBlock::layoutBlock
                chrome.dll!WebCore::RenderBlock::layout
                chrome.dll!WebCore::RenderBlock::insertFloatingObject
                chrome.dll!WebCore::RenderBlock::layoutBlockChildren
                chrome.dll!WebCore::RenderBlock::layoutBlock
                chrome.dll!WebCore::RenderBlock::layout
                chrome.dll!WebCore::RenderBlock::insertFloatingObject
                chrome.dll!WebCore::RenderBlock::layoutBlockChildren
                chrome.dll!WebCore::RenderBlock::layoutBlock
                chrome.dll!WebCore::RenderBlock::layout
                chrome.dll!WebCore::RenderView::layout
                chrome.dll!WebCore::FrameView::layout
                chrome.dll!WebCore::Document::implicitClose
                chrome.dll!WebCore::FrameLoader::checkCompleted
                chrome.dll!WebCore::FrameLoader::finishedParsing
                chrome.dll!WebCore::Document::finishedParsing
                chrome.dll!WebCore::HTMLDocumentParser::prepareToStopParsing
                chrome.dll!WebCore::DocumentWriter::endIfNotLoadingMainResource
                chrome.dll!WebCore::FrameLoader::finishedLoading
                chrome.dll!WebCore::MainResourceLoader::didFinishLoading
                chrome.dll!WebCore::ResourceLoader::didFinishLoading
                chrome.dll!WebCore::ResourceHandleInternal::didFinishLoading
                chrome.dll!webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest
                chrome.dll!ResourceDispatcher::OnRequestComplete
                chrome.dll!IPC::MessageWithTuple<...>::Dispatch<...>
                chrome.dll!ResourceDispatcher::DispatchMessageW
                chrome.dll!ResourceDispatcher::OnMessageReceived
                chrome.dll!ChildThread::OnMessageReceived
                chrome.dll!RunnableMethod<...>::Run
                chrome.dll!MessageLoop::RunTask
                chrome.dll!MessageLoop::DoWork
                chrome.dll!base::MessagePumpDefault::Run
                chrome.dll!MessageLoop::RunInternal
                chrome.dll!MessageLoop::Run
                chrome.dll!RendererMain

[Yuqiang Xian]

For this case, when removeFloatingObjectsBelow is invoked the floatingObjectSet is not empty and it contains one element. It becomes empty after one iteration of the loop which removes the last (and the only) element from the set.

The reason why "lastFloat" parameter is 0 while the floatingObjectSet is not empty is caused by the code below in RenderBlockLineLayout.cpp -

void RenderBlock::layoutInlineChildren(bool relayoutChildren, int& repaintLogicalTop, int& repaintLogicalBottom) {
...
            // around line 691, at this point the set is empty so lastFloat is 0
            FloatingObject* lastFloatFromPreviousLine = (m_floatingObjects && !m_floatingObjects->set().isEmpty()) ? m_floatingObjects->set().last() : 0;
            // one element is inserted into the floating object set
            end = findNextLineBreak(resolver, firstLine, isLineEmpty, lineBreakIteratorInfo, previousLineBrokeCleanly, hyphenated, &clear, lastFloatFromPreviousLine);
...
            // around line 828, the "removeXXX" is invoked with lastFloat as 0  
            removeFloatingObjectsBelow(lastFloatFromPreviousLine, oldLogicalHeight);
...
}

So there seems to be two issues,
1) in the loop of removeFloatingObjectsBelow there's no check for null lastFloat, which should be added. In this special case we may directly clean-up the floatingObjectSet instead of removing the objects one by one?
2) no "isEmpty()" check for floatingObjectSet before the invocations to "first()", "last()" and "removeLast()" as these ListHashSet operations require the set to be non-empty.

[Yuqiang Xian]

Created attachment 89726 [details]
proposed patch

proposed patch to fix this issue. Also some style fixes for class FloatingObjects within it.

[Alexis Menard (darktears)]

Comment on attachment 89726 [details]
proposed patch

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

The fix itself looks good though.

> Source/WebCore/ChangeLog:9
> +        an empty set. Also have some style fixes for class FloatingObjects.

The style fixes should be probably committed as a separate patch.

> Source/WebCore/ChangeLog:11
> +        No new tests, relying on existing layout tests.

But do they actually cover the problem? I believe no, so it would be nice to write one which covers the crash.

> Source/WebCore/rendering/RenderBlock.cpp:139
> +

This code is the same as the one you removed below. Why this change? It makes the diff harder to read and introduces changes for nothing, making hard for people to blame for example. Could you please fix that? Thanks.

[Eric Seidel (no email)]

Null pointer crashes aren't security bugs, are they?

[Berend-Jan Wever]

In general, NULL ptr crashes are not security issues if they directly reference the NULL or NULL plus a small static offset. However, if they reference memory at an attacker controllable offset from NULL, they may be exploitable. Eg. if you have a NULL ptr used for an array, and the attacker can control the index into this array. This pseudo-code would be bad:

void bad(uint index) {
  char* null = 0;
  null[index] = 'A';
}

[David Levin]

Comment on attachment 89726 [details]
proposed patch

There should be a new test for this. The existing tests must not cover this or else it would have been known before now (and this change should be unskipping a test).

[Ahmad Saleem]

Chrome bug mentions that it was fixed in this commit:

https://trac.webkit.org/changeset/80657/webkit

Marking as "RESOLVED CONFIGURATION CHANGED". Thanks!