65316 2011-07-28 07:28:46 -0700 Potential NULL-pointer vulnerability in [RenderLayer::updateLayerPosition] 2024-08-05 18:43:13 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All UNCONFIRMED http://www.gismeteo.ru/city/daily/4079/ EasyFix, GoodFirstBug, NeedsReduction P3 Normal --- 0 alexey.utkin webkit-unassigned abarth ap deepak.m1 eric jaepark jchaffraix webkit.review.bot oldest_to_newest 443480 0 alexey.utkin 2011-07-28 07:28:46 -0700 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 447141 1 alexey.utkin 2011-08-05 03:30:37 -0700 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> 447145 2 alexey.utkin 2011-08-05 03:38:13 -0700 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()) { 743836 3 169084 jaepark 2012-10-16 21:36:45 -0700 Created attachment 169084 Patch 744736 4 169084 abarth 2012-10-17 16:01:28 -0700 Comment on attachment 169084 Patch Can you write a test that triggers this issue? 745079 5 alexey.utkin 2012-10-18 01:30:17 -0700 (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. 745339 6 jchaffraix 2012-10-18 09:58:00 -0700 (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. 746153 7 alexey.utkin 2012-10-19 02:19:28 -0700 (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. 746287 8 jaepark 2012-10-19 07:12:54 -0700 (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. 981433 9 deepak.m1 2014-02-17 07:06:55 -0800 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()) .. 169084 2012-10-16 21:36:45 -0700 2012-10-17 16:01:27 -0700 Patch bug-65316-20121017133553.patch text/plain 1629 jaepark U3VidmVyc2lvbiBSZXZpc2lvbjogMTMxNTI2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTc5MTIyMDE1MWU0N2Ix YTc0N2I4ZDFhNGRmNjU2MTliYWI4OTM0My4uOWY4YTAyYTgxZjNhMDFhNWIyMjFmOGVhMDdiZTIx YjYzZDNjYjkyZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTEwLTE2ICBKYWUg SHl1biBQYXJrICA8amFlLnBhcmtAY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgUG90ZW50aWFs IE5VTEwtcG9pbnRlciB2dWxuZXJhYmlsaXR5IGluIFtSZW5kZXJMYXllcjo6dXBkYXRlTGF5ZXJQ b3NpdGlvbl0KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTY1MzE2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg VGhlcmUgY2FuIGJlIGEgcG90ZW50aWFsIE5VTEwtcG9pbnRlciB2dWxuZXJhYmlsaXR5IHNpbmNl IGl0IGFjY2Vzc2VzCisgICAgICAgIGN1cnIgZGlyZWN0bHkgd2l0aG91dCBjaGVja2luZyB0byBz ZWUgaWYgaXQgZXhpc3RzIGZpcnN0LiBUaGlzIHBhdGNoCisgICAgICAgIHByZXZlbnRzIHN1Y2gg TlVMTC1wb2ludGVyIHZ1bG5lcmFiaWxpdHkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIChObyBi ZWhhdmlvciBjaGFuZ2UpLgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckxheWVyOjp1cGRhdGVMYXllclBvc2l0aW9uKToKKwog MjAxMi0xMC0xNiAgTWljaGFlbCBTYWJvZmYgIDxtc2Fib2ZmQGFwcGxlLmNvbT4KIAogICAgICAg ICBDaGFuZ2UgV1RGX1VTRV84QklUX1RFWFRSVU4gdG8gRU5BQkxFXzhCSVRfVEVYVFJVTgpkaWZm IC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAKaW5kZXggMWU2YTM3OTk1NTc1YWI3 MWQxYTAwM2QyYzk1MTg4Mjk1YjkwNmIzNi4uZDFlM2FkNjNjMmFmMTdkYjgyZGZkNzVlYjM0M2Yz NjBiMzQ1YzU5OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxh eWVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCkBA IC04NTcsNyArODU3LDcgQEAgdm9pZCBSZW5kZXJMYXllcjo6dXBkYXRlTGF5ZXJQb3NpdGlvbigp CiAgICAgICAgICAgICB9CiAgICAgICAgICAgICBjdXJyID0gY3Vyci0+cGFyZW50KCk7CiAgICAg ICAgIH0KLSAgICAgICAgaWYgKGN1cnItPmlzQm94KCkgJiYgY3Vyci0+aXNUYWJsZVJvdygpKSB7 CisgICAgICAgIGlmIChjdXJyICYmIGN1cnItPmlzQm94KCkgJiYgY3Vyci0+aXNUYWJsZVJvdygp KSB7CiAgICAgICAgICAgICAvLyBQdXQgb3Vyc2VsdmVzIGludG8gdGhlIHJvdyBjb29yZGluYXRl IHNwYWNlLgogICAgICAgICAgICAgbG9jYWxQb2ludCAtPSB0b1JlbmRlckJveChjdXJyKS0+dG9w TGVmdExvY2F0aW9uT2Zmc2V0KCk7CiAgICAgICAgIH0K