Bug (CVE-2012-3686) – Absolute positioned elements with Inline Relative Positioned Container are not layout correctly

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Robin Cao]

Absolute positioned elements with Inline Relative Positioned Container are not layout correctly

[Robin Cao]

Created attachment 127179 [details]
test case

[Robin Cao]

The test case will cause an assert failure. Here is the call stack.


Program received signal SIGSEGV, Segmentation fault.
0x00007ffb99b53a90 in WebCore::InlineTextBox::constructTextRun (this=0x1994448, style=0x19acb70, font=..., characters=0x15d2e50, length=4, maximumLength=1, charactersWithHyphen=0x0)
    at ../../../../Source/WebCore/rendering/InlineTextBox.cpp:1320
1320	    ASSERT(maximumLength >= length);
(gdb) bt
#0  0x00007ffb99b53a90 in WebCore::InlineTextBox::constructTextRun (this=0x1994448, style=0x19acb70, font=..., characters=0x15d2e50, length=4, maximumLength=1, charactersWithHyphen=0x0)
    at ../../../../Source/WebCore/rendering/InlineTextBox.cpp:1320
#1  0x00007ffb99b4fb08 in WebCore::InlineTextBox::paint (this=0x1994448, paintInfo=..., paintOffset=...) at ../../../../Source/WebCore/rendering/InlineTextBox.cpp:669
#2  0x00007ffb99b4634a in WebCore::InlineFlowBox::paint (this=0x1994548, paintInfo=..., paintOffset=..., lineTop=0, lineBottom=19) at ../../../../Source/WebCore/rendering/InlineFlowBox.cpp:1069
#3  0x00007ffb99ccc9a0 in WebCore::RootInlineBox::paint (this=0x1994548, paintInfo=..., paintOffset=..., lineTop=0, lineBottom=19) at ../../../../Source/WebCore/rendering/RootInlineBox.cpp:196
#4  0x00007ffb99c4a5c5 in WebCore::RenderLineBoxList::paint (this=0x1993598, renderer=0x19934f8, paintInfo=..., paintOffset=...) at ../../../../Source/WebCore/rendering/RenderLineBoxList.cpp:262
#5  0x00007ffb99b680fb in WebCore::RenderBlock::paintContents (this=0x19934f8, paintInfo=..., paintOffset=...) at ../../../../Source/WebCore/rendering/RenderBlock.cpp:2580
#6  0x00007ffb99b6895e in WebCore::RenderBlock::paintObject (this=0x19934f8, paintInfo=..., paintOffset=...) at ../../../../Source/WebCore/rendering/RenderBlock.cpp:2690
#7  0x00007ffb99b67218 in WebCore::RenderBlock::paint (this=0x19934f8, paintInfo=..., paintOffset=...) at ../../../../Source/WebCore/rendering/RenderBlock.cpp:2435
#8  0x00007ffb99c2a679 in WebCore::RenderLayer::paintLayerContents (this=0x1993708, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2912
#9  0x00007ffb99c29ebb in WebCore::RenderLayer::paintLayerContentsAndReflection (this=0x1993708, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2813
#10 0x00007ffb99c29d98 in WebCore::RenderLayer::paintLayer (this=0x1993708, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2794
#11 0x00007ffb99c2aba1 in WebCore::RenderLayer::paintList (this=0x198cd68, list=0x1921210, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2977
#12 0x00007ffb99c2a89d in WebCore::RenderLayer::paintLayerContents (this=0x198cd68, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2933
#13 0x00007ffb99c29ebb in WebCore::RenderLayer::paintLayerContentsAndReflection (this=0x198cd68, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2813
#14 0x00007ffb99c29d98 in WebCore::RenderLayer::paintLayer (this=0x198cd68, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2794
#15 0x00007ffb99c2aba1 in WebCore::RenderLayer::paintList (this=0x198a4a8, list=0x1994f10, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2977
#16 0x00007ffb99c2a89d in WebCore::RenderLayer::paintLayerContents (this=0x198a4a8, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=224) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2933
#17 0x00007ffb99c29ebb in WebCore::RenderLayer::paintLayerContentsAndReflection (this=0x198a4a8, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=0) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2813
#18 0x00007ffb99c29d98 in WebCore::RenderLayer::paintLayer (this=0x198a4a8, rootLayer=0x198a4a8, context=0x7fff5bb12080, paintDirtyRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, 
    overlapTestRequests=0x7fff5bb11e10, paintFlags=0) at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2794
