Null-dereference READ in std::__1::pair<WTF::KeyValuePair<std::__1::pair<blink::WeakMember<blink::SVGElem |
||||||
Issue descriptionDetailed 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.
,
Aug 2
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.
,
Aug 2
IncrementalMarking related crash
,
Aug 3
,
Aug 6
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
,
Aug 6
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.
,
Aug 6
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
,
Aug 6
,
Aug 7
I was still on an old commit where IM was enabled. Continuing digging...
,
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.
,
Aug 7
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.
,
Aug 7
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
,
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
,
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
,
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 |
||||||
Comment 1 by ClusterFuzz
, Aug 2Labels: Test-Predator-Auto-Components