media/libcubeb: pulse_context_init does not check for NULL returned from pa_context_new

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: max, Assigned: kinetik)

Tracking

({assertion, crash})

40 Branch
mozilla43
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:36.0) Gecko/20100101 Firefox/36.0 SeaMonkey/2.33.1
Build ID: 20150321194732

Steps to reproduce:

Open up an enormous Firefox session with 900+ tabs, such that the address space is either very fragmented or just all used up, then try to load any HTML5 video on YouTube. This is on 32-bit Linux Mint 13 (Ubuntu 12.04.)


Actual results:

Firefox instantly crashes with an assertion failure. I spent some time with GDB and looking through the Firefox and PulseAudio codebases, and I'm pretty sure the problem is that pa_context_new is returning NULL due to the low memory conditions, and instead of checking for that possibility, the function pulse_context_init in the file /media/libcubeb/src/cubeb_pulse.c is simply calling pa_context_set_state_callback with that NULL pointer, thus causing the assertion failure in PulseAudio's code. 

I did a more detailed writeup of my analysis here: https://plus.google.com/110008887158117796734/posts/W8JYvzBzLQh


Expected results:

Firefox gracefully continues without sound or does not attempt to play the video, or in some way continues without crashing instantly. pulse_context_init should check for the NULL pointer returned from pa_context_new and not attempt to use a NULL pointer with pa_context_set_state_callback.
OS: Unspecified → Linux
Hardware: Unspecified → x86
Keywords: assertion, crash
Component: Untriaged → Audio/Video
Product: Firefox → Core
Thanks for investigating and reporting this.  I've pushed a simple fix upstream: https://github.com/kinetiknz/cubeb/commit/09b07684c768adaf8511d6e44fabb661a29d0610

I'll take this bug and get the fix in to the Gecko version of libcubeb soon.
Assignee: nobody → kinetik
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Cool, thanks for addressing this so promptly! Also thanks for making me realize I misspelled my own name in my Bugzilla profile... lol.

I don't know about Mozilla's policies with regards to these types of fixes: is this kind of thing likely to get backported to the 40.x branch, and if not, what's the soonest I can actually get the fix myself? Are you able to replicate/test the issue yourself? I don't mind trying a Nightly build to see if it it's fixed.
(In reply to Max Eliaser from comment #2)
> I don't know about Mozilla's policies with regards to these types of fixes:
> is this kind of thing likely to get backported to the 40.x branch, and if
> not, what's the soonest I can actually get the fix myself? Are you able to
> replicate/test the issue yourself? I don't mind trying a Nightly build to
> see if it it's fixed.

It'll be fixed in nightly in a few days, depending on review, landing, merge turnaround times.  I can request uplift to aurora (42) and beta (41) and hopefully the fix will be accepted as it's trivial and safe, but due to the nature of the problem it won't be fixed in release (40.x) as that branch only takes critical fixes.

I haven't had a chance to test it myself.  It wouldn't surprise me if there are other issues present in this code and elsewhere in out-of-memory/out-of-address-space situations.
Attachment #8649034 - Flags: review?(padenot)
Fair enough. One thing to note is that the Firefox 41.x branch and on have a fix that is said to drastically improve memory usage on installations that have AdBlock Plus. While I certainly cannot wait to get that update, it could also be a confounding factor making it harder to replicate this issue and prove that your fix works.

I can still look for the chatter about mmap on stderr, so if that's there and Firefox doesn't crash, then that should prove that the fix works.
Attachment #8649034 - Flags: review?(padenot) → review+
Comment on attachment 8649034 [details] [diff] [review]
bug1195058_v0.patch

Approval Request Comment
[Feature/regressing bug #]: bug 837563
[User impact if declined]: crash in OOM situation
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: virtual no risk, adds a NULL check and return to an existing allocation
[String/UUID change made/needed]: none
Attachment #8649034 - Flags: approval-mozilla-beta?
Attachment #8649034 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/09e913e90731
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8649034 [details] [diff] [review]
bug1195058_v0.patch

Avoid a crash, taking it.
Attachment #8649034 - Flags: approval-mozilla-beta?
Attachment #8649034 - Flags: approval-mozilla-beta+
Attachment #8649034 - Flags: approval-mozilla-aurora?
Attachment #8649034 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.