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

Issue 631052: Use-after-poison in blink::CompositorAnimationPlayer::NotifyAnimationStarted

Reported by ClusterFuzz, Jul 25 2016 Project Member

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5078527401787392

Fuzzer: attekett_dom_fuzzer
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Use-after-poison READ 8
Crash Address: 0x7e9287e2d1d0
Crash State:
  blink::CompositorAnimationPlayer::NotifyAnimationStarted
  cc::ElementAnimations::NotifyAnimationStarted
  cc::AnimationHost::SetAnimationEvents
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=376988:377035

Minimized Testcase (0.65 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95G5jt1IuF1YDuD6-CrBL5SrKpxEN-PTOXLjDhvuK_dm3UigX7E1fqGxmXvolSEtUYcgal2k-JJkPV37CRGE9-ZLYmm4V05osYCkBjQtFtxXxt2wk7dlWgfPdwa7YBK5Yx_aoMagti8ed66R7m0gO0b5h7-5Q?testcase_id=5078527401787392
<menu id="app-context-menu">
<script> 
function convertArrayToStrings(array){; return array};
var test0=document.getElementById("app-context-menu")
var test1=test0.appendChild(document.createElement("ul"))
var test3=test1.appendChild(document.createElement("cite"))
var test4=test0.appendChild(document.createElement("font"))
var test7=test3.appendChild(document.createElement("marquee"))

setInterval(function(){
test4.appendChild(test7.cloneNode());
})

test4.style.zoom=3.525269266217947


setTimeout(function(){
})
setTimeout(function(){
test7.style.zoom=3.525269266217947
})


setTimeout(function(){
})
document.body.style.zoom=3.5822747899219394


</script>


Filer: aarya

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 

Comment 1 by aarya@google.com, Jul 25 2016

Labels: Pri-1
Owner: ymalik@chromium.org
Status: Assigned (was: Untriaged)
regression from https://chromium.googlesource.com/chromium/src/+/8ce9d13097b1b0e14b9710b0b2670a510e258502

Comment 2 by kerrnel@chromium.org, Jul 25 2016

Components: Blink>Scroll

Comment 3 Deleted

Comment 4 by sheriffbot@chromium.org, Jul 26 2016

Project Member
Labels: M-52

Comment 5 by ymalik@chromium.org, Jul 26 2016

Debugging further, it looks like the CompositorAnimationPlayer is bogus to begin with. Looking into it further...

Comment 6 by ymalik@chromium.org, Jul 26 2016

Cc: -dstockwell@chromium.org ymalik@chromium.org
Owner: alancutter@chromium.org
The range in the bug is wrong. The crash is reproducible well before the oldest change in the range.

I did a manual bisect and got this narrow range:
https://chromium.googlesource.com/chromium/src/+log/29a24a77f2641ec45924093e06049ad03cdcaa5a..0c56dabf4895816405464b4955d4824c56e450a3

Reverting https://codereview.chromium.org/1698093005 fixes the crash.

Assigning to alancutter.

Comment 7 by alancutter@chromium.org, Jul 27 2016

Cc: vollick@chromium.org ajuma@chromium.org
Ran a (non-asan) debug build of content_shell and hit the crash site after a couple of minutes.
At frame 1 cc::ElementAnimations::NotifyPlayersAnimationStarted() it's iterating through players_list_ and the current value of node is 0x3636363636363636 which looks rather dodgy.

players_list_->head() and players_list_->end() look normal (0x35113e1c5c68 and 0x35113c64f390 respectively).



cc::AnimationPlayer inherits from base::RefCounted and base::LinkNode.
base::LinkNode has no destructor.
~AnimationPlayer() doesn't remove itself from a linked list if it's in one.
I believe this is the root cause of the crash, my patch probably just makes it more likely that AnimationPlayers are cleaned up.

Comment 8 by alancutter@chromium.org, Jul 27 2016

Looks like my theory is incorrect, AnimationPlayer is never in a linked list by the time ~AnimationPlayer() is called during this crash repro.

Comment 9 by alancutter@chromium.org, Jul 27 2016

Tried making cc::AnimationPlayer inherit from base::RefCountedThreadSafe since blink::CompositorAnimationPlayer has a scoped_refptr to it.
Didn't fix the crash but seems like something that should be done anyway.

Comment 10 by alancutter@chromium.org, Jul 27 2016

Small breakthrough, node seems to get deleted during the callback.

#3  0x00007ffff1266036 in cc::ElementAnimations::NotifyPlayersAnimationStarted (this=0x252cbf8126e0, 
    monotonic_time=19 days, 4:37:56.787160, target_property=cc::TargetProperty::TRANSFORM, group=99)
    at ../../cc/animation/element_animations.cc:1444
