Closed Bug 1181735 Opened 9 years ago Closed 9 years ago

crash in mozilla::AudioMixer::FinishMixing()

Categories

(Core :: Audio/Video, defect)

40 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + verified
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: away, Assigned: padenot)

References

Details

(Keywords: crash, csectype-uaf, sec-critical)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-6fbd247f-9d3a-479d-b6b6-6c3272150705.
=============================================================

Top crash in 40 beta 1. Looks like a dead mixer callback. Paul can you take a look?

00 0e85f9a8 63eba191 xul!mozilla::AudioMixer::FinishMixing+0x1c
01 0e85f9d8 63eb81e8 xul!mozilla::MediaStreamGraphImpl::Process+0x174
02 0e85fa20 63e98904 xul!mozilla::MediaStreamGraphImpl::OneIteration+0x150
03 0e85fa88 63e98cbd xul!mozilla::AudioCallbackDriver::DataCallback+0x22d
04 0e85fa94 6452c04b xul!mozilla::AudioCallbackDriver::DataCallback_s+0x11
05 0e85faa8 6452c631 xul!noop_resampler::fill+0x11
06 0e85fad4 6452d1c7 xul!`anonymous namespace'::refill+0x34
07 0e85fb14 6f5fc01d xul!`anonymous namespace'::wasapi_stream_render_loop+0x17f
Flags: needinfo?(padenot)
Group: core-security
Keywords: csectype-uaf
Attached patch patch — — Splinter Review
Let's try this, being a bit more tight when removing callbacks. Let's then
observe the crash numbers. I have another lead to explore if this is not it.
Attachment #8631585 - Flags: review?(rjesup)
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Attachment #8631585 - Flags: review?(rjesup) → review+
To see impact, we'll likely need a reproducible case, or to get it into beta (aurora may show enough signal to read if this is helping, though).  Kairo?

We'll need s-a to land, and aurora and/or beta approval
Flags: needinfo?(kairo)
(In reply to Randell Jesup [:jesup] from comment #3)
> To see impact, we'll likely need a reproducible case, or to get it into beta
> (aurora may show enough signal to read if this is helping, though).  Kairo?

I don't know of a reproducible case, but Dev Edition is showing a volume of multiple crashes per day so should give us some insight at least. In the end, we'll need this on beta both to verify with data that it works and to actually fix the crash there. :)
Flags: needinfo?(kairo)
Comment on attachment 8631585 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  
Hard based on the patch.  Perhaps easier if someone can link the patch back to the crashstats reports that reference this bug and show 0x5a*/etc addresses.  Would still need a POC to cause those semi-at-will.

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?
34.*, 35.*, apparently 36
No crashes seen in 37-39 (so far) in the last month
40, 41, 42 affected.

If not all supported branches, which bug introduced the flaw?
Likely the main cubeb/MediaStreamGraph refactor last fall to drive the graph from callbacks.

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

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely to cause regressions; only basic smoketesting needed.
Attachment #8631585 - Flags: sec-approval?
Attachment #8631585 - Flags: approval-mozilla-beta?
Attachment #8631585 - Flags: approval-mozilla-aurora?
sec-approval+ and other approvals given.
Keywords: sec-critical
Attachment #8631585 - Flags: sec-approval?
Attachment #8631585 - Flags: sec-approval+
Attachment #8631585 - Flags: approval-mozilla-beta?
Attachment #8631585 - Flags: approval-mozilla-beta+
Attachment #8631585 - Flags: approval-mozilla-aurora?
Attachment #8631585 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/25820d96ddf8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(clearing NI)
Flags: needinfo?(padenot)
Any noticeable change in crash rate since this landed? Should we consider a belts-and-suspenders backport to esr38 as well?
Flags: needinfo?(padenot)
I can't find a good graph that would show if this has fixed the issue.

This is what I did: click on the link in comment 0, click on "more reports", click on "graph", change the channel to beta. It says that it does not fine any occurrence. What am I doing wrong?
Flags: needinfo?(padenot) → needinfo?(dmajor)
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3AAudioMixer%3A%3AFinishMixing() - expanding the "Product" area still shows right now:
Firefox	40.0b3	51.23 %	643
Firefox	40.0b2	30.68 %	385
Firefox	40.0b4	8.37 %	105
Firefox	40.0b1	4.86 %	61

Beta 4 (released ~2 days ago) still looks affected.
I'm thinking maybe 1181097 will help here. It's currently waiting for for a+. NI Sylvestre so he knows it's more important than what is stated in the bug.
Flags: needinfo?(sledru)
Sylvestre is still in PTO. Ritu, would you mind having a look ?

After further analysis, I'm rather confident bug 1181097 will help.
Flags: needinfo?(sledru) → needinfo?(rkothari)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> https://hg.mozilla.org/releases/mozilla-beta/rev/bd50a25b0ff3

(In reply to Paul Adenot (:padenot) from comment #13)
> I can't find a good graph that would show if this has fixed the issue.
> 
> This is what I did: click on the link in comment 0, click on "more reports",
> click on "graph", change the channel to beta. It says that it does not fine
> any occurrence. What am I doing wrong?

I never use the graph (I don't remember if it's because I don't trust it, or some other reason). Here's a supersearch that says this still happens a lot on 40b4: https://crash-stats.mozilla.com/search/?signature=~FinishMixing&release_channel=beta&product=Firefox&platform=Windows+NT&_facets=signature&_facets=build_id&_facets=version&_facets=release_channel&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-build_id
Flags: needinfo?(dmajor)
Paul, I have approved the patch for bug 1181097 to be uplifted to Aurora. I am not comfortable approving for Beta. Let's give it a few days to ensure it doesn't have a negative impact.
Flags: needinfo?(rkothari)
(In reply to Ritu Kothari (:ritu) from comment #18)
> Paul, I have approved the patch for bug 1181097 to be uplifted to Aurora. I
> am not comfortable approving for Beta. Let's give it a few days to ensure it
> doesn't have a negative impact.

Sorry, I needinfo'd you in bug 1181097, not realizing the discussion was happening here.  I can clear the needinfo there, but I feel strongly that we need to get this patch on Beta as soon as we can -- before the next go-to Beta (Beta 4).   In this particular case, we're not going to gain much by waiting, and we stand to lose valuable time if this patch doesn't fix the crash here.  I'm happy to talk about this more, but I need Beta approval today or tomorrow if we're going to get this into Beta 4 which I believe goes to build on Monday.  Thanks!
Flags: needinfo?(rkothari)
Lawrence approved the patch for bug 1181097. Clearing n-i.
Flags: needinfo?(rkothari)
Per IRC discussion with Maire, setting Gecko <40 to unaffected as the fix that landed here is not applicable even though similar crashes have existed earlier.
(In reply to Ritu Kothari (:ritu) from comment #20)
> Lawrence approved the patch for bug 1181097. Clearing n-i.

Looks like we are not seeing this in 40.0b6 so far. \o/
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #22)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Lawrence approved the patch for bug 1181097. Clearing n-i.
> 
> Looks like we are not seeing this in 40.0b6 so far. \o/

Yes, the signature is gone in everything after beta 4, we're good. :)
Comment 23 sounds like a verification to me!
Status: RESOLVED → VERIFIED
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.