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

VERIFIED FIXED in Firefox 49

Status

()

defect
P1
critical
Rank:
5
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: philipp, Assigned: jesup)

Tracking

({crash, regression, sec-critical})

49 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49+ verified, firefox-esr45 unaffected, firefox50+ verified, firefox51+ verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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.
Rank: 15
Priority: -- → P1
Rank: 15 → 10
Rank: 10 → 5
e5e5 signature.  Most likely mListeners needs to be locked.
Assignee: nobody → rjesup
Group: media-core-security
Component: WebRTC: Audio/Video → Audio/Video: MediaStreamGraph
Keywords: sec-critical
Which mListeners?

mAudioInputs seems to always be dealt with on the MSG thread.
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()).
Posted patch make mAudioInputs use RefPtrs (obsolete) — Splinter Review
Attachment #8780388 - Flags: review?(pehrson)
(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 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.
Attachment #8780388 - Flags: review?(pehrson)
Attachment #8780388 - Attachment is obsolete: true
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
Attachment #8780525 - Flags: review?(pehrson) → review+
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.
Attachment #8780525 - Flags: sec-approval?
Attachment #8780525 - Flags: approval-mozilla-beta?
Attachment #8780525 - Flags: approval-mozilla-aurora?
Comment on attachment 8780525 [details] [diff] [review]
make mAudioInputs use RefPtrs

Approvals given.
Attachment #8780525 - Flags: sec-approval?
Attachment #8780525 - Flags: sec-approval+
Attachment #8780525 - Flags: approval-mozilla-beta?
Attachment #8780525 - Flags: approval-mozilla-beta+
Attachment #8780525 - Flags: approval-mozilla-aurora?
Attachment #8780525 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/f09e0ebac665
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: media-core-security → core-security-release
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.
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?
Status: VERIFIED → REOPENED
Flags: needinfo?(pehrson)
Flags: needinfo?(padenot)
Resolution: FIXED → ---
Blocks: 1302231
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.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: needinfo?(pehrson)
Flags: needinfo?(padenot)
Resolution: --- → FIXED
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
Group: core-security-release
Updating overall status based on Comment 16.
Status: RESOLVED → VERIFIED
Blocks: 1360334
You need to log in before you can comment on or make changes to this bug.