837324 - WebRTC crash [@fsmdef_ev_addcandidate]

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

Click "start" to reproduce.

Tested with m-c changeset: 120354:2cc710018b14

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

0 	XUL 	fsmdef_ev_addcandidate 	fsmdef.c:3654
1 	XUL 	sm_process_event 	sm.c:48
2 	XUL 	fim_process_event 	fim.c:651
3 	XUL 	gsm_process_msg 	gsm.c:132
4 	XUL 	GSMTask 	gsm.c:324
5 	libsystem_c.dylib 	libsystem_c.dylib@0x4e8bf 	
6 	libsystem_c.dylib 	libsystem_c.dylib@0x51b75 	
7 	XUL 	XUL@0x17185b0

Comment 3

[Christoph Diehl [:posidron]]

Attachment: testcase

The while loop is not needed.

Comment 4

[Eric Rescorla (:ekr)]

I cannot reproduce this with a local build of asan and the listed line number (3654) is a comment in my copy of fsmdef.c

Please provide a new run with not just the line number but also the actual contents of the failing line of code.
Best-case, please break it in the debugger and show the offending object.

Comment 5

[Christoph Diehl [:posidron]]

Also can not reproduce it with m-i 120658:4466d3ff8ada

The crashing line for m-c 120354:2cc710018b14 is:

    gsmsdp_set_ice_attribute (SDP_ATTR_ICE_CANDIDATE, level,
      dcb->sdp->dest_sdp, candidate);


gdb $ p dcb
$4 = (fsmdef_dcb_t *) 0x114fe8080
gdb $ p dcb->sdp
$5 = (cc_sdp_t *) 0x0
gdb $ p dcb->sdp->dest_sdp
Cannot access memory at address 0x8

Comment 6

[Christoph Diehl [:posidron]]

It is line 3668 on m-i 120658:4466d3ff8ada

Comment 7

[Eric Rescorla (:ekr)]

Fixed in 120528:f8e40ab9bf6c

Comment 8

[Jason Smith [:jsmith]]

Verified on 2/3/2013 - not getting a crash.

Comment 9

[Jason Smith [:jsmith]]

Attachment: Crashtest for 1st testcase v1

Comment 10

[Scoobidiver (away)]

(In reply to Jason Smith [:jsmith] from comment #8)
> Verified on 2/3/2013 - not getting a crash.
There are still crashes with this signature: https://crash-stats.mozilla.com/report/list?signature=fsmdef_ev_addcandidate

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Correct. So this crash has not been fixed by bug 834100. It's also a cross-platform crash.

One of those reports from a build on Feb 4th: bp-64ffad4b-0ce8-4e97-bd72-17f272130205

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

The only URL existent for now is:
https://apprtc.appspot.com

The others are the testcases from this bug and the one Eric thought it will fix this bug.

Comment 13

[Christoph Diehl [:posidron]]

Attachment: testcase-after-fix

Depends, the testcase here does not crash anymore immediately. We are now crashing here:

   if (dcb == NULL) {
        FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
        ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
            dcb->caller_id.call_instance_id, strlib_empty());
        return SM_RC_CLEANUP;
    }

Have added a new testcase, click the button a few times and wait.

Comment 14

[Christoph Diehl [:posidron]]

Attachment: testcase-after-fix

Let's use this testcase for simplicity, since the malformed candidate strings doesn't seem to cause the problem.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Christoph Diehl [:cdiehl] from comment #14)
> Created attachment 710234 [details]
> testcase-after-fix
> 
> Let's use this testcase for simplicity, since the malformed candidate
> strings doesn't seem to cause the problem.

Is this the same crash as this bug?

Cause the stack I got looks different:

https://crash-stats.mozilla.com/report/index/bp-12a04f7b-34e4-422a-8b36-b34402130205

Comment 16

[Christoph Diehl [:posidron]]

Ohw, that looks like the callstack I just filed for a use-after-free crash for which I had no testcase. See bug: 838169. Somehow those crashes are all connected.

I will hide this bug because it contains a testcase which can trigger also the use-after-free crash.

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 710084 [details] [diff] [review]
Crashtest for 1st testcase v1

Given that the original test case isn't causing the crash, I'm clearing the review request here.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to Jason Smith [:jsmith] from comment #17)
> Given that the original test case isn't causing the crash, I'm clearing the
> review request here.

This testcase was crashing the browser with this stack. So it would still be worth having this test in the tree. Also because it's different from the one from bug 837324.

Comment 19

[Christoph Diehl [:posidron]]

The original testcase is not obsolete per se, but it requires you now to click the crash button a few more times. Therefore I have added the setTimeout function to the new testcase. Before the patch landed a single PeerConnection/addIceCandidate call was enough to trigger the crash.

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 710084 [details] [diff] [review]
Crashtest for 1st testcase v1

Alright, putting r? back out then. Sounds like it's still worthwhile to check this in then.

Might need to make the 2nd testcase have a different name though.

Comment 21

[Christoph Diehl [:posidron]]

Edit: The callstack is the same except the line number changed for #0, it is:

media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:3539

Comment 22

[Eric Rescorla (:ekr)]

OK, so the problem here is a simple null-ptr deref, because dcb == NULL and then
we are doing dcb->caller_id


    if (dcb == NULL) {
        FSM_DEBUG_SM(DEB_F_PREFIX"dcb is NULL.\n", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
        ui_ice_candidate_add(evAddIceCandidateError, line, call_id,
            dcb->caller_id.call_instance_id, strlib_empty());


Yo

Comment 23

[Eric Rescorla (:ekr)]

This probably means that the NrIceCtx::Release() in c15 is a different defect.

Comment 24

[Ethan Hugg [:ehugg]]

Attachment: Fix ui_ice_candidate_add call when dcb is NULL

Comment 25

[Eric Rescorla (:ekr)]

Comment on attachment 710316 [details] [diff] [review]
Fix ui_ice_candidate_add call when dcb is NULL

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

lgtm

Comment 26

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e978440a0ce

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e978440a0ce

Comment 28

[Jason Smith [:jsmith]]

Verified on 2/8/2013 with updated test case.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f2e0fe92df9

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f2e0fe92df9

Comment 31

[Scoobidiver (away)]

There are crashes from a single user in 20.0b1.