Summary: | Potential NULL-pointer vulnerability in [RenderLayer::updateLayerPosition] | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Utkin <alexey.utkin> | ||||
Component: | Layout and Rendering | Assignee: | 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
Alexey Utkin
2011-07-28 07:28:46 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> 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()) { Created attachment 169084 [details]
Patch
Comment on attachment 169084 [details]
Patch
Can you write a test that triggers this issue?
(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. (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. (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. (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. 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()) .. |