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
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.
Created attachment 89726 [details] proposed patch proposed patch to fix this issue. Also some style fixes for class FloatingObjects within it.
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.
Null pointer crashes aren't security bugs, are they?
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'; }
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).
Chrome bug mentions that it was fixed in this commit: https://trac.webkit.org/changeset/80657/webkit Marking as "RESOLVED CONFIGURATION CHANGED". Thanks!