#19 0x00007ffb99c2913d in WebCore::RenderLayer::paint (this=0x198a4a8, context=0x7fff5bb12080, damageRect=..., paintBehavior=0, paintingRoot=0x0, region=0x0, paintFlags=0)
    at ../../../../Source/WebCore/rendering/RenderLayer.cpp:2611
#20 0x00007ffb99a001e4 in WebCore::FrameView::paintContents (this=0x1929a90, p=0x7fff5bb12080, rect=...) at ../../../../Source/WebCore/page/FrameView.cpp:2922
#21 0x00007ffb991c01ff in QWebFramePrivate::renderRelativeCoords (this=0x7ffb80017910, context=0x7fff5bb12080, layers=..., clip=...) at ../../../Source/WebKit/qt/Api/qwebframe.cpp:393
#22 0x00007ffb991c3a85 in QWebFrame::render (this=0x7ffb80015080, painter=0x7fff5bb12150, clip=...) at ../../../Source/WebKit/qt/Api/qwebframe.cpp:1293
#23 0x00007ffb991ecac8 in QWebView::paintEvent (this=0x7ffb80024070, ev=0x7fff5bb12960) at ../../../Source/WebKit/qt/Api/qwebview.cpp:811
#24 0x00007ffb96743146 in QWidget::event(QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#25 0x00007ffb991ec83c in QWebView::event (this=0x7ffb80024070, e=0x7fff5bb12960) at ../../../Source/WebKit/qt/Api/qwebview.cpp:715
#26 0x00007ffb966f2a74 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#27 0x00007ffb966f78f3 in QApplication::notify(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#28 0x00007ffb95e41ecc in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtCore.so.4
#29 0x00007ffb9673ede6 in QWidgetPrivate::drawWidget(QPaintDevice*, QRegion const&, QPoint const&, int, QPainter*, QWidgetBackingStore*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#30 0x00007ffb9690b84c in ?? () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#31 0x00007ffb96735b40 in QWidgetPrivate::syncBackingStore() () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#32 0x00007ffb9674365c in QWidget::event(QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#33 0x00007ffb96b18c7b in QMainWindow::event(QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#34 0x00007ffb966f2a74 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#35 0x00007ffb966f78f3 in QApplication::notify(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtGui.so.4
#36 0x00007ffb95e41ecc in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /home/robincao/src/qt/qt-4.8/lib/libQtCore.so.4
#37 0x00007ffb95e4576a in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /home/robincao/src/qt/qt-4.8/lib/libQtCore.so.4
#38 0x00007ffb95e70ac3 in ?? () from /home/robincao/src/qt/qt-4.8/lib/libQtCore.so.4
#39 0x00007ffb93318a5d in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#40 0x00007ffb93319258 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#41 0x00007ffb93319429 in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
---Type <return> to continue, or q <return> to quit---

[Robin Cao]

Created attachment 127186 [details]
proposed patch

[Ken Buchanan]

I knew there was a problem here, related to my patch in http://trac.webkit.org/changeset/104183/ but I didn't have a test case showing it clearly. 

Should this change be in RenderObject::container(), rather than markContainingBlocksForLayout()? I'm just wondering if this container() confusion might happen in other places.

[Eric Seidel (no email)]

Julien:  Could you look?  You've recently strolled through the anonymous block code and could comment if this looks reasonable?

[Julien Chaffraix]

Comment on attachment 127186 [details]
proposed patch

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

This is the wrong fix. RenderObject::container() should be patched not markContainingBlockForLayout().

The right fix (TM) is to find a way to merge container() and containingBlock() so that such bugs don't happen down the road again.

> Source/WebCore/ChangeLog:12
> +        This is a regression. r104183 changes containingBlock() so that it returns the container
> +        of an anonymous block for positioned objects, not the anonymous block itself. We should
> +        change markContainingBlocksForLayout() to match the change in containingBlock().

Your analysis is good but the conclusion is wrong. container() and containingBlock() should return the same result and here they don't.

[Robin Cao]

Thanks for all the comments below.

[Robin Cao]

(In reply to comment #6)
> This is the wrong fix. RenderObject::container() should be patched not markContainingBlockForLayout().
> 
> The right fix (TM) is to find a way to merge container() and containingBlock() so that such bugs don't happen down the road again.
> 
> > Source/WebCore/ChangeLog:12
> > +        This is a regression. r104183 changes containingBlock() so that it returns the container
> > +        of an anonymous block for positioned objects, not the anonymous block itself. We should
> > +        change markContainingBlocksForLayout() to match the change in containingBlock().
> 
> Your analysis is good but the conclusion is wrong. container() and containingBlock() should return the same result and here they don't.

I admit my patch may be incomplete, but seems we can not simply make container() and containingBlock() return the same result here.

[Julien Chaffraix]

> I admit my patch may be incomplete, but seems we can not simply make container() and containingBlock() return the same result here.

Could you get in the details as to why not? I fail to see what is preventing that from the top of my head.

[Julien Chaffraix]

(In reply to comment #9)
> > I admit my patch may be incomplete, but seems we can not simply make container() and containingBlock() return the same result here.
> 
> Could you get in the details as to why not? I fail to see what is preventing that from the top of my head.

Re-reading the code for another bug, the answer is written at the top of container(). A lot of the code assumes that actually container() has a different behavior than containingBlock(). So much for my unification dreams...

I will do an (hopefully better) review now but dhyatt@ should really be the one reviewing.

[Julien Chaffraix]

Comment on attachment 127186 [details]
proposed patch

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

>> Source/WebCore/ChangeLog:12
>> +        change markContainingBlocksForLayout() to match the change in containingBlock().
> 
> Your analysis is good but the conclusion is wrong. container() and containingBlock() should return the same result and here they don't.

Correcting myself: the conclusion is right. Sharing more code between the 2 functions is a separate issue even if it feels like markContainingBlocksForLayout() is embedding a safer alternative to containingBlock() build on top of container().

> Source/WebCore/rendering/RenderObject.cpp:630
>              bool willSkipRelativelyPositionedInlines = !object->isRenderBlock();

There is an issue here: |willSkipRelativelyPositionedInlines| will not be set if object->isAnonymousBlock() is true. This means that you would not update |container| below and actually still mark some of the anonymous blocks as needing layout.

It looks like this line should be:

bool willSkipRenderObjects = !object->isRenderBlock() || object->isAnonymousBlock();

> Source/WebCore/rendering/RenderObject.cpp:634
> +            while (object && (!object->isRenderBlock() || object->isAnonymousBlock()))
>                  object = object->container();

Noteworthy: this is not strictly equivalent to what containingBlock() is doing after r104183 (for example, we don't check if |object| is relatively positioned in the loop even if we mention it in the comment above). I wonder if that will bite us down the road.

[Ken Buchanan]

Robin: We're seeing some consequences of this bug in Chrome, and I'd like to get a fix in as soon as possible. Are you able to address Julien's comments?

If you're too busy to work on this I can take it over.

[Ken Buchanan]

(In reply to comment #11)
> > Source/WebCore/rendering/RenderObject.cpp:634
> > +            while (object && (!object->isRenderBlock() || object->isAnonymousBlock()))
> >                  object = object->container();
> 
> Noteworthy: this is not strictly equivalent to what containingBlock() is doing after r104183 (for example, we don't check if |object| is relatively positioned in the loop even if we mention it in the comment above). I wonder if that will bite us down the road.

dhyatt should comment on this. It seems to be enough that it is positioned and it is inline, though I'm not sure if we might be affecting the case where we have positioned blocks within anonymous blocks.

[Ken Buchanan]

Created attachment 128774 [details]
Patch

[Ken Buchanan]

I've tweaked Robin's patch based on Julien's comments. I'd like to get this worked out as soon as possible.

[Dave Hyatt]

Comment on attachment 128774 [details]
Patch

r=me, although this is pretty subtle. Right now this works because anonymous blocks can't ever be positioned. If they can, this code will have to be changed.

[WebKit Review Bot]

Comment on attachment 128774 [details]
Patch

Clearing flags on attachment: 128774

Committed r109011: <http://trac.webkit.org/changeset/109011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/10987709>