1444        DCHECK(node->next() != (void*)(0x3636363636363636));
(gdb) l
1439      for (PlayersListNode* node = players_list_->head();
1440           node != players_list_->end(); node = node->next(), count++) {
1441        DCHECK(node->next() != (void*)(0x3636363636363636));
1442        AnimationPlayer* player = node->value();
1443        player->NotifyAnimationStarted(monotonic_time, target_property, group);
1444        DCHECK(node->next() != (void*)(0x3636363636363636));
1445      }
1446    }
1447
1448    void ElementAnimations::NotifyPlayersAnimationFinished(

Notice it's the second DCHECK that fails, not the first.

Capturing next = node->next(); before calling the callback doesn't seem to fix the crash though but now node dereferences to 0x3636363636363636 instead of equalling it.

Comment 11 by alancutter@chromium.org, Jul 27 2016

Components: -Blink>Scroll Blink>Animation
Labels: Update-Daily

Comment 12 by alancutter@chromium.org, Jul 27 2016

Cc: -ymalik@chromium.org alancutter@chromium.org
Owner: ymalik@chromium.org
I believe the proper fix would be to replace the use of base::LinkedList with base::ObserverList however that's unlikely to be suitable for merging into stable.
A "quick fix" patch is not obvious to me here.

I'm out for the day, passing on to ymalik to take over.

Comment 13 by ajuma@chromium.org, Jul 27 2016

To summarize the current state, it sounds like we're now (more aggressively?) deleting animations during NotifyAnimationStarted, which (in this example) results in the currently visited node and its successor node getting deleted during NotifyAnimationStarted, which breaks the iteration in ElementAnimations::NotifyPlayersAnimationStarted since we're using a linked list.

Using base::ObserverList seems like the right fix to me too. I tried thinking of "quick fixes", but everything I could think of amounted to hackily re-implementing some of the functionality of ObserverList on top of a linked list, which actually seems riskier to me.

Comment 14 by ymalik@chromium.org, Jul 27 2016

Cc: jbroman@chromium.org
I agree that base::ObserverList is probably what we need. I also agree that this fix is unlikely suitable for merging into stable. FWIW, alancutter's change that made this crash more visible made it to M50, and it doesn't seem to be a real issue in practice.

Comment 15 by ymalik@chromium.org, Jul 27 2016

I verified that using base::ObserverList fixes the crash. Patch here: https://codereview.chromium.org/2189813002/

@allancutter can you verify that this indeed fixes the crash.

Comment 16 by alancutter@chromium.org, Jul 28 2016

Thanks for continuing the work here.

If my patch reverted cleanly then it would probably a good candidate for merging into stable. Unfortunately there are conflicts when reverting on ToT and since it's of a memory management nature I doubt it's safe to merge.

I'm unable to build https://codereview.chromium.org/2189813002/, animation_player_observer.h is missing from the upload.

Comment 17 by alancutter@chromium.org, Jul 28 2016

I made an attempt at a mergeable patch by taking refs of players_list_ before iteration but it didn't work, I'm not entirely sure why.
Posting it here for information sharing.
https://codereview.chromium.org/2187813003

Comment 18 by ymalik@chromium.org, Jul 28 2016

re #16, animation_player_observer.h was removed but I forgot to remove it from the includes. Please try again.

Comment 19 by jbroman@chromium.org, Jul 28 2016

#17: Re-reading the above, it sounded from above that the problem was removal from players_list_, rather than the player itself being freed. If so, taking a ref won't prevent it from being removed from players_list_.

(Aside: The unit test in ymalik's CL calls AnimationTimeline::DetachPlayer, which more calls ElementAnimations::RemovePlayer after some indirection, then destroys it. I think it's the ElementAnimations::RemovePlayer bit that's connected to the corruption of players_list_, rather than the destruction.)

Comment 20 by alancutter@chromium.org, Jul 28 2016

I'm still seeing the crash with https://codereview.chromium.org/2189813002/ applied running an ASAN build of content_shell.

    #0 0x7f5ba9fdd0d2 in ?? ./out/asan/../../third_party/WebKit/Source/platform/animation/CompositorAnimationPlayer.cpp:92:21
    #1 0x7f5bae43813e in NotifyPlayersAnimationFinished ./out/asan/../../cc/animation/element_animations.cc:1440:3
    #2 0x7f5bae421c6f in SetAnimationEvents ./out/asan/../../cc/animation/animation_host.cc:320:27
    #3 0x7f5bae757503 in SetAnimationEvents ./out/asan/../../cc/trees/layer_tree_host.cc:757:20
    #4 0x7f5bae83ce89 in SetAnimationEvents ./out/asan/../../cc/trees/proxy_main.cc:99:21

Comment 21 by ymalik@chromium.org, Jul 28 2016

I verified that my unittest crashes in the same way in the linked list case (without my patch). I didn't test my patch on asan actually. looking..

Comment 22 by alancutter@chromium.org, Jul 28 2016

#19: I also added a check for whether node is nullptr in the loop condition. If the current node gets removed then next will be nullptr and the loop should terminate. Probably not correct behaviour but shouldn't crash.

I believe the players_list_ is mutated by blink::Animation getting GC'd resulting in the following call chain:
=> ~Animation()
=> destroyCompositorPlayer()
=> detachCompositorTimeline()
=> timeline->playerDestroyed(*this)
=>  m_animationTimeline->DetachPlayer(client.compositorPlayer()->animationPlayer())

Comment 23 by ymalik@chromium.org, Jul 28 2016

Cc: -alancutter@chromium.org ymalik@chromium.org
Owner: alancutter@chromium.org
Yeah, it still crashes on ASAN with https://codereview.chromium.org/2189813002/. Not sure what's going on.

I'm OOO for the day. @alancutter, sending it your way.

Comment 24 by alancutter@chromium.org, Jul 28 2016

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/platform/heap/BlinkGCAPIReference.md#GarbageCollectedFinalized states that "finalization is done at an arbitrary time after the object becomes unreachable."

I believe that the blink::Animation object that owns the CompositorAnimationPlayer is being poisoned prior to ~Animation executing and getting a chance to free CompositorAnimationPlayer which in its destructor would remove the cc::AnimationPlayer from cc::ElementAnimations.

blink::Animations needs to use Oilpan's USING_PRE_FINALIZER() macro to ensure the handles to it from the impl thread get cleaned up before it gets poisoned.

Comment 25 by alancutter@chromium.org, Jul 28 2016

Created two line fix that no longer crashes ASAN content_shell, please confirm patch: https://codereview.chromium.org/2188623006


Other TODOs to do that came out of this investigation:
 - Use ObserverList for players_list_(https://codereview.chromium.org/2189813002/)
 - Make cc::AnimationPlayer inherit from RefCountedThreadSafe
 - Make blink::Animation's destructor virtual

Comment 26 by alancutter@chromium.org, Jul 28 2016

Cc: haraken@chromium.org

Comment 27 by ymalik@chromium.org, Jul 28 2016

Can confirm that the crash is gone on ASAN with https://codereview.chromium.org/2188623006.

Though, I get the following assertion failure on a debug build.

ASSERTION FAILED: !m_orderedPreFinalizers.contains(PreFinalizer(self, T::invokePreFinalizer))
../../third_party/WebKit/Source/platform/heap/ThreadState.h(403) : void blink::ThreadState::registerPreFinalizer(T *) [T = blink::Animation]

Received signal 11 SEGV_MAPERR 0000fbadbeef
#0 0x7fe005dafe5e base::debug::StackTrace::StackTrace()
#1 0x7fe005daf99f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fdff34a9330 <unknown>
#3 0x7fdfed682db7 blink::ThreadState::registerPreFinalizer<>()
#4 0x7fdfed67e3f5 blink::Animation::createCompositorPlayer()
#5 0x7fdfed67e21b blink::Animation::preCommit()
#6 0x7fdfed6db3b6 blink::CompositorPendingAnimations::update()
#7 0x7fdfed6e2e7e blink::DocumentAnimations::updateCompositorAnimations()
#8 0x7fdfee04022c blink::PaintLayerCompositor::updateIfNeededRecursiveInternal()
#9 0x7fdfee03ff3b blink::PaintLayerCompositor::updateIfNeededRecursive()

Comment 28 by bugdroid1@chromium.org, Jul 29 2016

Project Member

Comment 29 by alancutter@chromium.org, Jul 29 2016

Labels: Merge-Request-52 Merge-Request-53
Status: Fixed (was: Assigned)

Comment 30 by gov...@chromium.org, Jul 29 2016

Cc: awhalley@chromium.org
+ awhalley for M52 and M53 approval. Change listed at #28 is not yet baked in Canary.

Comment 31 by sheriffbot@chromium.org, Jul 29 2016

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

Comment 32 by dimu@chromium.org, Jul 30 2016

Labels: -Merge-Request-52 Merge-Review-52 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M52), manual review required.

Comment 33 by dimu@chromium.org, Jul 30 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)

Comment 34 by dimu@chromium.org, Jul 30 2016

Labels: -Merge-Request-52 Merge-Review-52 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M52), manual review required.

