Bug 65316

Summary: Potential NULL-pointer vulnerability in [RenderLayer::updateLayerPosition]
Product: WebKit Reporter: Alexey Utkin <alexey.utkin>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: abarth, ap, deepak.m1, eric, jaepark, jchaffraix, webkit.review.bot
Priority: P3 Keywords: EasyFix, NeedsReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.gismeteo.ru/city/daily/4079/
Attachments:
Description Flags
Patch abarth: review-

Description Alexey Utkin 2011-07-28 07:28:46 PDT
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
Comment 1 Alexey Utkin 2011-08-05 03:30:37 PDT
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>
Comment 2 Alexey Utkin 2011-08-05 03:38:13 PDT
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()) {
Comment 3 Jae Hyun Park 2012-10-16 21:36:45 PDT
Created attachment 169084 [details]
Patch
Comment 4 Adam Barth 2012-10-17 16:01:28 PDT
Comment on attachment 169084 [details]
Patch

Can you write a test that triggers this issue?
Comment 5 Alexey Utkin 2012-10-18 01:30:17 PDT
(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.
Comment 6 Julien Chaffraix 2012-10-18 09:58:00 PDT
(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.
Comment 7 Alexey Utkin 2012-10-19 02:19:28 PDT
(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.
Comment 8 Jae Hyun Park 2012-10-19 07:12:54 PDT
(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.
Comment 9 Deepak Mittal 2014-02-17 07:06:55 PST
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()) ..