Assertion failure: mPromisesForOperation.IsEmpty(), at /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:555

NEW
Unassigned

Status

()

Core
Web Audio
P2
normal
Rank:
25
a month ago
9 days ago

People

(Reporter: jkratzer, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

a month ago
Created attachment 8921158 [details]
trigger.html

Testcase found while fuzzing mozilla-central rev ce1a86d3b4db.  Testcase requires the fuzzpriv extension which can be found here:
https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension

Further, the attached testcase could only be reproduced on an EC2 spot instance with no sound card.
(Reporter)

Comment 1

a month ago
Created attachment 8921160 [details]
oscillator-ended-1.html
(Reporter)

Comment 2

a month ago
Created attachment 8921161 [details]
log_minidump.txt
(Reporter)

Comment 3

a month ago
Created attachment 8921162 [details]
log_stderr.txt
Component: Audio/Video → Web Audio
Marking P2 because of crash.

Alex could you have an initial look at what is going on here?
Rank: 25
Flags: needinfo?(achronop)
Priority: -- → P2
Jason, can you try a patch for me?
Flags: needinfo?(achronop) → needinfo?(jkratzer)
(Reporter)

Comment 6

15 days ago
(In reply to Alex Chronopoulos [:achronop] from comment #5)
> Jason, can you try a patch for me?

Certainly.  Feel free to pass it along and I'll test.
Flags: needinfo?(jkratzer)
Created attachment 8925873 [details] [diff] [review]
Bug1410982-protect-mPromisesForOperation.patch

Can you please check if you still repro the crash with this patch on?
Flags: needinfo?(jkratzer)
(Reporter)

Comment 8

12 days ago
(In reply to Alex Chronopoulos [:achronop] from comment #7)
> Created attachment 8925873 [details] [diff] [review]
> Bug1410982-protect-mPromisesForOperation.patch
> 
> Can you please check if you still repro the crash with this patch on?

Alex, I cannot reproduce the issue with the patch applied.
Flags: needinfo?(jkratzer)
Attachment #8925873 - Flags: review?(padenot)
Comment on attachment 8925873 [details] [diff] [review]
Bug1410982-protect-mPromisesForOperation.patch

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

Yes. Please write a proper commit message before landing.
Attachment #8925873 - Flags: review?(padenot) → review+
Comment on attachment 8925873 [details] [diff] [review]
Bug1410982-protect-mPromisesForOperation.patch

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

::: dom/media/GraphDriver.cpp
@@ +553,5 @@
>  AudioCallbackDriver::~AudioCallbackDriver()
>  {
> +#ifdef DEBUG
> +  {
> +    MonitorAutoLock mon(mGraphImpl->GetMonitor());

To me this points to a serious problem in the AudioCallbackDriver code.

mPromisesForOperation is an array owned by the AudioCallbackDriver and not shared with anything else.

That you could have a race in the destructor to access this member, one that requires the monitor to be held, indicates that another thread is still using the AudioCallbackDriver even after the destructor is being called.

That likely points to a UAF.
Comment on attachment 8925873 [details] [diff] [review]
Bug1410982-protect-mPromisesForOperation.patch

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

sorry, I have to remove this review, because as it is it makes no sense.

If the fix requires the code to hold the monitor to read a private member in the destructor then it means you have a UAF on that object (it is accessed in another thread even after the destructor has been called).
If you don't have a UAF, then requiring the monitor shouldn't be required.

In any case, there's a problem that this change doesn't fix, just hiding the effect.
Attachment #8925873 - Flags: review+
You need to log in before you can comment on or make changes to this bug.