New issue
Advanced search Search tips

Issue 859294 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Security



Sign in to add a comment

Heap-use-after-free in blink::PaintController::FinishCycle

Project Member Reported by ClusterFuzz, Jun 30 2018

Issue description

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

Fuzzer: bj_broddelwerk
Job Type: windows_asan_chrome_no_sandbox
Platform Id: windows

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x122ff317e000
Crash State:
  blink::PaintController::FinishCycle
  blink::GraphicsLayer::PaintRecursively
  blink::LocalFrameView::PaintTree
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=571248:571253

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Jun 30 2018

Components: Blink>Internals Blink>Paint
Labels: Test-Predator-Auto-Components
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 2 by ClusterFuzz, Jun 30 2018

Labels: Test-Predator-Auto-Owner
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/f0498e2265442349ed2b837c09631fb3b9eb237c ([CI] Remove cache generation mechanism of DisplayItemClient).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Project Member

Comment 3 by sheriffbot@chromium.org, Jun 30 2018

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by sheriffbot@chromium.org, Jun 30 2018

Labels: Pri-1
Cc: wangxianzhu@chromium.org
Components: -Blink>Internals Blink>SVG Blink>Compositing
Owner: chrishtr@chromium.org
The root cause is mismatching hierarchies of PaintLayer and compositing. 
The DOM tree is like:
  <svg style="backface-visibility: hidden">
    <foreignObject>
      some inline boxes
    </foreignObject>
  </svg>

In the PaintLayer tree, the layers for the svg root and the foreign object are siblings:
 layer for HTML
  normal flow list
   layer for the svg
  positive z-order list
   layer for the foreignObject
while in the GraphicsLayer tree, the foreignObject is painted on the GraphicsLayer of the svg.

When the contents of the foreignObject changes, we mark the painting layer ancestors for NeedsRepaint, and miss the svg which is not in the painting layer ancestor path. In the next paint, the svg's GraphicsLayer is repainted. Then during FinishCycle() when we iterate the DisplayItemClients we access some deleted DisplayItemClients under the foreignObject.

I can change GraphicsLayer to call FinishCycle() for real repainted layers only to avoid the heap-use-after-free for the test case. That won't fix the under-invalidation though.

chrishtr@ can you look into the under-invalidation issue?
Project Member

Comment 6 by sheriffbot@chromium.org, Jul 1

Labels: M-69 Target-69
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 2

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

commit e71a3846080ff6cc758f39c2fea5c5a903572e61
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Mon Jul 02 01:16:45 2018

[PE] Don't call PaintController::FinishCycle() if not repainted

This is mainly a performance optimization because we don't need
FinishCycle() if PaintController has nothing changed.

By the way it can also avoid the immediate bad effect (but not the
root cause) of  bug 859294 .

Bug:  859294 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iad3e14ee303c08185922d3d6e495c3b25ead5434
Reviewed-on: https://chromium-review.googlesource.com/1121783
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571803}
[modify] https://crrev.com/e71a3846080ff6cc758f39c2fea5c5a903572e61/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/e71a3846080ff6cc758f39c2fea5c5a903572e61/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/e71a3846080ff6cc758f39c2fea5c5a903572e61/third_party/blink/renderer/platform/graphics/graphics_layer.cc

Project Member

Comment 8 by ClusterFuzz, Jul 2

ClusterFuzz has detected this issue as fixed in range 571802:571803.

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

Fuzzer: bj_broddelwerk
Job Type: windows_asan_chrome_no_sandbox
Platform Id: windows

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x122ff317e000
Crash State:
  blink::PaintController::FinishCycle
  blink::GraphicsLayer::PaintRecursively
  blink::LocalFrameView::PaintTree
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=571248:571253
Fixed: https://clusterfuzz.com/revisions?job=windows_asan_chrome_no_sandbox&range=571802:571803

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

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 9 by ClusterFuzz, Jul 2

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

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

Comment 10 by sheriffbot@chromium.org, Jul 2

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 3

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

commit 74780299ea1b72a9f8b6415943213c0cf0f7ee23
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Jul 03 18:40:12 2018

Revert "[PE] Don't call PaintController::FinishCycle() if not repainted"

This reverts commit e71a3846080ff6cc758f39c2fea5c5a903572e61.

Reason for revert: Caused performance regressions because changed
paint properties are not cleared changed flags.

Original change's description:
> [PE] Don't call PaintController::FinishCycle() if not repainted
> 
> This is mainly a performance optimization because we don't need
> FinishCycle() if PaintController has nothing changed.
> 
> By the way it can also avoid the immediate bad effect (but not the
> root cause) of  bug 859294 .
> 
> Bug:  859294 
> Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: Iad3e14ee303c08185922d3d6e495c3b25ead5434
> Reviewed-on: https://chromium-review.googlesource.com/1121783
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#571803}

TBR=wangxianzhu@chromium.org,chrishtr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  859294 ,  859885 
Change-Id: I2a4d2a7d09cc3bf5edfbf0c6dee275581030c2c9
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1124701
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572302}
[modify] https://crrev.com/74780299ea1b72a9f8b6415943213c0cf0f7ee23/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/74780299ea1b72a9f8b6415943213c0cf0f7ee23/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/74780299ea1b72a9f8b6415943213c0cf0f7ee23/third_party/blink/renderer/platform/graphics/graphics_layer.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 3

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

commit d7209bb8a53bcc9636b2c12f18e7f7b74b0c02c8
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Jul 03 22:34:13 2018

[PE] Don't validate DisplayItemClients if PaintController changed nothing

This is mainly a performance optimization because we don't need
FinishCycle() if PaintController has nothing changed.

By the way it can also avoid the immediate bad effect (but not the
root cause) of  bug 859294 .

Bug:  859294 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3a4d26253fef2b04adc805ef94b93c5479c88759
Reviewed-on: https://chromium-review.googlesource.com/1124980
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572376}
[modify] https://crrev.com/d7209bb8a53bcc9636b2c12f18e7f7b74b0c02c8/third_party/blink/renderer/core/paint/paint_and_raster_invalidation_test.cc
[modify] https://crrev.com/d7209bb8a53bcc9636b2c12f18e7f7b74b0c02c8/third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
[modify] https://crrev.com/d7209bb8a53bcc9636b2c12f18e7f7b74b0c02c8/third_party/blink/renderer/platform/graphics/paint/paint_controller.h

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 4

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

commit 5c301712a67bbe70026048a224b4608628199e12
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Wed Jul 04 01:20:53 2018

Fix CompositingContainer for replaced normal-flow stacking contexts.

For such PaintLayers, the CompositingContainer is the parent, not the
containing stacking context. This is because such stacking contexts
paint during the normal-flow of the ancestor layout object.

This also allows us to enable subsequence caching; the previous attempt
last week was still suffering from the under-invalidation reported
in  issue 859520 .

The testcases that crash on ASAN from the referenced bugs are fixed;
one such example is included in this CL.

Bug:859294, 719835 ,723076, 859520 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6ab5271d70bd834482c22b89f09b93907788ed0c
Reviewed-on: https://chromium-review.googlesource.com/1125269
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572428}
[add] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/WebKit/LayoutTests/svg/foreignObject/foreignObject-position-crash.html
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/compositing/graphics_layer_updater.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_invalidator.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer.h
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer_test.cc

Labels: -ReleaseBlock-Stable
Project Member

Comment 15 by sheriffbot@chromium.org, Oct 8

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment