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

Issue 174150 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2013
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment

Crashing in media::VideoRendererBase::ThreadMain()

Project Member Reported by scherkus@chromium.org, Feb 4 2013

Issue description

Version: 26.0.1397.2

I'm confused by this one. Maybe because VideoRendererBase only has a PlatformThreadId and not an actual Thread object we're not stopping the thread and doing a use-after-free on the condition variable!?

It happens at a very low frequency compared to the rest of our crashes. I might sprinkle the code with some CHECK()s to track down what the heck is happening...


EXCEPTION_ACCESS_VIOLATION_WRITE @ 0x00000014
0x77418c39	 [ntdll.dll]	 + 0x00038c39]	RtlpWaitOnCriticalSection
0x77418b47	 [ntdll.dll]	 + 0x00038b47]	RtlpDeCommitFreeBlock
0x774878e2	 [ntdll.dll]	 + 0x000a78e2]	RtlSleepConditionVariableCS
0x75044d92	 [kernel32.dll]	 + 0x00094d92]	SleepConditionVariableCS
0x6bed3588	 [chrome.dll]	 - condition_variable_win.cc:106]	base::WinVistaCondVar::TimedWait(base::TimeDelta const &)
0x6d5c2235	 [chrome.dll]	 - video_renderer_base.cc:274]	media::VideoRendererBase::ThreadMain()
0x6bb8726f	 [chrome.dll]	 - platform_thread_win.cc:57]	base::`anonymous namespace'::ThreadFunc(void *)
0x74fc3676	 [kernel32.dll]	 + 0x00013676]	BaseThreadInitThunk
0x77419d71	 [ntdll.dll]	 + 0x00039d71]	__RtlUserThreadStart
0x77419d44	 [ntdll.dll]	 + 0x00039d44]	_RtlUserThreadStart


media/filters/video_renderer_base.cc:
272     // Remain idle as long as we're not playing.
273     if (state_ != kPlaying || playback_rate_ == 0) {
274       frame_available_.TimedWait(kIdleTimeDelta);  // *** BOOM ***
275       continue;
276     }
277 
278     // Remain idle until we have the next frame ready for rendering.
279     if (ready_frames_.empty()) {
280       frame_available_.TimedWait(kIdleTimeDelta);
281       continue;
282     }

Crash IDs:
3a07d14d0e39e1ba
410f6badee1b705f
6e4b1a85856474b6
7eda07734224ea92
89fcabca19b43fa3
8f390e199b7a4390
924ce2355de734f2
9bf1a2b4568b27cd
dafb4c7220d9d295
 

Comment 1 by cdn@chromium.org, Feb 4 2013

Labels: Restrict-View-SecurityTeam
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 5 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=180573

------------------------------------------------------------------------
r180573 | scherkus@chromium.org | 2013-02-05T00:22:12.592600Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/video_renderer_base.cc?r1=180573&r2=180572&pathrev=180573

Sprinkle some CHECK()s in media::VideoRendererBase to help track down a crash.

BUG= 174150 

Review URL: https://codereview.chromium.org/12179023
------------------------------------------------------------------------
Labels: -Mstone-26 Mstone-27
No crash reports yet in 26.0.1406.0 or 26.0.1407.0 canary.

Moving to Mstone-27 seeing as we'll have to wait for a dev channel release to collect more data.
Labels: -Mstone-27 Mstone-24 SecImpacts-Stable SecImpacts-Beta SecSeverity-High
The bug seem to affecting stable branches as well. Based on https://crash.corp.google.com/search?query=product%3A%22Chrome%22+crashed_thread_function_name%3A%22media%3A%3AVideoRendererBase%3A%3AThreadMain%28%29%22, i am fixing the labels.
Labels: -Area-WebKit -Feature-Media Area-Internals Feature-Media-Video
Labels: -Mstone-24 Mstone-25
moving m24 bugs to m25.
Looks like my CHECK() has picked up some crashes:

 46 VideoRendererBase::~VideoRendererBase() {
 47   base::AutoLock auto_lock(lock_);
 48   CHECK(state_ == kUninitialized || state_ == kStopped) << state_;  // *** BOOM ***
 49   CHECK_EQ(thread_, base::kNullThreadHandle);
 50 }

Crash IDs:
0773ca9531b324fb
0953cbf741894fe8
3a4f04bd9ac29f7e
46b3382c22be54e2
4b320ce89461178b
6098e298cc424ca1
89fa7977652af9aa
a2a135be166d614c
a67d4e11091a6116
b0eefa57fcb7a0e1
bcb256d3ce98699b
cb4164af144567d5
d4d676ab422c13e0
f0b8940da8b822f3
ff5dc9a0e462a915

Stack Trace:
0x63768ec9	 [chrome.dll]	 - debugger_win.cc:107]	base::debug::BreakDebugger()
0x64beaa71	 [chrome.dll]	 - video_renderer_base.cc:48]	media::VideoRendererBase::~VideoRendererBase()
0x64beb130	 [chrome.dll]	 + 0x01a4b130]	media::VideoRendererBase::`scalar deleting destructor'(unsigned int)
0x631af037	 [chrome.dll]	 - scoped_ptr.h:239]	base::internal::scoped_ptr_impl<cc::ScrollbarAnimationController,base::DefaultDeleter<cc::ScrollbarAnimationController> >::reset(cc::ScrollbarAnimationController *)
0x64be41e3	 [chrome.dll]	 - pipeline.cc:656]	media::Pipeline::OnStopCompleted(media::PipelineStatus)
0x634857ff	 [chrome.dll]	 - bind_internal.h:1228]	base::internal::Invoker<1,base::internal::BindState<base::internal::RunnableAdapter<void ( WebDataService::*)(WDTypedResult const *)>,void (WebDataService *,WDTypedResult const *),void (base::internal::UnretainedWrapper<WebDataService>)>,void (WebDataService *,WDTypedResult const *)>::Run(base::internal::BindStateBase *,WDTypedResult const * const &)
0x64bfe3ae	 [chrome.dll]	 - serial_runner.cc:81]	media::SerialRunner::RunNextInSeries(media::PipelineStatus)
0x6372912b	 [chrome.dll]	 - bind_internal.h:911]	base::internal::InvokeHelper<1,void,base::internal::RunnableAdapter<void ( content::PepperUDPSocketPrivateHost::*)(int)>,void (base::WeakPtr<content::PepperUDPSocketPrivateHost> const &,int const &)>::MakeItSo(base::internal::RunnableAdapter<void ( content::PepperUDPSocketPrivateHost::*)(int)>,base::WeakPtr<content::PepperUDPSocketPrivateHost> const &,int const &)
0x645e73a9	 [chrome.dll]	 - bind_internal.h:1228]	base::internal::Invoker<1,base::internal::BindState<base::internal::RunnableAdapter<void ( google_apis::OperationRunner::*)(google_apis::AuthenticatedOperationInterface *)>,void (google_apis::OperationRunner *,google_apis::AuthenticatedOperationInterface *),void (base::WeakPtr<google_apis::OperationRunner>)>,void (google_apis::OperationRunner *,google_apis::AuthenticatedOperationInterface *)>::Run(base::internal::BindStateBase *,google_apis::AuthenticatedOperationInterface * const &)
0x636a86eb	 [chrome.dll]	 - bind_internal.h:1173]	base::internal::Invoker<1,base::internal::BindState<base::Callback<void (media::Decryptor *)>,void (media::Decryptor *),void (media::Decryptor *)>,void (media::Decryptor *)>::Run(base::internal::BindStateBase *)
0x631d5188	 [chrome.dll]	 - message_loop.cc:476]	MessageLoop::RunTask(base::PendingTask const &)
0x631d4ea4	 [chrome.dll]	 - message_loop.cc:671]	MessageLoop::DoWork()
0x631d5364	 [chrome.dll]	 - message_pump_default.cc:29]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x631d4b04	 [chrome.dll]	 - message_loop.cc:433]	MessageLoop::RunInternal()
0x631d4a5c	 [chrome.dll]	 - run_loop.cc:45]	base::RunLoop::Run()
0x631d7da0	 [chrome.dll]	 - thread.cc:152]	base::Thread::Run(MessageLoop *)
0x631d7732	 [chrome.dll]	 - thread.cc:197]	base::Thread::ThreadMain()
0x631d7626	 [chrome.dll]	 - platform_thread_win.cc:57]	base::`anonymous namespace'::ThreadFunc(void *)
0x761233a9	 [kernel32.dll]	 + 0x000133a9]	BaseThreadInitThunk
0x77e59ef1	 [ntdll.dll]	 + 0x00039ef1]	__RtlUserThreadStart
0x77e59ec4	 [ntdll.dll]	 + 0x00039ec4]	_RtlUserThreadStart



Looks like it's a race during WebMediaPlayerImpl destruction:
0x77dcf8b1	 [ntdll.dll]	 + 0x0001f8b1]	NtWaitForSingleObject
0x767a149c	 [KERNELBASE.dll]	 + 0x0001149c]	WaitForSingleObjectEx
0x76cb1193	 [kernel32.dll]	 + 0x00011193]	WaitForSingleObjectExImplementation
0x76cb1147	 [kernel32.dll]	 + 0x00011147]	WaitForSingleObject
0x6a95b6b0	 [chrome.dll]	 - waitable_event_win.cc:52]	base::WaitableEvent::Wait()
0x6bb4c08f	 [chrome.dll]	 - webmediaplayer_impl.cc:1222]	webkit_media::WebMediaPlayerImpl::Destroy()
0x6bb4c133	 [chrome.dll]	 - webmediaplayer_impl.cc:213]	webkit_media::WebMediaPlayerImpl::~WebMediaPlayerImpl()
0x6bb4d32d	 [chrome.dll]	 + 0x0140d32d]	webkit_media::WebMediaPlayerImpl::`scalar deleting destructor'(unsigned int)
0x6babe014	 [chrome.dll]	 - webmediaplayerclientimpl.cpp:110]	WebKit::WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
0x6babe090	 [chrome.dll]	 + 0x0137e090]	WebKit::WebMediaPlayerClientImpl::`vector deleting destructor'(unsigned int)
0x6afb3ad0	 [chrome.dll]	 - mediaplayer.cpp:365]	WebCore::MediaPlayer::~MediaPlayer()
0x6a7833df	 [chrome.dll]	 - stringimpl.cpp:128]	WTF::StringImpl::~StringImpl()
0x6afb3a11	 [chrome.dll]	 + 0x00873a11]	WebCore::MediaPlayer::`scalar deleting destructor'(unsigned int)



Going to take a look at a crash dump to see what the value of state_ is.
Status: Started
Here's the disassembly for ~VRB()

667eaa5c e82b825cfe      call    chrome_64da0000!base::internal::LockImpl::Lock (64db2c8c)
667eaa61 8b4660          mov     eax,dword ptr [esi+60h]
667eaa64 85c0            test    eax,eax
667eaa66 740a            je      chrome_64da0000!media::VideoRendererBase::~VideoRendererBase+0x32 (667eaa72)
667eaa68 83f809          cmp     eax,9
667eaa6b 7405            je      chrome_64da0000!media::VideoRendererBase::~VideoRendererBase+0x32 (667eaa72)
667eaa6d e847e4b7fe      call    chrome_64da0000!base::debug::BreakDebugger (65368eb9)
667eaa72 837e6400        cmp     dword ptr [esi+64h],0
667eaa76 7405            je      chrome_64da0000!media::VideoRendererBase::~VideoRendererBase+0x3d (667eaa7d)
667eaa78 e83ce4b7fe      call    chrome_64da0000!base::debug::BreakDebugger (65368eb9)
667eaa7d 8d4e1c          lea     ecx,[esi+1Ch]
667eaa80 e831825cfe      call    chrome_64da0000!base::internal::LockImpl::Unlock (64db2cb6)

If my WinDbg-fu is correct, eax contains state_ before we break on 667eaa6d. Turns out the value is 5 which corresponds to kFlushed.

Other fun facts: VRB::state_ is loaded from @esi+60h (see instruction @ 667eaa61). Conveniently enough, VRB::thread_ (which is a PlatformThreadHandle, which on Windows is a HANDLE) is located right after state_ and has a non-NULL value (!!!)

This indeed confirms my suspicion that we are destroying VRB as the thread is still running. Eek.
Man I need to keep a WinDbg cheat sheet around. The dt command confirms this:

dt media::VideoRendererBase @esi
chrome_64da0000!media::VideoRendererBase
   +0x000 __VFN_table : 0x67022288 
   +0x004 __VFN_table : 0x67022280 
   +0x008 message_loop_    : scoped_refptr<base::MessageLoopProxy>
   +0x00c weak_factory_    : base::WeakPtrFactory<media::VideoRendererBase>
   +0x014 weak_this_       : base::WeakPtr<media::VideoRendererBase>
   +0x01c lock_            : base::Lock
   +0x034 set_decryptor_ready_cb_ : base::Callback<void __cdecl(base::Callback<void __cdecl(media::Decryptor *)> const &)>
   +0x03c decoder_         : scoped_refptr<media::VideoDecoder>
   +0x040 decrypting_demuxer_stream_ : scoped_refptr<media::DecryptingDemuxerStream>
   +0x044 ready_frames_    : std::deque<scoped_refptr<media::VideoFrame>,std::allocator<scoped_refptr<media::VideoFrame> > >
   +0x05c frame_available_ : base::ConditionVariable
   +0x060 state_           : 5 ( kFlushed )
   +0x064 thread_          : 0x0000023c Void
   +0x068 pending_read_    : 0
   +0x069 drop_frames_     : 1
   +0x06c playback_rate_   : 0 
   +0x070 flush_cb_        : base::Callback<void __cdecl(void)>
   +0x078 preroll_cb_      : base::Callback<void __cdecl(enum media::PipelineStatus)>
   +0x080 init_cb_         : base::Callback<void __cdecl(enum media::PipelineStatus)>
   +0x088 statistics_cb_   : base::Callback<void __cdecl(media::PipelineStatistics const &)>
   +0x090 max_time_cb_     : base::Callback<void __cdecl(base::TimeDelta)>
   +0x098 size_changed_cb_ : base::Callback<void __cdecl(gfx::Size const &)>
   +0x0a0 ended_cb_        : base::Callback<void __cdecl(void)>
   +0x0a8 error_cb_        : base::Callback<void __cdecl(enum media::PipelineStatus)>
   +0x0b0 get_time_cb_     : base::Callback<base::TimeDelta __cdecl(void)>
   +0x0b8 get_duration_cb_ : base::Callback<base::TimeDelta __cdecl(void)>
   +0x0c0 preroll_timestamp_ : base::TimeDelta
   +0x0c8 prerolling_delayed_frame_ : scoped_refptr<media::VideoFrame>
   +0x0cc paint_cb_        : base::Callback<void __cdecl(scoped_refptr<media::VideoFrame> const &)>
   +0x0d4 set_opaque_cb_   : base::Callback<void __cdecl(bool)>
   +0x0dc last_natural_size_ : gfx::Size
   +0x0e8 last_timestamp_  : base::TimeDelta
OK, I think the issue is due to state_ remaining in kUninitialized during the call to Stop() and then OnDecoderSelected() coming along, setting the state_ to kFlushed and starting the thread despite Stop() having completed. Sigh.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 22 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=184048

------------------------------------------------------------------------
r184048 | scherkus@chromium.org | 2013-02-22T06:14:07.781883Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/video_renderer_base_unittest.cc?r1=184048&r2=184047&pathrev=184048
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/video_renderer_base.cc?r1=184048&r2=184047&pathrev=184048
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/filters/video_renderer_base.h?r1=184048&r2=184047&pathrev=184048

Fix crash in VideoRendererBase::ThreadMain().

The interleaving of VideoRendererBase::Stop() with outstanding asynchronous calls to a VideoDecoder would result in continuing to execute code (e.g., starting up a thread) on a stopped VideoRendererBase. The most common manifestation of the bug was crashing in ThreadMain() as VideoRendererBase was being destroyed.

The CHECK()s added to ~VideoRendererBase() in r180573 will stay there as they've proven useful.

BUG= 174150 


Review URL: https://chromiumcodereview.appspot.com/12324005
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: Fixed
inferno: OOC do we want to let this bake on canary for a bit before merging?
Yes, we do wait a little bit before merging. We just put out the flags when security bugs gets fixed, so that we remember to merge it later.
Cool. The crash triggers 1 or 2 times per canary release so we should get pretty quick feedback.
Canary results are looking great. I'd really like to get this merged ASAP -- it's one of our top media crashers :|
@scherkus: I'll do merges today.
scarybeasts: awesome! thanks!
Project Member

Comment 19 by bugdroid1@chromium.org, Feb 26 2013

Labels: merge-merged-1364
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=184729

------------------------------------------------------------------------
r184729 | cevans@chromium.org | 2013-02-26T20:52:40.803778Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/media/filters/video_renderer_base_unittest.cc?r1=184729&r2=184728&pathrev=184729
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/media/filters/video_renderer_base.cc?r1=184729&r2=184728&pathrev=184729
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/media/filters/video_renderer_base.h?r1=184729&r2=184728&pathrev=184729

Merge 184048
> Fix crash in VideoRendererBase::ThreadMain().
> 
> The interleaving of VideoRendererBase::Stop() with outstanding asynchronous calls to a VideoDecoder would result in continuing to execute code (e.g., starting up a thread) on a stopped VideoRendererBase. The most common manifestation of the bug was crashing in ThreadMain() as VideoRendererBase was being destroyed.
> 
> The CHECK()s added to ~VideoRendererBase() in r180573 will stay there as they've proven useful.
> 
> BUG= 174150 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12324005

TBR=scherkus@chromium.org
Review URL: https://codereview.chromium.org/12335105
------------------------------------------------------------------------
Labels: -Merge-Approved Merge-Merged Release-1
Project Member

Comment 21 by bugdroid1@chromium.org, Feb 26 2013

Labels: merge-merged-1410
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=184731

------------------------------------------------------------------------
r184731 | cevans@chromium.org | 2013-02-26T20:58:24.304168Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/media/filters/video_renderer_base_unittest.cc?r1=184731&r2=184730&pathrev=184731
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/media/filters/video_renderer_base.cc?r1=184731&r2=184730&pathrev=184731
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/media/filters/video_renderer_base.h?r1=184731&r2=184730&pathrev=184731

Merge 184048
> Fix crash in VideoRendererBase::ThreadMain().
> 
> The interleaving of VideoRendererBase::Stop() with outstanding asynchronous calls to a VideoDecoder would result in continuing to execute code (e.g., starting up a thread) on a stopped VideoRendererBase. The most common manifestation of the bug was crashing in ThreadMain() as VideoRendererBase was being destroyed.
> 
> The CHECK()s added to ~VideoRendererBase() in r180573 will stay there as they've proven useful.
> 
> BUG= 174150 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12324005

TBR=scherkus@chromium.org
Review URL: https://codereview.chromium.org/12342020
------------------------------------------------------------------------
Labels: -SecSeverity-High SecSeverity-Medium
Labels: CVE-2013-0907
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -Feature-Media-Video -SecImpacts-Stable -SecImpacts-Beta -SecSeverity-Medium -Mstone-25 Security-Impact-Stable Security-Impact-Beta Security-Severity-Medium Cr-Internals Cr-Internals-Media-Video M-25 Type-Bug-Security
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member

Comment 29 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 30 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 31 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
Labels: CVE_description-submitted
Project Member

Comment 34 by sheriffbot@chromium.org, Jul 29

Labels: -Pri-2 Pri-1

Sign in to add a comment