933447 - mozRTCPeerConnection crash with GC/CC

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Andrew McCreight [:mccr8]]

See test case 1 in bug 928221.

Comment 1

[Maire Reavy [:mreavy]]

Thanks for filing this, Andrew. 

Adam, I believe you've already starting working on this, so I'm officially assigning it to you.  If you're too busy with IETF/W3 stuff, just let me know.  Thanks.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I'm already looking at this one FWIW.

Comment 3

[Maire Reavy [:mreavy]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> I'm already looking at this one FWIW.

Jan-Ivar, if you want to take over ownership of this bug, that's fine by me.  Just coordinate with Abr first.  Thanks.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Agreed with abr that I'd take a stab at this one.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

OK, I removed the (arguably redundant main-to-main) dispatch and it didn't help: 

#0 mozilla::dom::CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*>, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) at dom/bindings/CallbackObject.cpp:65
#1 mozilla::dom::CallbackObject::CallSetup::CallSetup(JS::Handle<JSObject*>, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) at dom/bindings/CallbackObject.cpp:147
#2 mozilla::dom::PeerConnectionObserverJSImpl::OnStateChange(mozilla::dom::PCObserverStateType, mozilla::ErrorResult&, JSCompartment*) at dom/bindings/./PeerConnectionObserverBinding.cpp:2027
#3 mozilla::dom::PeerConnectionObserver::OnStateChange(mozilla::dom::PCObserverStateType, mozilla::ErrorResult&, JSCompartment*) at dom/bindings/./PeerConnectionObserverBinding.cpp:2477

PeerConnectionObserver::OnStateChange(PCObserverStateType state, ErrorResult& aRv, JSCompartment* aCompartment)
{
  return mImpl->... // mImpl == 0x0.

#4 sipcc::PeerConnectionImpl::IceStateChange_m(mozilla::dom::PCImplIceState) at media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1613

  ...
  nsRefPtr<PeerConnectionObserver> pco = mPCObserver.MayGet();
  if (!pco) {
    return NS_OK;
  }
  JSErrorResult rv;
  pco->OnStateChange(PCObserverStateType::IceState, rv);

mPCObserver	sipcc::PeerConnectionImpl::WeakConcretePtr	
  mObserver	mozilla::dom::PeerConnectionObserver *	0x0000000121208140
  mWeakPtr	nsWeakPtr	
  mRawPtr	nsWeakReference *	0x0000000121277da0

#5 mozilla::runnable_args_m_1<nsRefPtr<sipcc::PeerConnectionImpl>, tag_nsresult (sipcc::PeerConnectionImpl::*)(mozilla::dom::PCImplIceState), mozilla::dom::PCImplIceState>::Run() at media/webrtc/signaling/../../../media/mtransport/runnable_utils_generated.h:125
#6 nsThread::ProcessNextEvent(bool, bool*) at xpcom/threads/nsThread.cpp:622

So, pco->mImpl is gone, yet the WeakReferent guard object is still alive (this is all on main thread), so the guard must not be working, as I'm not keeping what I want alive. I'm pondering what bz said in Bug 928535 comment 7.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Using new JS webidl weakPtr; Removed weakConcretePtr kludge + redundant runnables; Still crashes

This patch depends on the patch in bug 928535 and is a lot cleaner, but still crashes - This rules out the problem being the WeakConcretePtr kludge.

Symptom is unchanged (see comment 5): mImpl = null.

mImpl is inside the DOM bindings, and breakpoints on weakPtr returning NULL never hit, so it looks like the webidl binding code doesn't handle being weak-referenced at all. If someone could confirm this that would be great.

I'm looking for someone in the DOM team to help or take this over as I'm stumped, but I'm posting here rather than in bug 928535 because of the sec crash.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: modified testcase 1

Adds a 1-15 ms delay and loops to more reliably produce problem (I sometimes had trouble with the original).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Does applying this on top of your patch help?

The only way mImpl can be null is if the object has already been unlinked.

It's possible for the object to still be alive even though it's been unlinked, though, because we do lazy deletion of CCed objects, as far as I can tell.  Does explicitly dropping weak refs in unlink like this help?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Created attachment 829455 [details] [diff] [review]
> Does applying this on top of your patch help?

Yes it works! \o/

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 829440 [details] [diff] [review]
Using new JS webidl weakPtr; Removed weakConcretePtr kludge + redundant runnables; Still crashes

Thanks for figuring this out, Boris!  Weak references are the worst.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ouch.  This is a pretty big footgun.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So Olli had some thoughts on IRC about making do_QueryReferent return null for referents with a refcount of 0, I think.  That would provide a less whack-a-molish fix for the problem.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, we definitely need a general solution here.

Comment 14

[Andrew McCreight [:mccr8]]

I filed bug 936728 for a general solution.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Uses new JS webidl weakPtr (removed weakConcretePtr kludge)

Cleaner fix, and no longer removes any runnables.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try with all 3 patches - https://tbpl.mozilla.org/?tree=Try&rev=3119f8792dda

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 829650 [details] [diff] [review]
Uses new JS webidl weakPtr (removed weakConcretePtr kludge)

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

Looks fine to me, though I'm not familiar with the PeerConnection stuff in particular.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&reason_type=contains&date=2013-11-09&range_value=28&range_unit=days&hang_type=any&process_type=any&signature=mozilla%3A%3Adom%3A%3ACallbackObject%3A%3ACallSetup%3A%3ACallSetup%28JS%3A%3AHandle%3CJSObject*%3E%2C+mozilla%3A%3AErrorResult%26%2C+mozilla%3A%3Adom%3A%3ACallbackObject%3A%3AExceptionHandling%2C+JSCompartment*%29

Comment 19

[Adam Roach [:abr]]

Comment on attachment 829650 [details] [diff] [review]
Uses new JS webidl weakPtr (removed weakConcretePtr kludge)

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

Looks good to me, assuming the magic function that I don't understand makes sense to Andrew.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +977,5 @@
>  #endif
>    return NS_OK;
>  }
>  
> +// Helper - specialized for the single type needed here

This really needs a better comment. My understanding is that this is a temporary fix, to be pulled out when Bug 936728 lands.

Please expand the comment to minimally explain what this code does, indicate it's temporary, and cite Bug 936728.

Also, I don't understand what this does at all. I have not reviewed it for correctness. I'm assuming that Andrew's review covered this part of the code in particular.

Comment 20

[Andrew McCreight [:mccr8]]

> I'm assuming that Andrew's review covered this part of the code in particular.
Yes.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Adam Roach [:abr] from comment #19)
> Comment on attachment 829650 [details] [diff] [review]
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> > +// Helper - specialized for the single type needed here
> 
> This really needs a better comment. My understanding is that this is a
> temporary fix, to be pulled out when Bug 936728 lands.

Alas, the do_QueryObjectReferent() helper is not temporary till Bug 936728.

Here's what each of the 9 weak-ref calls would look like without it: 

 nsCOMPtr<nsISupportsWeakReference> tmp = do_QueryReferent(mPCObserver);
 nsRefPtr<nsSupportsWeakReference> tmp2 = do_QueryObject(tmp);
 nsRefPtr<PeerConnectionObserver> pco = static_cast<PeerConnectionObserver*>(&*tmp2);
 if (!pco) {
   return;
 }

because nsWeakPtr deals in XPCOM interfaces, while webidl deals in concrete objects (Andrew, if this is excessive please let me know. Perhaps tmp2 can be optimized out, but it's still pretty ugly).

do_QueryObjectReferent() or something like should probably be turned into a central function or template somewhere, but right now I'm looking for Aurora uplift.

I'll add this in the code-comment.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Merged patch (Uses new JS webidl weakPtr + codegen fix). r=smaug, mccr8, abr

Added comment and merged in bz's diff-patch.

Carrying forward reviews from smaug, mccr8 and abr (patch notes pieces are reviewed by one or more).

Ready to go once Bug 928535 lands.

Andrew, feel free to move codegen changes to Bug 928535 if you prefer.

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Patch still applies (with an offset), works and local mochitests are clean.

Fresh try, just in case - https://tbpl.mozilla.org/?tree=Try&rev=420089a45d89

Otherwise this is ready to go.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try looks good (mediarecorder orange is not new)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs sec-approval since sec-crit and affecting Aurora too, no?

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 833010 [details] [diff] [review]
Merged patch (Uses new JS webidl weakPtr + codegen fix). r=smaug, mccr8, abr

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I'm going to go with low-to-medium, since the known case consistently ends up in a null pointer, and the window is relatively narrow (GC delayed delete). However, a GC'ed object is involved, which makes it hard to be certain.

A script like the test-case, can create new PeerConnections in a loop without holding them, which creates a situation where browser-chrome follows a weak reference to call a function of a garbage collected object that has been unlinked yet still hasn't been deleted. When this member function runs, it crashes when it tries to use its unlinked member reference to another object (null).

However, I can't rule out that there aren't ways to provoke different uses of this GC'ed object that I haven't thought of that might produce heap_after_use.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

mozilla27

If not all supported branches, which bug introduced the flaw?

Bug 882145

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same patch should work on Aurora, except the fix depends on Bug 928535 which must be backported first.

How likely is this patch to cause regressions; how much testing does it need?

Not likely. It relies on the new functionality in Bug 928535 but is a lot cleaner than existing code.

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 833010 [details] [diff] [review]
Merged patch (Uses new JS webidl weakPtr + codegen fix). r=smaug, mccr8, abr

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 882145
User impact if declined: Intermittent nullptr crash, not ruling out heap_after_use
Testing completed (on m-c, etc.): Successful try. Test-case no longer crashes.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

*** REQUIRES BACKPORT OF BUG 928535 FIRST ***

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 833010 [details] [diff] [review]
Merged patch (Uses new JS webidl weakPtr + codegen fix). r=smaug, mccr8, abr

sec-approval+ for trunk.

After it goes in there and things are green, please put on Aurora. I'm giving approval there too.

Comment 29

[Daniel Veditz [:dveditz]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #26)
> Which older supported branches are affected by this flaw?
> 
> mozilla27
> 
> If not all supported branches, which bug introduced the flaw?
> 
> Bug 882145

Those seem contradictory... what about Firefox 26? Seems like a big set of patches for 2nd half beta though.

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Daniel Veditz [:dveditz] from comment #29)
> > Bug 882145
> 
> Those seem contradictory... what about Firefox 26? Seems like a big set of
> patches for 2nd half beta though.

Yes you're right. Bug 882145 landed just before end of 26, which is probably what confused us. Probably also why Bug 928221 was never backported to 26 like it should have been. :-(

Comment 31

[Daniel Veditz [:dveditz]]

PeerConnection is disabled on b2g1.2 so we don't have to worry about that long-lived branch.

The patch isn't as big as it seems when you realize some of it is backing out parts of bug 928221 in favor of the small patch in bug 928535. Fixing the three bugs in one combined patch shouldn't be too risky for Beta.

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Daniel Veditz [:dveditz] from comment #29)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #26)
> > Bug 882145
> 
> Those seem contradictory... what about Firefox 26?

Whoops, make that Bug 917328.

These bugs were caused by Bug 917328 which landed 10-14. So Beta is fine and Bug 928221 is fine. We just need Aurora as already requested.

So sorry about the false alarm!

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Al Billings [:abillings] from comment #28)
> Comment on attachment 833010 [details] [diff] [review]
> Merged patch (Uses new JS webidl weakPtr + codegen fix). r=smaug, mccr8, abr
> 
> sec-approval+ for trunk.
> 
> After it goes in there and things are green, please put on Aurora. I'm
> giving approval there too.

Thanks, does this Aurora approval extend to Bug 928535 as well, or must a separate request be written there? It is not a sec bug.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

abillings gave aurora approval to the merged patch, so that implies approval for bug 928535, but that probably should be made explicit as we'll be landing that as well.

Please ask for approval on that bug, and state "so we can land dependent bugs on Aurora" for a reason; and keep the request very simple.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5bd84d8e2239

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9e046ac17cd3
(with a=bajaj for aurora as well by IRC)

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bd84d8e2239

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Yeah, I should be explicit. If I'm not, it is probably just my own oversight. Ideally, we explicitly approve each bug for clarity's sake. I see it is all taken care of though.

Comment 39

[Matt Wobensmith [:mwobensmith][:matt:]]

Using bug file, I see an immediate crash on FF28, 2013-11-13.

On today's builds of FF27 and FF28, I don't see that same crash, but I get many of these:

"[57313] ###!!! ASSERTION: You can't dereference a NULL nsRefPtr with operator*().: 'mRawPtr != 0', file /Users/mwobensmith/mozilla_central/media/webrtc/signaling/../../../xpcom/base/nsAutoPtr.h, line 1072"

On at least one occasion, it eventually crashed. 

Any idea?

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

What's the stack trace of the assert and the crash?

Comment 41

[Matt Wobensmith [:mwobensmith][:matt:]]

I can't seem to repro the crash, and I didn't get the stack when it did happen.

However, if you run the latest debug build of 27/28 with the bug file, you'll see the assert.

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Matt Wobensmith from comment #41)
> However, if you run the latest debug build of 27/28 with the bug file,
> you'll see the assert.

I see the assert and the one I'm seeing is harmless and caused by this line:

http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#1005
>   nsRefPtr<PeerConnectionObserver> tmp3 = static_cast<PeerConnectionObserver*>(&*tmp2);

To explain: It is ok for WeakPtrs to be null sometimes (it's their purpose), but in order to cast it to the right type, this line dereferences the nullptr and then takes the address of it, which works but is obviously not ideal, since it causes these NS_PRECONDITION assertions.

Thanks for spotting the assertions! I'll file myself a bug to find a way less noisy way to solve this, though they should be harmless.

Comment 43

[Jesse Ruderman]

I filed bug 944854 on the "NULL nsRefPtr" assertion.

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Jan. Based on that, we can mark this bug fixed/verified.