WebAudio use-after-free crash [@mozilla::MediaInputPort::Release]

RESOLVED FIXED in Firefox 24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: posidron, Assigned: Ehsan)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla24
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main24-])

Attachments

(6 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
Posted file testcase
If the testcase doesn't trigger at the first try, refresh the page one or two times.


alloc: ./content/media/webaudio/AudioNode.cpp:174

void
AudioNode::Connect(AudioNode& aDestination, uint32_t aOutput,
                   uint32_t aInput, ErrorResult& aRv)
{
[...]
*     ps->AllocateInputPort(mStream, MediaInputPort::FLAG_BLOCK_INPUT,
                            static_cast<uint16_t>(aInput),
                            static_cast<uint16_t>(aOutput));
[...]


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

void
MediaInputPort::Destroy()
{
  class Message : public ControlMessage {
  public:
    Message(MediaInputPort* aPort)
      : ControlMessage(nullptr), mPort(aPort) {}
    virtual void Run()
    {
      mPort->Disconnect();
      --mPort->GraphImpl()->mPortCount;
*     NS_RELEASE(mPort);
    }
[...]
}


re-use: ./content/media/MediaStreamGraph.h:737

* NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MediaInputPort)



Tested with m-i changeset: 132784:57f555f96ac9 and the patch in bug 874952
Reporter

Updated

6 years ago
Summary: WebAudio use-after-free crash [@] → WebAudio use-after-free crash [@mozilla::MediaInputPort::Release]
Reporter

Comment 1

6 years ago
Posted file callstack
Reporter

Updated

6 years ago
Blocks: 875414
Assignee

Updated

6 years ago
Assignee: nobody → ehsan
Assignee

Comment 3

6 years ago
OK, so MediaInputPort::Destroy::Command holds a plain pointer to MediaInputPort, and NS_RELEASES it directly, claiming that the graph is the last thing holding on to the MediaInputPort.  That is completely incorrect.  You can allocate an input port and hold on to it as much as you want.
Assignee

Comment 4

6 years ago
And FWIW, this bug should have been discovered before by fuzzing WebRTC etc, I would have expected.
I doubt the sequence that causes this is possible in webrtc, which only uses MediaInputPorts as part of nsDOMUserMediaStream in MediaManager.cpp (aka getUserMedia).  The failure paths there are minimal, and it's closely tied to the associated MediaStream/TrackUnion.
Assignee

Comment 6

6 years ago
After fixing that part of the bug, I hit bug 875221 on this test case.
Assignee

Updated

6 years ago
Depends on: 875221
Assignee

Comment 7

6 years ago
(In reply to Randell Jesup [:jesup] from comment #5)
> I doubt the sequence that causes this is possible in webrtc, which only uses
> MediaInputPorts as part of nsDOMUserMediaStream in MediaManager.cpp (aka
> getUserMedia).  The failure paths there are minimal, and it's closely tied
> to the associated MediaStream/TrackUnion.

Hmm, actually, maybe this is only an issue for the non-realtime MSGs...
Assignee

Comment 8

6 years ago
Posted patch Patch (v1) (obsolete) — Splinter Review
Attachment #753493 - Flags: review?(roc)
Comment on attachment 753493 [details] [diff] [review]
Patch (v1)

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

::: content/media/MediaStreamGraph.cpp
@@ +1906,5 @@
>      virtual void RunDuringShutdown()
>      {
>        Run();
>      }
> +    nsRefPtr<MediaInputPort> mPort;

This is going to leak since you're adding a reference we weren't adding before.

The comment is definitely wrong, but I'm not sure why the NS_RELEASE is wrong. We need to release the MediaStreamGraph's reference to the port.
Assignee

Comment 10

6 years ago
Hmm, I think you're right.  For some reason the leak detector doesn't detect that. :(

So perhaps the real problem here is that mConsumers holds weak pointers to MediaInputPorts, and everything else holds a strong reference to it.  If we made those nsRefPtrs, we should be able to remove this NS_RELEASE and just make the ControlCommand hold a normal nsRefPtr.  Does that make sense?
Flags: needinfo?(roc)
Read the comment on MediaInputPort in MediaStreamGraph.h :-)

Note that in ProcessedMediaStream::AllocateInputPort, in the Run method that runs in the MediaStreamGraph thread, we have
      // The graph holds its reference implicitly
      mPort.forget();

So the already_AddRefed we throw away there is supposed to be released by the NS_RELEASE we're doing in Destroy's Run method. There shouldn't be a need to use nsRefPtrs actually inside the MediaStreamGraph.

So, I don't know why this code isn't working.
Flags: needinfo?(roc)
Reporter

Comment 12

6 years ago
Posted file testcase-reduced
Assignee

Comment 13

6 years ago
OK, so the problem here is that if ProcessedMediaStream::AllocateInputPort::Command is run during shutdown, it Release()'s mPort without registering it with the graph, and then later on we NS_RELEASE the pointer because we think that the graph is holding a reference to it, while it's not.
Assignee

Comment 14

6 years ago
Posted patch Patch (v2)Splinter Review
Attachment #753493 - Attachment is obsolete: true
Attachment #753493 - Flags: review?(roc)
Attachment #753822 - Flags: review?(roc)
Assignee

Comment 15

6 years ago
With the patch v2, we seem to hit another bug with this callstack...
Assignee

Comment 16

6 years ago
Christoph, can you please see if you can minimize the test case once more with this patch applied?
Flags: needinfo?(cdiehl)
Reporter

Comment 17

6 years ago
:Ehsan, I have noticed something similar, see: https://bugzilla.mozilla.org/show_bug.cgi?id=875221#c6

Yes, give me a few minutes.
Flags: needinfo?(cdiehl)
Assignee

Comment 19

6 years ago
(In reply to Christoph Diehl [:cdiehl] from comment #18)
> Created attachment 753840 [details]
> testcase-reduced-after-patch-2

Attached this to bug 875221.
Assignee

Updated

6 years ago
Depends on: 875911
Assignee

Comment 22

6 years ago
The leak is bug 875911.
Assignee

Comment 23

6 years ago
Landed with the fix to bug 875911 and converted the test to a mochitest to use SpecialPowers instead of fuzzPriv to force GC and CC.
https://hg.mozilla.org/mozilla-central/rev/5bb195db9f44
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee

Comment 25

6 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
Whiteboard: [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.