WebAudio use-after-free crash [@mozilla::::MediaStreamGraphShutDownRunnable::Run]

RESOLVED FIXED in Firefox 24

Status

()

Core
Web Audio
--
critical
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: posidron, Assigned: Away for a while)

Tracking

(Blocks: 1 bug, {crash, csectype-uaf, sec-critical})

Trunk
mozilla24
x86_64
Mac OS X
crash, csectype-uaf, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 disabled, firefox23- disabled, firefox24+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main24-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 754258 [details]
callstack

This crash is very similar to bug 875221 but it happened while processing different Audio node objects - see frame #6 in the stack of alloc and free. The re-use condition differs here too.


alloc: ./content/media/webaudio/AudioDestinationNode.cpp:188

AudioDestinationNode::AudioDestinationNode(AudioContext* aContext,
                                           bool aIsOffline,
                                           uint32_t aNumberOfChannels,
                                           uint32_t aLength,
                                           float aSampleRate)
[...]
  MediaStreamGraph* graph = aIsOffline ?
*                           MediaStreamGraph::CreateNonRealtimeInstance() :
                            MediaStreamGraph::GetInstance();


free: ./content/media/MediaStreamGraph.cpp:1525

void
MediaStream::Destroy()
{
[...]
*  GraphImpl()->AppendMessage(new Message(this));
[...]
}


re-use: ./content/media/MediaStreamGraph.cpp:1199

class MediaStreamGraphShutDownRunnable : public nsRunnable {
public:
  MediaStreamGraphShutDownRunnable(MediaStreamGraphImpl* aGraph) : mGraph(aGraph) {}
  NS_IMETHOD Run()
  {
    NS_ASSERTION(mGraph->mDetectedNotRunning,
                 "We should know the graph thread control loop isn't running!");

*   mGraph->ShutdownThreads();
[...]


Tested with m-i changeset: 132982:ce25da24ba1c
(Assignee)

Comment 1

5 years ago
I have a dollar here which says this is a dupe of bug 875221.  :-)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 875221
(Reporter)

Comment 2

5 years ago
The fuzzer is still hitting this crash.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 3

5 years ago
I owe you a dollar then.  :-)
Keywords: testcase-wanted
(Assignee)

Comment 4

5 years ago
So this crash is caused because we start to use the code in MediaStreamGraphShutDownRunnable to delete the graph, but then we end up in the case where the last media stream is deleted and so we delete the graph, and later on when we get to run MediaStreamGraphShutDownRunnable the graph is already deleted.

roc, what is the reliable condition to check for if I want to know which one of the shutdown paths will be taken by the graph?
Flags: needinfo?(roc)
I think in MediaStreamGraphImpl::AppendMessage, the check for IsEmpty() should also check mLifecycleState >= LIFECYCLE_WAITING_FOR_STREAM_DESTRUCTION before deleting the graph.
Flags: needinfo?(roc)
(Reporter)

Updated

5 years ago
Duplicate of this bug: 877695
(Assignee)

Comment 7

5 years ago
Created attachment 756092 [details] [diff] [review]
Patch (v1)

You're right!
Assignee: nobody → ehsan
Status: REOPENED → ASSIGNED
Attachment #756092 - Flags: review?(roc)
status-b2g18: --- → unaffected
status-firefox22: --- → disabled
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Attachment #756092 - Flags: review?(roc) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08523c04d61a
https://hg.mozilla.org/mozilla-central/rev/08523c04d61a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox24: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 10

4 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
No longer tracking for FF23
tracking-firefox23: + → -
status-firefox23: affected → disabled
Whiteboard: [adv-main24-]
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.