820990 - WebRTC use-after-free crash [@mozilla::NrIceCtx::EmitAllCandidates]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

alloc: ./media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:248

PeerConnectionImpl::PeerConnectionImpl()
: mRole(kRoleUnknown)
  , mCall(NULL)
  , mReadyState(kNew)
  , mIceState(kIceGathering)
  , mPCObserver(NULL)
  , mWindow(NULL)
  , mIdentity(NULL)
  , mSTSThread(NULL)
  , mMedia(new PeerConnectionMedia(this)) {
  MOZ_ASSERT(NS_IsMainThread());
}


free: ./media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h:349

  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PeerConnectionMedia)


re-use: ./media/mtransport/nricectx.cpp:377

  SignalGatheringCompleted(this);


Tested with m:i changeset:   115815:ec65853affc8

Comment 1

[Christoph Diehl [:posidron]]

Perhaps it is important to mention that the sdp attribute for the offer in setLocalDescription() was empty.

@rforbes
SEED: 1355341643.91
Fault was detected on test 7

Comment 2

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Christoph Diehl [:cdiehl] from comment #2)
> Created attachment 692842 [details]
> testcase

Do you have any steps to perform with that testcase? I can't get my nightly on OS X to crash loading and reloading the testcase a couple of times.

Comment 4

[Christoph Diehl [:posidron]]

It's a use-after-free bug, you need an ASan build to catch it.

Comment 5

[Eric Rescorla (:ekr)]

I think this is a dup of: 822158. Let's revisit once that's fixed.

Comment 6

[Christoph Diehl [:posidron]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> I think this is a dup of: 822158. Let's revisit once that's fixed.

822158 would be the dup..

Comment 7

[Christoph Diehl [:posidron]]

This bug needs some priority, we are crashing here almost on every testcase at this location. We are not able to make a step forward in fuzzing as long this isn't fixed.

Comment 8

[Maire Reavy [:mreavy]]

(In reply to Christoph Diehl [:cdiehl] from comment #7)
> This bug needs some priority, we are crashing here almost on every testcase
> at this location. We are not able to make a step forward in fuzzing as long
> this isn't fixed.

We're prioritizing solving this and Bug 822158.  We hope to have a solution soon.

Comment 9

[Christoph Diehl [:posidron]]

This got now fixed by the patch of bug 822158.

Comment 12

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 13

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 14

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 15

[Jason Smith [:jsmith]]

Comment on attachment 709608 [details] [diff] [review]
Crashtest v1

One small nit I just saw...let me fix that

Comment 16

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 709613 [details] [diff] [review]
Crashtest v1

This time I got it right.

Comment 18

[Jason Smith [:jsmith]]

Comment on attachment 709613 [details] [diff] [review]
Crashtest v1

Actually I just realized what I did here won't work. Let me fix this again...

Comment 19

[Jason Smith [:jsmith]]

Attachment: Crashtest v2

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 709622 [details] [diff] [review]
Crashtest v2

Now I think I got it right.

Comment 21

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

Comment on attachment 709622 [details] [diff] [review]
Crashtest v2

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

I don't see how this properly tests the ASAN-requiring bug here...  How was this verified?

Comment 22

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #21)
> Comment on attachment 709622 [details] [diff] [review]
> Crashtest v2
> 
> Review of attachment 709622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see how this properly tests the ASAN-requiring bug here...  How was
> this verified?

I reduced Christophl's test case to only execution path that was possible in the code, which is what this HTML test case in the crashtest is. When addStream was being called on a closed peer connection, an exception fires indicating that the stream is already closed. So nothing after v0.addStream(v5) ever executes in the test case shown. We also know the error callback never fires, so none of that code executes, so we remove that as well.

The only possibility is that when this crash did happen, it happened in a situation such that addStream happened after the close in some weird race condition case.

I guess I need more info on where the crash failed then.

Comment 23

[Jason Smith [:jsmith]]

Comment on attachment 709622 [details] [diff] [review]
Crashtest v2

Removing review -  I think more info is needed here to confirm where the crash was failing in the attached test case.

Comment 24

[Jason Smith [:jsmith]]

Christophl - Can you provide some input at what point that the crash failed? I'm having some difficulty confirming if the crashtest I have falls in line with the crash filed here.

Comment 25

[Christoph Diehl [:posidron]]

Hm, not sure where exactly the crash triggered, sorry. Note, that you will need an ASan build to reproduce this crash.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this fixed by bug 822158?  If so, please update the status-firefox20 flag to 'fixed', otherwise nominate additional fixes for uplift to branches.