WebAudio use-after-free [@mozilla::dom::AudioNode::SendDoubleParameterToStream]

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: posidron, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86_64
Mac OS X
crash, csectype-uaf, regression, sec-critical
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main23-])

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
alloc: ./obj-ff64-asan-opt/dom/bindings/AudioContextBinding.cpp:320

static bool
createBufferSource(JSContext* cx, JSHandleObject obj, mozilla::dom::AudioContext* self, unsigned argc, JS::Value* vp)
{
  nsRefPtr<mozilla::dom::AudioBufferSourceNode > result;
  result = self->CreateBufferSource();


free: ./content/media/webaudio/AudioNode.cpp:36

AudioNode::Release()
{
  if (mRefCnt.get() == 1) {
    // We are about to be deleted, disconnect the object from the graph before
    // the derived type is destroyed.
    DisconnectFromGraph();
  }
* nsrefcnt r = nsDOMEventTargetHelper::Release();


re-use: ./content/media/webaudio/AudioNode.cpp:159

void
AudioNode::SendDoubleParameterToStream(uint32_t aIndex, double aValue)
{
* AudioNodeStream* ns = static_cast<AudioNodeStream*>(mStream.get());


The attached testcase is not reliable, sometime we get the following after some reloads:

Assertion failure: op == PL_DHASH_LOOKUP || (*(uint32_t*)(table->entryStore + ((uint32_t)1 << (32 - (table)->hashShift)) * table->entrySize)) == 0, at /Users/cdiehl/dev/repos/mozilla/mozilla-inbound/obj-ff64-asan-opt/xpcom/build/pldhash.cpp:573


Tested with m-i changeset: 130337:5447d49a2c6f
(Reporter)

Comment 1

6 years ago
Created attachment 743659 [details]
testcase
(Reporter)

Comment 2

6 years ago
Created attachment 743660 [details]
callstack
(Assignee)

Updated

6 years ago
Blocks: 779297
(Assignee)

Comment 3

6 years ago
If I'm reading the code write, we never delete a buffer source node from mSources, which is, pretty broken!
(Assignee)

Comment 4

6 years ago
Created attachment 743925 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #743925 - Flags: review?(paul)
(Assignee)

Comment 5

6 years ago
Comment on attachment 743925 [details] [diff] [review]
Patch (v1)

I'd like to land this soonish...
Attachment #743925 - Flags: review?(roc)
Comment on attachment 743925 [details] [diff] [review]
Patch (v1)

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

I'd like to see some documentation for mPannerNode!
(Assignee)

Comment 7

6 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 743925 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 743925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see some documentation for mPannerNode!

In addition to what we have in AudioContext.h?
Created attachment 744634 [details] [diff] [review]
Remove useless mPannerNode member in AudioBufferSourceNode. r=

In lieu of providing explanation on why we have an |mPannerNode| member, here is
a patch that removes it, because it is actually useless. Not sure why I even
added it in the first place.
Attachment #744634 - Flags: review?(ehsan)

Updated

6 years ago
Assignee: ehsan → paul

Updated

6 years ago
Attachment #743925 - Flags: review?(paul) → review+

Updated

6 years ago
Assignee: paul → ehsan
status-b2g18: --- → unaffected
status-firefox21: --- → disabled
status-firefox22: --- → disabled
status-firefox23: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox23: --- → ?
Keywords: regression
(Assignee)

Comment 9

6 years ago
Comment on attachment 744634 [details] [diff] [review]
Remove useless mPannerNode member in AudioBufferSourceNode. r=

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

Ah, mPannerNode, not mPannerNodes!
Attachment #744634 - Flags: review?(ehsan) → review+
This and a bunch of others have been backed out for causing permaorange Android mochitest-2, which resulted in the closure of multiple trees, since it had been merged all over the place :-( Had to back out more than the likely cause (bug 867174) due to conflicts & incorrect disabling of Android tests.

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/698f6059ef47
https://hg.mozilla.org/integration/mozilla-inbound/rev/396f66db6b55
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-firefox23: ? → +
(Reporter)

Updated

6 years ago
Blocks: 875414
(Assignee)

Comment 15

5 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.