New issue
Advanced search Search tips

Issue 779377 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-11-16
OS: Linux , Android , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::ThemePainter::PaintBorderOnly

Project Member Reported by ClusterFuzz, Oct 29 2017

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5207788967165952

Fuzzer: inferno_layout_test_unmodified
Job Type: mac_asan_content_shell
Platform Id: mac

Crash Type: Null-dereference READ
Crash Address: 0x000000000020
Crash State:
  blink::ThemePainter::PaintBorderOnly
  blink::BoxPainter::PaintBoxDecorationBackgroundWithRect
  blink::BoxPainter::PaintBoxDecorationBackground
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=506280:506298

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5207788967165952

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Oct 29 2017

Labels: OS-Linux
Project Member

Comment 2 by ClusterFuzz, Oct 29 2017

Components: Blink>Paint
Labels: Test-Predator-AutoComponents
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 3 by ClusterFuzz, Oct 29 2017

Labels: Test-Predator-AutoOwner
Owner: e...@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/3e05d98d5234fd322d796e3b49d68da725a9c868 (Break LayoutTheme dependency on LayoutObject).

If this is incorrect, please remove the owner and apply the Test-Predator-Wrong-CLs label.
Labels: -Pri-1 Pri-2
Owner: schenney@chromium.org
Null reads that do not (often) result in user crashes are not P1.
Project Member

Comment 5 by ClusterFuzz, Nov 1 2017

Labels: OS-Windows
Labels: Test-Predator-Auto-CC
Labels: -Test-Predator-AutoComponents Test-Predator-Auto-Components
Labels: -Test-Predator-AutoOwner Test-Predator-Auto-Owner
Labels: -Test-Predator-Auto-CC

Comment 10 by e...@chromium.org, Nov 13 2017

Cc: e...@chromium.org
 Issue 777212  has been merged into this issue.
Project Member

Comment 11 by ClusterFuzz, Nov 13 2017

Labels: OS-Android
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1

commit a1c0004ec1034fa715ded22620f3b7c7b3cef6d1
Author: Stephen Chenney <schenney@chromium.org>
Date: Tue Nov 14 03:35:08 2017

Avoid null Node pointers in theme painting code

A recent change to reduce the dependence of theme
code on layout objects changed painting code to take Node
objects instead of LayoutObjects. However, Node can be null
for anonymous layout objects, and the theme code crashes
when accessing this null node in numerous places.

As part of this patch, all of the uses of Node in the theme
painting code were audited to identify potential null accesses.
The sites affected have been changed to either check for null
or take a Document as an argument to avoid the node access.

R=eae@chromium.org
BUG= 782817 , 779377 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I409ea5f16f462d959cf2864b7019ca9ca0bf22c2
Reviewed-on: https://chromium-review.googlesource.com/764447
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516175}
[add] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-border-painting-expected.txt
[add] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-border-painting.html
[add] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-menulist-painting-expected.txt
[add] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-menulist-painting.html
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/BoxPainter.cpp
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainter.cpp
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainter.h
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainterDefault.cpp
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainterDefault.h
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainterMac.h
[modify] https://crrev.com/a1c0004ec1034fa715ded22620f3b7c7b3cef6d1/third_party/WebKit/Source/core/paint/ThemePainterMac.mm

Status: Fixed (was: Assigned)
Labels: Merge-Request-63
Status: Assigned (was: Fixed)
The patch that introduced these crashes in Theme Painting code landed in 63.0.3233.0 and will likely cause a spike in crashes in the wild should it go into stable. Requesting merge now before the M-63 branch hits stable.
Project Member

Comment 15 by sheriffbot@chromium.org, Nov 14 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge?

Any other imp details to justify the merge.


NextAction: 2017-11-16
The change has significant auto testing coverage, particularly through clusterfuzz. It has not baked enough yet, so the plan is to merge later in the week for the next Beta release.

Next action to verify Canary results from patch.
The NextAction date has arrived: 2017-11-16
Please verify in Canary and let us know.
No crashes in theme code for recent Canary. This is safe to merge into M-63, I believe.
Labels: -Merge-Review-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comments #14, #17 and #20. Please merge ASAP. Thank you.
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 16 2017

Labels: -merge-approved-63 merge-merged-3239
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/444367bed510036efd5684ba1bb31746c3449cbe

commit 444367bed510036efd5684ba1bb31746c3449cbe
Author: Stephen Chenney <schenney@chromium.org>
Date: Thu Nov 16 20:29:52 2017

Avoid null Node pointers in theme painting code

M-63 Merge.

A recent change to reduce the dependence of theme
code on layout objects changed painting code to take Node
objects instead of LayoutObjects. However, Node can be null
for anonymous layout objects, and the theme code crashes
when accessing this null node in numerous places.

As part of this patch, all of the uses of Node in the theme
painting code were audited to identify potential null accesses.
The sites affected have been changed to either check for null
or take a Document as an argument to avoid the node access.

TBR=​eae@chromium.org
BUG= 782817 , 779377 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I409ea5f16f462d959cf2864b7019ca9ca0bf22c2
Reviewed-on: https://chromium-review.googlesource.com/764447
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#516175}(cherry picked from commit a1c0004ec1034fa715ded22620f3b7c7b3cef6d1)
Reviewed-on: https://chromium-review.googlesource.com/775001
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#522}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[add] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-border-painting-expected.txt
[add] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-border-painting.html
[add] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-menulist-painting-expected.txt
[add] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/LayoutTests/paint/theme/anonymous-element-menulist-painting.html
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/BoxPainter.cpp
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainter.cpp
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainter.h
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainterDefault.cpp
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainterDefault.h
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainterMac.h
[modify] https://crrev.com/444367bed510036efd5684ba1bb31746c3449cbe/third_party/WebKit/Source/core/paint/ThemePainterMac.mm

Project Member

Comment 23 by ClusterFuzz, Nov 18 2017

ClusterFuzz has detected this issue as fixed in range 514498:517702.

Detailed report: https://clusterfuzz.com/testcase?key=5207788967165952

Fuzzer: inferno_layout_test_unmodified
Job Type: mac_asan_content_shell
Platform Id: mac

Crash Type: Null-dereference READ
Crash Address: 0x000000000020
Crash State:
  blink::ThemePainter::PaintBorderOnly
  blink::BoxPainter::PaintBoxDecorationBackgroundWithRect
  blink::BoxPainter::PaintBoxDecorationBackground
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=506280:506298
Fixed: https://clusterfuzz.com/revisions?job=mac_asan_content_shell&range=514498:517702

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5207788967165952

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 24 by ClusterFuzz, Nov 18 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 6653089754120192 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Sign in to add a comment