1293976 - Crash in mozilla::MediaStreamGraph::NotifyOutputData since Firefox 49

Status: VERIFIED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-c3d75891-a31e-49da-ab1c-a8ebe2160809.
=============================================================

Crashing Thread (98)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MediaStreamGraph::NotifyOutputData(float*, unsigned int, int, unsigned int) 	dom/media/MediaStreamGraph.cpp:1256
1 	xul.dll 	mozilla::AudioCallbackDriver::DataCallback(float const*, float*, long) 	dom/media/GraphDriver.cpp:911
2 	xul.dll 	mozilla::AudioCallbackDriver::DataCallback_s(cubeb_stream*, void*, void const*, void*, long) 	dom/media/GraphDriver.cpp:772
3 	xul.dll 	noop_resampler::fill(void*, long*, void*, long) 	media/libcubeb/src/cubeb_resampler.cpp:54
4 	xul.dll 	`anonymous namespace'::refill 	media/libcubeb/src/cubeb_wasapi.cpp:529
5 	xul.dll 	`anonymous namespace'::refill_callback_duplex 	media/libcubeb/src/cubeb_wasapi.cpp:737
6 	xul.dll 	`anonymous namespace'::wasapi_stream_render_loop 	media/libcubeb/src/cubeb_wasapi.cpp:906
7 	ucrtbase.dll 	_crt_at_quick_exit 	
8 	kernel32.dll 	BaseThreadInitThunk 	
9 	ntdll.dll 	__RtlUserThreadStart 	
10 	ntdll.dll 	_RtlUserThreadStart

this crash signature on windows and os x seems to be regressing since firefox 49 builds - on 49.0b1 it's currently making up 0.25% of all browser crashes.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

e5e5 signature.  Most likely mListeners needs to be locked.

Comment 2

[Andreas Pehrson [:pehrsons]]

Which mListeners?

mAudioInputs seems to always be dealt with on the MSG thread.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

mAudioInputs (what I meant to say) is only modified on the Graph thread... but it's a refcounted object, and mAudioInputs isn't a RefPtr, so if the caller calls CloseAudioInput (which is async!) and then lets the listener die, mAudioInputs may still have a pointer to it and try to call through it.  That's what's happening here I believe.

Simple fix: make it an nsTArray<RefPtr<...>>.  I also found a missing lock in the AudioInput object (for DeviceChanged()).

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: make mAudioInputs use RefPtrs

Comment 5

[Andreas Pehrson [:pehrsons]]

(In reply to Randell Jesup [:jesup] from comment #3)
> mAudioInputs (what I meant to say) is only modified on the Graph thread...
> but it's a refcounted object, and mAudioInputs isn't a RefPtr, so if the
> caller calls CloseAudioInput (which is async!) and then lets the listener
> die, mAudioInputs may still have a pointer to it and try to call through it.
> That's what's happening here I believe.
> 
> Simple fix: make it an nsTArray<RefPtr<...>>.  I also found a missing lock
> in the AudioInput object (for DeviceChanged()).

It's async, but we keep a ref to the listener through the whole asyncness, see [1].

Unless there's a listener implemented without the proper addref forwarding I can't see how this is the problem.


[1] http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/dom/media/MediaStreamGraph.cpp#1064

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8780388 [details] [diff] [review]
make mAudioInputs use RefPtrs

Review of attachment 8780388 [details] [diff] [review]:
-----------------------------------------------------------------

I'd be willing to r+ this, but first I think we should have a grip on what's causing this so we don't just mask it into something else.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: make mAudioInputs use RefPtrs

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

The actual hole is the DispatchToMainThread without holding a ref... We should still hold a ref in the array, to guard against accidental missing the call to CloseAudioInput

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8780525 [details] [diff] [review]
make mAudioInputs use RefPtrs

Approval Request Comment
[Feature/regressing bug #]: full_duplex landings; Bug 1221587.  full_duplex is off in 48 except for Linux.  It's on for windows and mac in 49, but will be turned off on mac in 49 shortly.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 
Not easily - we're clearly adding a ref to a Dispatch, but it's a very tough hole to provoke.  One might be able to flood MainThread with events to slow processing of the Dispatch, and give time for the calling thread to release the underlying object.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  No.

Which older supported branches are affected by this flaw? 46 and later, but only if full_duplex is on.  full_duplex is off for all but linux in 48; 49 will go out with windows added; 50 will go out with mac as well; and android will likely be 51.

If not all supported branches, which bug introduced the flaw?  48 is the earliest that has the flaw, but we have 0 hits in crash-stats for 48.  I do not recommend holding 48.0.1 for this or shipping a new point release unless we believe someone has discovered a way to exploit it on Linux.  If we had to update linux, we could force the pref to false instead of changing the code.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Trivial to apply.

How likely is this patch to cause regressions; how much testing does it need?  Extremely unlikely to cause regressions; at most it would leak memory and mochitests are green.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8780525 [details] [diff] [review]
make mAudioInputs use RefPtrs

Approvals given.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f09e0ebac665

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13c0062b6b6d
https://hg.mozilla.org/releases/mozilla-beta/rev/27e2b6774c00

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f09e0ebac665

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

25 crashes with this signature in the last week, with 4 of them on 49 RC, 2 on Aurora and 2 on Nightly.

https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3AMediaStreamGraph%3A%3ANotifyOutputData

This seems very low volume, so I assume it is safe to close it.
If someone considers these results worrying, feel free to reopen.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Still happening with UAF signature in FF 51 and 50 and 49.0b99, such as a 50.0a2 from build 20160908004007
https://crash-stats.mozilla.com/report/index/0032d33d-dd99-4e8b-9c84-687f22160910
or a 49.0b99 (buildid  20160907153016)
https://crash-stats.mozilla.com/report/index/9b1a9e00-db49-4e76-8f13-48a8a2160910

The frequency is lower, but it's not (completely) fixed.  There might be a secondary cause.  We did close a clear hole, and the frequency dropped sharply.  When I expand the search to start July 1, and then look at the Graphs using 'version', I see the crash rate drops to almost but not quite 0 after the fix here landed.

Padenot, pehrsons - any thoughts how this could still be happening?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

re-closing and remarking verified - I spun off a bug to handle the remaining crashes which likely have a slightly different source.  The fix here clearly closed the primary hole.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::MediaStreamGraph::NotifyOutputData':
 - nightly (version 52): 1 crash from 2016-09-19.
 - aurora  (version 51): 2 crashes from 2016-09-19.
 - beta    (version 50): 46 crashes from 2016-09-20.
 - release (version 49): 366 crashes from 2016-09-05.
 - esr     (version 45): 0 crashes from 2016-07-25.

Crash volume on the last weeks (Week N is from 10-17 to 10-23):
            W. N-1  W. N-2  W. N-3  W. N-4
 - nightly       0       1       0       0
 - aurora        0       1       1       0
 - beta         12      15      10       3
 - release      91      63     128      37
 - esr           0       0       0       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly
 - aurora
 - beta    #1315     #581
 - release #807      #236
 - esr

Comment 18

[Andrei Vaida [:avaida]]

Updating overall status based on Comment 16.