Null-dereference READ in blink::TransitionInterpolation::TransitionInterpolation |
|||||||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=5116895427297280 Fuzzer: miaubiz_css_fuzzer Job Type: linux_msan_chrome Platform Id: linux Crash Type: Null-dereference READ Crash Address: 0x000000000000 Crash State: blink::TransitionInterpolation::TransitionInterpolation blink::TransitionInterpolation::Create blink::TransitionKeyframe::PropertySpecificKeyframe::CreateInterpolation Sanitizer: memory (MSAN) Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5116895427297280 Issue filed automatically. See https://github.com/google/clusterfuzz-tools for more information.
,
Jun 20 2018
Rob, could you take a look at this crash please?
,
Jun 20 2018
,
Jun 28 2018
Digging into the failure it seems to happen when we start a transition on filter for an incompatible filter type while another is in progress. Our code seems to make the incorrect assumption that we can transition from an existing saturate filter value to a contrast filter. Since these are incompatible the start interpolable value is null and trying to clone that crashes. I came up with a simpler and 100% reproducible test case and bisected it: https://chromium.googlesource.com/chromium/src/+log/d374c7111b60395ad0a715f5e1c0ddbb41c802af..d4360a5709faa46ea332fca1e9ccf1804051d566 Which points at a skia roll https://skia.googlesource.com/skia.git/+log/f1bc5e8b7837..5dac9b3b5bf7 This seems a bit odd since I would have imagined the problem is in blink but I'm continuing to investigate.
,
Jun 29 2018
The bisect seems to have been a red herring, the test subsequently passed on the same build. Similarly when I did a local bisect it pointed me at a different build and then started passing after that. This seems like it must have been an issue for a long time, including in current stable. Removing Release-Block. For example, if I look for similar crashes (with % to match webkit|blink): https://crash.corp.google.com/browse?q=EXISTS+%28SELECT+1+FROM+UNNEST%28CrashedStackTrace.StackFrame%29+WHERE+FunctionName+LIKE+%27%25%3A%3ATransitionInterpolation%3A%3ACreate%28%25%29%27%29#-property-selector,productname:1000,-magicsignature:50,-magicsignature2:50,-stablesignature:50,-magicsignaturesorted:50 I can find crashes dating back as far as M61, and one in each of M60 and M59.
,
Jul 3
Thoughts: InterpolationValue::Clone() doesn not copy the NonInterpolableValue, instead it takes a reference to it. Very bad design in retrospect. It's possible that a NonInterpolableList of a Clone()d filter value is being mutated resulting in the original seeing a different list of filter types. This might explain the complicated repro case and the flakiness. I haven't managed to see any further evidence that this is the case though, the CSSFilterListInterpolationType doesn't appear to mutate existing NIVs and extra DCHECKS that the InterpolableLists and NonInterpolableLists share the same length pass. Still, I don't feel like I've ruled out the possibility that the non-cloning of NIVs is a possible root cause.
,
Jul 3
I'm not sure I follow, the line that is crashing is: cached_interpolable_value_(merge_.start_interpolable_value->Clone()) https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/animation/transition_interpolation.h?dr=CSs&q=file:transition_interpolation.h+cached_interpolable_value_%5C(&sq=package:chromium&g=0&l=80 Since start_interpolable_value_ is an InterpolableValue with a virtual Clone method a nullptr will crash, no? But more high level, it seems like we expect TransitionInterpolation to be constructed with incompatible InterpolationValues, given the checks in MaybeMergeSingles, so I'm guessing cached_interpolable_value_ should just accept that start_interpolable_value_ may be null and "copy" a nullptr.
,
Jul 4
Looks like in normal cases, MaybeMergeSingles is tested before starting a TransitionInterpolation in blink::CSSAnimations::CalculateTransitionUpdateForProperty: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/animation/css/css_animations.cc?type=cs&q=file:css_animations.cc+MaybeMergeSingles&g=0&l=734 In the crashing case, the transition keyframes are constructed by blink::CSSAnimations::MaybeApplyPendingUpdate as part of retargeted_compositor_transitions. Perhaps this needs to apply a similar check and then immediately finish or cancel the new transition if the retargeting results in non-interpolable keyframes.
,
Jul 4
I think I understand what triggers this. Start a transition, wait until animations have been updated but not started running, and then start an incompatible transition. The second transition should never be started but I think it isn't aware of the first transition's incompatible filter until we later try to retarget the running transition. I put together a simpler repro - attached.
,
Jul 4
I put together a patch which fixes the test cases - just working on a test and verifying correctness: https://chromium-review.googlesource.com/c/chromium/src/+/1126519 I believe this should fix the underlying issue - ensuring that we check the retargeted start value when deciding whether to start a new smooth interpolation.
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6562517fa6e3a6d3a9690f36d3582e2fd23fa63a commit 6562517fa6e3a6d3a9690f36d3582e2fd23fa63a Author: Robert Flack <flackr@chromium.org> Date: Tue Jul 10 23:23:24 2018 Use retargeted start point when starting composited transitions. When starting transitions we check if they are interpolable, and don't start a TransitionInterpolation if not. When retargeting, the start keyframe from the previously active interpolation may cause an incompatible interpolation so we must take this into account. Bug: 853698 Change-Id: Iaeeba35c64c659818c68d30e8f999e704b382926 Reviewed-on: https://chromium-review.googlesource.com/1126519 Commit-Queue: Robert Flack <flackr@chromium.org> Reviewed-by: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#573980} [modify] https://crrev.com/6562517fa6e3a6d3a9690f36d3582e2fd23fa63a/third_party/blink/renderer/core/BUILD.gn [modify] https://crrev.com/6562517fa6e3a6d3a9690f36d3582e2fd23fa63a/third_party/blink/renderer/core/animation/css/css_animations.cc [add] https://crrev.com/6562517fa6e3a6d3a9690f36d3582e2fd23fa63a/third_party/blink/renderer/core/animation/css/css_animations_test.cc
,
Jul 10
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/426a53b7a3cf88a3c24a61d14346a6815ee2b2d4 commit 426a53b7a3cf88a3c24a61d14346a6815ee2b2d4 Author: Dirk Pranke <dpranke@chromium.org> Date: Wed Jul 11 01:50:31 2018 Revert "Use retargeted start point when starting composited transitions." This reverts commit 6562517fa6e3a6d3a9690f36d3582e2fd23fa63a. Reason for revert: I think this has an invalid cast and is causing CFI failures, see https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20CFI/9012 . Original change's description: > Use retargeted start point when starting composited transitions. > > When starting transitions we check if they are interpolable, and don't > start a TransitionInterpolation if not. When retargeting, the start > keyframe from the previously active interpolation may cause an > incompatible interpolation so we must take this into account. > > Bug: 853698 > Change-Id: Iaeeba35c64c659818c68d30e8f999e704b382926 > Reviewed-on: https://chromium-review.googlesource.com/1126519 > Commit-Queue: Robert Flack <flackr@chromium.org> > Reviewed-by: Majid Valipour <majidvp@chromium.org> > Cr-Commit-Position: refs/heads/master@{#573980} TBR=flackr@chromium.org,majidvp@chromium.org Change-Id: I4de74036835b09da2fe4751280bea9d9b0e205a0 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 853698 Reviewed-on: https://chromium-review.googlesource.com/1132016 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Dirk Pranke <dpranke@chromium.org> Cr-Commit-Position: refs/heads/master@{#574034} [modify] https://crrev.com/426a53b7a3cf88a3c24a61d14346a6815ee2b2d4/third_party/blink/renderer/core/BUILD.gn [modify] https://crrev.com/426a53b7a3cf88a3c24a61d14346a6815ee2b2d4/third_party/blink/renderer/core/animation/css/css_animations.cc [delete] https://crrev.com/775a3bbc9b1a2237d4f6acdd1e8d1febfb382c47/third_party/blink/renderer/core/animation/css/css_animations_test.cc
,
Jul 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7af4e4c6f0e60ee19e76add9bd5fc2d936d731bb commit 7af4e4c6f0e60ee19e76add9bd5fc2d936d731bb Author: Robert Flack <flackr@chromium.org> Date: Wed Jul 11 15:25:37 2018 Reland "Use retargeted start point when starting composited transitions." This reverts commit 426a53b7a3cf88a3c24a61d14346a6815ee2b2d4. Original review at https://chromium-review.googlesource.com/c/chromium/src/+/1126519 TBR=majidvp@chromium.org Bug: 853698 Change-Id: I8dce9db88ee4af68d5cf93e89ac15963f39f6da0 Reviewed-on: https://chromium-review.googlesource.com/1133125 Commit-Queue: Robert Flack <flackr@chromium.org> Reviewed-by: Robert Flack <flackr@chromium.org> Cr-Commit-Position: refs/heads/master@{#574170} [modify] https://crrev.com/7af4e4c6f0e60ee19e76add9bd5fc2d936d731bb/third_party/blink/renderer/core/BUILD.gn [modify] https://crrev.com/7af4e4c6f0e60ee19e76add9bd5fc2d936d731bb/third_party/blink/renderer/core/animation/css/css_animations.cc [add] https://crrev.com/7af4e4c6f0e60ee19e76add9bd5fc2d936d731bb/third_party/blink/renderer/core/animation/css/css_animations_test.cc
,
Jul 12
ClusterFuzz has detected this issue as fixed in range 574169:574170. Detailed report: https://clusterfuzz.com/testcase?key=5116895427297280 Fuzzer: miaubiz_css_fuzzer Job Type: linux_msan_chrome Platform Id: linux Crash Type: Null-dereference READ Crash Address: 0x000000000000 Crash State: blink::TransitionInterpolation::TransitionInterpolation blink::TransitionInterpolation::Create blink::TransitionKeyframe::PropertySpecificKeyframe::CreateInterpolation Sanitizer: memory (MSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_msan_chrome&range=567917:567918 Fixed: https://clusterfuzz.com/revisions?job=linux_msan_chrome&range=574169:574170 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5116895427297280 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.
,
Jul 12
ClusterFuzz testcase 5116895427297280 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 |
|||||||
Comment 1 by brajkumar@chromium.org
, Jun 19 2018Components: Blink>Animation
Labels: M-69 Test-Predator-Wrong CF-NeedsTriage