New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment
Heap-use-after-free in media::Pipeline::StateTransitionTask
Project Member Reported by ClusterFuzz, Apr 20 2014 Back to list
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5202346583785472

Fuzzer: Media_source_fuzzer
Job Type: Linux_asan_chrome_media

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x617000007bc0
Crash State:
  - crash stack -
  media::Pipeline::StateTransitionTask
  base::MessageLoop::RunTask
  - free stack -
  blink::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl
  WebCore::HTMLMediaElement::createMediaPlayer
  


Additional requirements: Requires HTTP
 
Cc: dalecur...@chromium.org acolwell@chromium.org
Owner: scherkus@chromium.org
Status: Assigned
this race condition (crash and free on different threads) is hitting every now and then.

READ of size 4 at 0x617000007bc0 thread T5 (Media)
    #0 0x7f93a91bfb05 in media::Pipeline::StateTransitionTask(media::PipelineStatus) src/media/base/pipeline.cc:365
    #1 0x7f939e61774e in base::MessageLoop::RunTask(base::PendingTask const&) src/base/callback.h:401
freed by thread T0 (chrome) here:
    #0 0x7f939d15d44b in operator delete(void*) _asan_rtl_
    #1 0x7f93a1489709 in blink::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl() src/third_party/WebKit/Source/wtf/OwnPtrCommon.h:52
    #2 0x7f93a1bbb4dc in WebCore::HTMLMediaElement::createMediaPlayer() src/third_party/WebKit/Source/wtf/OwnPtrCommon.h:52
Project Member Comment 2 by ClusterFuzz, Apr 20 2014
Labels: Pri-1
I wonder if this is the same crash:

https://crash.corp.google.com/samples?stbtiq=b477e235b54b7879

We've got a few of these per version.
Project Member Comment 4 by ClusterFuzz, Apr 22 2014
Labels: Missing_Impact-1
Project Member Comment 5 by ClusterFuzz, Apr 24 2014
Labels: -Missing_Impact-1 Missing_Impact-2
Comment 6 by kenrb@chromium.org, Apr 24 2014
scherkus@: Are you able to take a look at this one?
Project Member Comment 7 by ClusterFuzz, Apr 26 2014
Labels: -Missing_Impact-2 Missing_Impact-3
Project Member Comment 8 by ClusterFuzz, Apr 28 2014
Labels: Nag
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 9 by ClusterFuzz, Apr 30 2014
Labels: -Missing_Impact-3 Missing_Impact-4
Project Member Comment 10 by ClusterFuzz, May 13 2014
Labels: -Missing_Impact-4 Missing_Impact-11
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 11 by ClusterFuzz, May 14 2014
Labels: -Missing_Impact-11 Missing_Impact-12
Labels: -Missing_Impact-12 Security_Impact-Stable Security_Impact-Beta
Project Member Comment 13 by ClusterFuzz, May 22 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 14 by ClusterFuzz, May 30 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 15 by ClusterFuzz, Jun 7 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 16 by ClusterFuzz, Jun 16 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Cc: timwillis@chromium.org
scherkus@ - can you please provide an update on this bug as well? (That said, please address  Issue 356540  first though as it has a higher security severity).
somewhat on my radar ... I'm making some promising changes in this area, which could help fix this
Roger that - Thanks. Is there a rough ETA you could provide for the implementation of said promising changes? :) 
Project Member Comment 20 by ClusterFuzz, Jul 2 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
bump - scherkus@ re: c#19?
Project Member Comment 22 by ClusterFuzz, Jul 10 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Labels: M-37
Project Member Comment 24 by ClusterFuzz, Jul 20 2014
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 25 by ClusterFuzz, Jul 28 2014
Labels: -Security_Impact-Beta
scherkus@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member Comment 26 by ClusterFuzz, Jul 29 2014
Labels: Security_Impact-Beta
Project Member Comment 27 by ClusterFuzz, Jul 29 2014
Labels: -Security_Impact-Beta
Project Member Comment 28 by ClusterFuzz, Jul 29 2014
Labels: Security_Impact-Beta
Project Member Comment 29 by ClusterFuzz, Jul 29 2014
Labels: -Security_Impact-Beta
Project Member Comment 30 by ClusterFuzz, Jul 30 2014
Labels: -Security_Impact-Stable Security_Impact-Beta
Labels: -Security_Impact-Beta Security_Impact-Stable
 Issue 400354  has been merged into this issue.
Cc: cloudfuz...@gmail.com
Andrew, please look at the reliable repro at  crbug.com/400354 . This bug is also found externally and this bug has stayed open for a long time, can you please try to prioritize fixing this.
Cc: xhw...@chromium.org
xhwang: do you think your WIP CLs will help address this issue as well?

it seems awfully similar to a lot of the types of crashes you've been fixing
scherkus: Yep, this should be fixed by my CL (hopefully). But to be safe, I'd like to run the repro steps in  crbug.com/400354 . Can you add me to that bug as well? (Can't access that now.)
done
Project Member Comment 37 by bugdroid1@chromium.org, Aug 6 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61949f628a7da3d98c6a4913eac026abcf6037ca

commit 61949f628a7da3d98c6a4913eac026abcf6037ca
Author: xhwang@chromium.org <xhwang@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed Aug 06 05:46:06 2014

Pipeline: Use WeakPtr for DemuxerHost Calls and Tasks.

There is a chance that the Demuxer calls host_->OnDemuxerError() right
before Stop() is called. The Pipeline always posts a ErrorChangedTask() for
OnDemuxerError() with base::Retained(this).

After the Demuxer fires the stop callback, the Pipeline could be destroyed
immediately. So If the media thread hasn't been destroyed we could end up with
running ErrorChangedTask() on null pipeline which causes a crash.

This CL uses a weak pointer for DemuxerHost calls so that no task will run
after the pipeline is destroyed.

BUG= 397656 ,  399417 ,  365141 
TEST=Updated unit tests to cover this case.
R=scherkus@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287687 0039d316-1c4b-4281-b951-d872f2087c98


Project Member Comment 38 by bugdroid1@chromium.org, Aug 6 2014
------------------------------------------------------------------
r287687 | xhwang@chromium.org | 2014-08-06T05:46:06.310655Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/base/pipeline_unittest.cc?r1=287687&r2=287686&pathrev=287687
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/base/pipeline.cc?r1=287687&r2=287686&pathrev=287687

Pipeline: Use WeakPtr for DemuxerHost Calls and Tasks.

There is a chance that the Demuxer calls host_->OnDemuxerError() right
before Stop() is called. The Pipeline always posts a ErrorChangedTask() for
OnDemuxerError() with base::Retained(this).

After the Demuxer fires the stop callback, the Pipeline could be destroyed
immediately. So If the media thread hasn't been destroyed we could end up with
running ErrorChangedTask() on null pipeline which causes a crash.

This CL uses a weak pointer for DemuxerHost calls so that no task will run
after the pipeline is destroyed.

BUG= 397656 ,  399417 ,  365141 
TEST=Updated unit tests to cover this case.
R=scherkus@chromium.org

Review URL: https://codereview.chromium.org/423073012
-----------------------------------------------------------------
Status: Fixed
I tested the fix with the repro steps in  crbug.com/400354  for a full of 30 minutes and couldn't observe any crash. So I'll mark this issue as fixed. If this ever show up again feel free to reopen.
Project Member Comment 40 by ClusterFuzz, Aug 9 2014
Labels: -Restrict-View-SecurityTeam Merge-Triage M-36 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Labels: -M-37 -Merge-Triage -M-36 Release-0-M38
Skipping merge based on medium severity and race condition requirement.
Labels: Merge-NA
Project Member Comment 43 by ClusterFuzz, Nov 12 2014
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member Comment 44 by sheriffbot@chromium.org, Oct 1 2016
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
Project Member Comment 45 by sheriffbot@chromium.org, Oct 2 2016
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
Labels: allpublic
Sign in to add a comment