New issue
Advanced search Search tips

Issue 870196 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Aug 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug

Blocking:
issue 757440



Sign in to add a comment

Null-dereference READ in std::__1::pair<WTF::KeyValuePair<std::__1::pair<blink::WeakMember<blink::SVGElem

Project Member Reported by ClusterFuzz, Aug 2

Issue description

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

Fuzzer: bj_broddelwerk
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000004
Crash State:
  std::__1::pair<WTF::KeyValuePair<std::__1::pair<blink::WeakMember<blink::SVGElem
  blink::SMILTimeContainer::Unschedule
  blink::SVGSMILElement::WillChangeAnimationTarget
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=579745:579746

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Aug 2

Components: Blink>SVG
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, Aug 2

Labels: Test-Predator-Auto-Owner
Owner: keishi@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/d92ab8be365f7c52d1ead03f2d2f3a75510957f3 (Oilpan: Enable incremental marking by default).

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.
Cc: mlippautz@chromium.org
Components: Blink>MemoryAllocator>GarbageCollection
IncrementalMarking related crash
Blocking: 757440
fyi: This one still reproduces for me with ToT 4115f382069fb5694a0d01c45acfd92001bce79a.

out/Release/content_shell --enable-logging=stderr --vmodule=*/heap/*=3 ~/Downloads/clusterfuzz-testcase-6367840530333696.html

I do observe that we initialize and delete a bucket that we later on find during iteration. The interesting part here is that the entry is completely empty (WeakMember == nullptr, QualiefiedName::ptr_ == nullptr) but we still find it during iteration.
And I can reproduce it without turning on incremental marking. This might just be flushed out now...

out/Debug/content_shell --single-process --js-flags="--predictable" --enable-logging=stderr --vmodule=*/heap/*=3 ~/Downloads/clusterfuzz-testcase-6367840530333696.html

Project Member

Comment 8 by ClusterFuzz, Aug 6

Labels: OS-Mac
I was still on an old commit where IM was enabled. Continuing digging...
Project Member

Comment 10 by ClusterFuzz, Aug 7

ClusterFuzz has detected this issue as fixed in range 580848:580849.

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

Fuzzer: bj_broddelwerk
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000004
Crash State:
  std::__1::pair<WTF::KeyValuePair<std::__1::pair<blink::WeakMember<blink::SVGElem
  blink::SMILTimeContainer::Unschedule
  blink::SVGSMILElement::WillChangeAnimationTarget
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=579745:579746
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=580848:580849

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

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 11 by ClusterFuzz, Aug 7

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Alright, this is what is going on here:

1. Mutator is populating SMILTimeContainer::scheduled_animations_ [1]
2. Incremental marking starts
3. Marker visits the container, and registers slots for weak callbacks in the keys
4. Mutator calls Unschedule [2] which erases an entry (deletes a bucket)
4.1 This means that we write "deleted value" into the WeakMember
5. Incremental marking finalizes
6. Weak callback fires and sees the deleted value
6.1 Weak callback overwrites the deleted value with the cleared value, effectively reviving the key. [3]
7. Iterators suddenly find deleted bucket and all sorts of things go wrong.

Bug is obviously in step 6.1. Solution is simple: The weak callbacks need to preserve the deletion value.

I already have a fix prepared locally which I will upload asap.

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/animation/smil_time_container.h?sq=package:chromium&g=0&l=142
[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/animation/smil_time_container.cc?sq=package:chromium&g=0&l=111
[3] https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/heap/heap.h?sq=package:chromium&g=0&l=686
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 7

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

commit 1638e067d7dccd748c024dcdf763a63a943c9656
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Tue Aug 07 15:58:10 2018

[heap] Do not revive WeakMember during incremental marking

Overwriting deleted values with cleared values potentially revives
buckets in collections. For atomic garbage collections this is not a
problem as those values are filered out in the visitors already. For
incremental garbage collections the deleted values may be introduced by
erasing a value from a container after it has been visited by the
incremental marker.

Bug:  chromium:870196 ,  chromium:757440 
Change-Id: I1d628f00c2e2ea6e36e880d50a397dcbcc8566d1
Reviewed-on: https://chromium-review.googlesource.com/1164950
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581238}
[modify] https://crrev.com/1638e067d7dccd748c024dcdf763a63a943c9656/third_party/blink/renderer/platform/heap/heap.h
[modify] https://crrev.com/1638e067d7dccd748c024dcdf763a63a943c9656/third_party/blink/renderer/platform/heap/incremental_marking_test.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 7

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

commit 460c4724a3221dfb0b438fa619b41ec4022d2d38
Author: Fredrik Hubinette <hubbe@chromium.org>
Date: Tue Aug 07 18:04:18 2018

Revert "[heap] Do not revive WeakMember during incremental marking"

This reverts commit 1638e067d7dccd748c024dcdf763a63a943c9656.

Reason for revert: New tests are flaky and causing problems when submitting code.

Original change's description:
> [heap] Do not revive WeakMember during incremental marking
> 
> Overwriting deleted values with cleared values potentially revives
> buckets in collections. For atomic garbage collections this is not a
> problem as those values are filered out in the visitors already. For
> incremental garbage collections the deleted values may be introduced by
> erasing a value from a container after it has been visited by the
> incremental marker.
> 
> Bug:  chromium:870196 ,  chromium:757440 
> Change-Id: I1d628f00c2e2ea6e36e880d50a397dcbcc8566d1
> Reviewed-on: https://chromium-review.googlesource.com/1164950
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Keishi Hattori <keishi@chromium.org>
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#581238}

TBR=haraken@chromium.org,keishi@chromium.org,mlippautz@chromium.org

Change-Id: I632b5594d137acd37b4639ebcd46f8d61638dbc0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:870196 ,  chromium:757440 
Reviewed-on: https://chromium-review.googlesource.com/1165328
Reviewed-by: Fredrik Hubinette <hubbe@chromium.org>
Commit-Queue: Fredrik Hubinette <hubbe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581277}
[modify] https://crrev.com/460c4724a3221dfb0b438fa619b41ec4022d2d38/third_party/blink/renderer/platform/heap/heap.h
[modify] https://crrev.com/460c4724a3221dfb0b438fa619b41ec4022d2d38/third_party/blink/renderer/platform/heap/incremental_marking_test.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 8

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

commit 3972c07ca0793f1f0a8d84ed79b43bd1faf42ea1
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Wed Aug 08 07:33:18 2018

Reland "[heap] Do not revive WeakMember during incremental marking"

Overwriting deleted values with cleared values potentially revives
buckets in collections. For atomic garbage collections this is not a
problem as those values are filered out in the visitors already. For
incremental garbage collections the deleted values may be introduced by
erasing a value from a container after it has been visited by the
incremental marker.

This reverts commit 460c4724a3221dfb0b438fa619b41ec4022d2d38.

Bug:  chromium:870196 ,  chromium:757440 
Change-Id: I2d4c173a2dc94b3b8615699f81b85e1b61c4c705
Reviewed-on: https://chromium-review.googlesource.com/1165556
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581494}
[modify] https://crrev.com/3972c07ca0793f1f0a8d84ed79b43bd1faf42ea1/third_party/blink/renderer/platform/heap/heap.h
[modify] https://crrev.com/3972c07ca0793f1f0a8d84ed79b43bd1faf42ea1/third_party/blink/renderer/platform/heap/incremental_marking_test.cc

Sign in to add a comment