Comment 35 by ClusterFuzz, Jul 31 2016

Project Member
ClusterFuzz has detected this issue as fixed in range 408444:408557.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5078527401787392

Fuzzer: attekett_dom_fuzzer
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Use-after-poison READ 8
Crash Address: 0x7e9287e2d1d0
Crash State:
  blink::CompositorAnimationPlayer::NotifyAnimationStarted
  cc::ElementAnimations::NotifyAnimationStarted
  cc::AnimationHost::SetAnimationEvents
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=376988:377035
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_media&range=408444:408557

Minimized Testcase (0.65 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95G5jt1IuF1YDuD6-CrBL5SrKpxEN-PTOXLjDhvuK_dm3UigX7E1fqGxmXvolSEtUYcgal2k-JJkPV37CRGE9-ZLYmm4V05osYCkBjQtFtxXxt2wk7dlWgfPdwa7YBK5Yx_aoMagti8ed66R7m0gO0b5h7-5Q?testcase_id=5078527401787392
<menu id="app-context-menu">
<script> 
function convertArrayToStrings(array){; return array};
var test0=document.getElementById("app-context-menu")
var test1=test0.appendChild(document.createElement("ul"))
var test3=test1.appendChild(document.createElement("cite"))
var test4=test0.appendChild(document.createElement("font"))
var test7=test3.appendChild(document.createElement("marquee"))

setInterval(function(){
test4.appendChild(test7.cloneNode());
})

test4.style.zoom=3.525269266217947


setTimeout(function(){
})
setTimeout(function(){
test7.style.zoom=3.525269266217947
})


setTimeout(function(){
})
document.body.style.zoom=3.5822747899219394


</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Comment 36 by bugdroid1@chromium.org, Aug 1 2016

Project Member
Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f29a4298592591e73ba3a05b8244f4669d1d90e

commit 6f29a4298592591e73ba3a05b8244f4669d1d90e
Author: Alan Cutter <alancutter@chromium.org>
Date: Mon Aug 01 00:26:43 2016

Add pre finalizer to Animation to ensure compositor handles get cleaned up

BUG= 631052 

Review-Url: https://codereview.chromium.org/2188623006
Cr-Commit-Position: refs/heads/master@{#408554}
(cherry picked from commit 7fec064b3a37032605f091ba1ba52846c0e05824)

Review URL: https://codereview.chromium.org/2198723002 .

Cr-Commit-Position: refs/branch-heads/2785@{#422}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/6f29a4298592591e73ba3a05b8244f4669d1d90e/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/6f29a4298592591e73ba3a05b8244f4669d1d90e/third_party/WebKit/Source/core/animation/Animation.h

Comment 37 by awhalley@chromium.org, Aug 2 2016

Labels: -reward-topanel reward-unpaid reward-3500

Comment 38 by awhalley@chromium.org, Aug 2 2016

Labels: reward_to-attekett_at_gmail.com
Nice one, $3,500 for this bug.

Comment 39 by awhalley@chromium.org, Aug 4 2016

Labels: -reward-unpaid reward-inprocess

Comment 40 by bugdroid1@chromium.org, Aug 5 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66cb1320242db9ce2954a539dc2d5410bf033dbd

commit 66cb1320242db9ce2954a539dc2d5410bf033dbd
Author: ymalik <ymalik@chromium.org>
Date: Fri Aug 05 20:01:07 2016

ElementAnimations should hold an ObservableList of AnimationPlayers.

Before this, the AnimationPlayers were held in a linked list, and
modifying the list while iterating could cause undefined behavior.

BUG= 631052 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2189813002
Cr-Commit-Position: refs/heads/master@{#410152}

[modify] https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd/cc/animation/animation_player.h
[modify] https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd/cc/animation/element_animations.cc
[modify] https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd/cc/animation/element_animations.h
[modify] https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd/cc/animation/element_animations_unittest.cc
[modify] https://crrev.com/66cb1320242db9ce2954a539dc2d5410bf033dbd/cc/test/animation_timelines_test_common.cc

Comment 41 by awhalley@chromium.org, Aug 10 2016

Labels: -Hotlist-Merge-Approved

Comment 42 by awhalley@chromium.org, Aug 30 2016

Labels: -M-52 M-53 Release-0-M53

Comment 43 by awhalley@chromium.org, Sep 14 2016

Labels: CVE-2016-5153

Comment 44 by sheriffbot@chromium.org, Nov 4 2016

Project Member
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

Comment 45 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment