Crash in MediaTaskQueue::Dispatch #896

@trav90 this is your code, apparently causing crashes, possible UAF going on.

See also https://forum.palemoon.org/viewtopic.php?f=15&t=14808 for an example, and I've seen the "MonitorAutoLock" signature in a few other crash reports but couldn't pinpoint the root cause.
Potentially happens when multiple videos are playing at the same time on the page (the reuters page autoplays like 4 of them ... :crazy:)

Crash point:

MediaTaskQueue::Dispatch(TemporaryRef<nsIRunnable> aRunnable)
{
0FF34EE0  push        esi  
0FF34EE1  mov         esi,ecx  
0FF34EE3  push        edi  
=>>  MonitorAutoLock mon(mQueueMonitor);
0FF34EE4  push        dword ptr [esi+0Ch]  
0FF34EE7  call        dword ptr ds:[1065C7DCh]  
  return DispatchLocked(aRunnable, AbortIfFlushing);
0FF34EED  mov         eax,dword ptr [esp+10h]  
  MonitorAutoLock mon(mQueueMonitor);
0FF34EF1  add         esp,4

Source:

nsresult
MediaTaskQueue::Dispatch(TemporaryRef<nsIRunnable> aRunnable)
{
  MonitorAutoLock mon(mQueueMonitor);
  return DispatchLocked(aRunnable, AbortIfFlushing);
}

Crash at MonitorAutoLock mon(mQueueMonitor);
So, potential UAF on mQueueMonitor? It's peculiar that it crashes on the statement to hold the pointer.

@trav90 this is your code, apparently causing crashes, possible UAF going on. See also https://forum.palemoon.org/viewtopic.php?f=15&t=14808 for an example, and I've seen the "MonitorAutoLock" signature in a few other crash reports but couldn't pinpoint the root cause. Potentially happens when multiple videos are playing at the same time on the page (the reuters page autoplays like 4 of them ... :crazy:) Crash point: ```` MediaTaskQueue::Dispatch(TemporaryRef<nsIRunnable> aRunnable) { 0FF34EE0 push esi 0FF34EE1 mov esi,ecx 0FF34EE3 push edi =>> MonitorAutoLock mon(mQueueMonitor); 0FF34EE4 push dword ptr [esi+0Ch] 0FF34EE7 call dword ptr ds:[1065C7DCh] return DispatchLocked(aRunnable, AbortIfFlushing); 0FF34EED mov eax,dword ptr [esp+10h] MonitorAutoLock mon(mQueueMonitor); 0FF34EF1 add esp,4 ```` Source: ```` nsresult MediaTaskQueue::Dispatch(TemporaryRef<nsIRunnable> aRunnable) { MonitorAutoLock mon(mQueueMonitor); return DispatchLocked(aRunnable, AbortIfFlushing); } ```` Crash at `MonitorAutoLock mon(mQueueMonitor);` So, potential UAF on mQueueMonitor? It's peculiar that it crashes on the statement to hold the pointer.

Verified it crashes both trunk and -release at the same crash point.

Setting media.autoplay.enabled and media.autoplay.allowscripted and restarting the browser prevents this crash, so it's a workaround and confirming it's concurrent playing of multiple videos that seems to cause this.

I'll look into it.

Call Stack:

>	xul.dll!mozilla::MediaTaskQueue::Dispatch(mozilla::TemporaryRef<nsIRunnable> aRunnable) Line 33	C++
 	xul.dll!mozilla::MediaFormatReader::NotifyDataArrived(const char * aBuffer, unsigned int aLength, __int64 aOffset) Line 1354	C++
 	xul.dll!mozilla::TrackBuffer::AppendDataToCurrentResource(mozilla::MediaLargeByteBuffer * aData, unsigned int aDuration) Line 267	C++
 	xul.dll!mozilla::TrackBuffer::AppendData(mozilla::MediaLargeByteBuffer * aData, __int64 aTimestampOffset) Line 234	C++
 	xul.dll!mozilla::dom::SourceBuffer::AppendData(mozilla::MediaLargeByteBuffer * aData, double aTimestampOffset, unsigned int aUpdateID) Line 463	C++
 	xul.dll!mozilla::dom::AppendDataRunnable::Run() Line 63	C++

Call Stack: ```` > xul.dll!mozilla::MediaTaskQueue::Dispatch(mozilla::TemporaryRef<nsIRunnable> aRunnable) Line 33 C++ xul.dll!mozilla::MediaFormatReader::NotifyDataArrived(const char * aBuffer, unsigned int aLength, __int64 aOffset) Line 1354 C++ xul.dll!mozilla::TrackBuffer::AppendDataToCurrentResource(mozilla::MediaLargeByteBuffer * aData, unsigned int aDuration) Line 267 C++ xul.dll!mozilla::TrackBuffer::AppendData(mozilla::MediaLargeByteBuffer * aData, __int64 aTimestampOffset) Line 234 C++ xul.dll!mozilla::dom::SourceBuffer::AppendData(mozilla::MediaLargeByteBuffer * aData, double aTimestampOffset, unsigned int aUpdateID) Line 463 C++ xul.dll!mozilla::dom::AppendDataRunnable::Run() Line 63 C++ ````

The MediaTaskQueue is actually Mozilla code we inherited with Tycho. Mozilla then proceeded to do a major rewrite and tie the MediaTaskQueue into new components after ESR38. Going to take more research.

Ah. Sorry points blaming finger to Mozilla, instead

Ah. Sorry *points blaming finger to Mozilla, instead*

So, if I read this correctly, the problem is that this runs on multiple threads. Gotta love async/promises. if people don't know how to parallelize, then they should do so :(

I think Mozilla dodged this particular bullet by implementing the MFR after this was already re-done. there are other crash on MediaTaskQueue:Dispatch() reports that were solved by adding extra checks in the caller site, so maybe a quick solution barring a rewrite is to add an extra check in MediaFormatReader::NotifyDataArrived?

I think Mozilla dodged this particular bullet by implementing the MFR after this was already re-done. there are other crash on `MediaTaskQueue:Dispatch()` reports that were solved by adding extra checks in the caller site, so maybe a quick solution barring a rewrite is to add an extra check in `MediaFormatReader::NotifyDataArrived`?

Labels Milestones

Crash in MediaTaskQueue::Dispatch #896