Potential NULL-pointer vulnerability in [RenderLayer::updateLayerPosition]

[Alexey Utkin]

Potential vulnerability in the method    
void RenderLayer::updateLayerPosition()
(file http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayer.cpp)

Code fragment (lines 660-676)

    if (!renderer()->isPositioned() && renderer()->parent()) {
        // We must adjust our position by walking up the render tree looking for the
        // nearest enclosing object with a layer.
        RenderObject* curr = renderer()->parent();
        while (curr && !curr->hasLayer()) {
            if (curr->isBox() && !curr->isTableRow()) {
                // Rows and cells share the same coordinate space (that of the section).
                // Omit them when computing our xpos/ypos.
                localPoint += toRenderBox(curr)->locationOffsetIncludingFlipping();
            }
            curr = curr->parent();
        }
        if (curr->isBox() && curr->isTableRow()) { // <--- here the [curr] var can has a NULL value: check the [while] condition.
            // Put ourselves into the row coordinate space.
            localPoint -= toRenderBox(curr)->locationOffsetIncludingFlipping();
        }
    }

has NULL-pointer vulnerability.

In our port we have a GPF on transfer from 
    http://www.gismeteo.ru
to (click on St.Petersburg city)
    http://www.gismeteo.ru/city/daily/4079/  

Seems the problem has a relation with 
https://bugs.webkit.org/show_bug.cgi?id=62120

[Alexey Utkin]

Stack trace:
WebCore::RenderLayer::updateLayerPosition()  Line 697	C++
WebCore::RenderLayer::updateLayerPositions(unsigned int flags, WebCore::IntPoint * cachedOffset)  Line 279	C++
WebCore::FrameView::layout(bool allowSubtree)  Line 974	C++
WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()  Line 2452	C++
WebCore::FocusController::setActive(bool active)  Line 419	C++ //active==false
<ProcessFocusEvent>

[Alexey Utkin]

Suggested fix in line 672 
(file http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayer.cpp):

-if (curr->isBox() && curr->isTableRow()) {
+if (curr && curr->isBox() && curr->isTableRow()) {

[Jae Hyun Park]

Created attachment 169084 [details]
Patch

[Adam Barth]

Comment on attachment 169084 [details]
Patch

Can you write a test that triggers this issue?

[Alexey Utkin]

(In reply to comment #4)
> (From update of attachment 169084 [details])
> Can you write a test that triggers this issue?

Seems that is possible in custom ports only (JavaFX port as an example - the scenario was described in bug synopsis). That happen in slow message queue.

Any way that is a bug form static code analyzer - it have to be fixed.

[Julien Chaffraix]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 169084 [details] [details])
> > Can you write a test that triggers this issue?
> 
> Seems that is possible in custom ports only (JavaFX port as an example - the scenario was described in bug synopsis). That happen in slow message queue.

If the bug requires a custom port, it is possibly not a WebCore issue.

> Any way that is a bug form static code analyzer - it have to be fixed.

No, it doesn't *have to* (see http://lists.webkit.org/pipermail/webkit-dev/2012-April/020365.html). I am not convinced the change is right as updateLayerPosition should be called only on an attached tree, which means that |curr| cannot be NULL as we are not called on the RenderView (renderer()->parent()) and the RenderView always has a RenderLayer.

[Alexey Utkin]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 169084 [details] [details] [details])
> > > Can you write a test that triggers this issue?
> > 
> > Seems that is possible in custom ports only (JavaFX port as an example - the scenario was described in bug synopsis). That happen in slow message queue.
> 
> If the bug requires a custom port, it is possibly not a WebCore issue.
> 
> > Any way that is a bug form static code analyzer - it have to be fixed.
> 
> No, it doesn't *have to* (see http://lists.webkit.org/pipermail/webkit-dev/2012-April/020365.html). I am not convinced the change is right as updateLayerPosition should be called only on an attached tree, which means that |curr| cannot be NULL as we are not called on the RenderView (renderer()->parent()) and the RenderView always has a RenderLayer.

Well, I am not a code owner or contributer. Code patching is out of competition. The only thing that I see is a code inconsistency. It could be not a WebCore issue, but code structure was contradictory. To be consistence the "while" circle need to be reduced to something like
    while (!curr->hasLayer())
, or insert an assertion about |curr| before 
    if (curr->isBox() && curr->isTableRow()) {
line, or accept the patch and report your objection in upper level function.
That is my IMHO.

[Jae Hyun Park]

(In reply to comment #7)
> Well, I am not a code owner or contributer. Code patching is out of competition. The only thing that I see is a code inconsistency. It could be not a WebCore issue, but code structure was contradictory. To be consistence the "while" circle need to be reduced to something like
>     while (!curr->hasLayer())
> , or insert an assertion about |curr| before 
>     if (curr->isBox() && curr->isTableRow()) {
> line, or accept the patch and report your objection in upper level function.
> That is my IMHO.

I agree. The main reason I created this patch was because of that inconsistency.

[Deepak Mittal]

I think adding check for curr in 

if (curr->isBox() && curr->isTableRow())  is required for consistency , as while loop will break when either curr is NULL or curr does not have the layer..

in first case crash will happen in 

if (curr->isBox() && curr->isTableRow()) ..