New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 853698 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jul 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::TransitionInterpolation::TransitionInterpolation

Project Member Reported by ClusterFuzz, Jun 18 2018

Issue description

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)

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: brajkumar@chromium.org
Components: Blink>Animation
Labels: M-69 Test-Predator-Wrong CF-NeedsTriage
Unable to find actual suspect through code search and also observing no suspected CL's under regression range, hence adding appropriate label and requesting someone from blink team to look in to this issue.

Thanks!
Labels: -Pri-1 ReleaseBlock-Beta Pri-2
Owner: flackr@chromium.org
Rob, could you take a look at this crash please?
Status: Assigned (was: Untriaged)

Comment 4 by flackr@chromium.org, Jun 28 2018

Status: Started (was: Assigned)
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.
test.html
1.1 KB View Download

Comment 5 by flackr@chromium.org, Jun 29 2018

Labels: -ReleaseBlock-Beta
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.
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.
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.
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.
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.
simple-repro.html
543 bytes View Download
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.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by ClusterFuzz, 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.
Project Member

Comment 16 by ClusterFuzz, Jul 12

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
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