WebRTC crash [@lsm_open_rx]

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC: Signaling
P1
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: posidron, Assigned: abr)

Tracking

(Blocks: 1 bug, {crash})

Trunk
mozilla20
crash
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+] [qa-])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 691553 [details]
callstack

./media/webrtc/signaling/src/sipcc/core/gsm/lsm.c:650

          // Check that we got a valid address and port
          if (default_addr && (strlen(default_addr) > 0) && (port_allocated != -1)) {
(Reporter)

Comment 1

5 years ago
Created attachment 691554 [details]
SDP
(Reporter)

Comment 2

5 years ago
@rforbes
SEED: 1355341643.91
Fault was detected on test 256
(Reporter)

Comment 3

5 years ago
Tested with m-i changeset: 115815:ec65853affc8
Assignee: nobody → adam
Priority: -- → P1
Whiteboard: [WebRTC], [blocking-webrtc+]
(Assignee)

Comment 4

5 years ago
Christoph: I suspect the information in comment 2 is useful in reproducing this bug, but I don't know how to make use of it. I spoke to Raymond, and he thinks this came out of a fuzzer run. Is there some way I can make use of the fuzzer and this seed to reproduce the bug?

As it is, I've tried feeding the attached SDP through the signaling library both as an offer and as an answer. I don't get a crash in either case; instead the SDP is thrown out as syntactically invalid in both cases. On a cursory examination, the parsing seems safe for this kind of string. I'll be getting my Linux environment set up to run it under valgrind to double-check that evaluation.

In the meanwhile, it would be much appreciated if you could attach a "steps to reproduce" comment -- or, at the very least, a succinct description of the scenario you were executing when you found this issue.
(Assignee)

Comment 5

5 years ago
As a quick note to myself -- the line we crashed on is trying to make use of the default_addr pointer, which was gleaned through a call to vcmRcAllocICE (itself a thunk to vcmRcAllocICE_m run as NS_DISPATCH_SYNC). This function does not initialize the default_addr parameter, nor does the lsm.c code that calls it. There are several return paths from the function that would lead to the default_addr param remaining uninitialized, and the function itself does not provide a return code to indicate success or failure.

Regardless of whether we can repro this bug, and regardless of whether the specific defect I describe above is the cause of the crash we see, the code in VcmSIPCCBinding.cpp would benefit from a quick audit to ensure that all out-parameters are properly initialized at the top of each function.

I also strongly suspect that such fix-ups would address the cause of the crash, but I'd really like a repro case before I declare the problem fixed.
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Flags: needinfo?(cdiehl)
(Reporter)

Comment 6

5 years ago
Created attachment 695069 [details]
template

Hi Adam, I suspected that the SDP is not triggering the crash here but this is more or less the only information I am able to provide. Although I have attached a HTML template for setting the SDP, it is what I am currently use for fuzzing. We are reloading this template every time with our SDP and this triggers a lot of crashes in different locations.
Flags: needinfo?(cdiehl)
(Reporter)

Comment 7

5 years ago
The seed is normally our way to reproduce the same crash but here - in case with PeerConnection/gUM crashes, it does not work all the time. For me it looks like that timing plays an important role here.

Bug 820990, 822187, 822196, 822197 and many others were all found with the exact same procedure.
(Assignee)

Comment 8

5 years ago
Thanks for the template -- it gives me a lot more context for what you're doing here. Do you happen to have a wiki page set up that would show me how to get ahold of the fuzzer and run this set of tests on my own machine? I suspect this will be a bit hard to reproduce without either using your fuzzer or effectively writing my own.
(Reporter)

Comment 9

5 years ago
Please let me know your username on Github and I will give you access to the fuzzer.
(Reporter)

Updated

5 years ago
Attachment #691554 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #695069 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
It looks that this got fixed in some way by the patch of bug 822158. This crash and many others do not pop up anymore.
That's pretty distressing. It's hard to see how 822158 could have fixed htis.
(Assignee)

Comment 12

5 years ago
Writes into reallocated memory can easily cause pointer corruption. I'm not surprised at all. 

Let's leave this bug open so I can fix the out parameter problem I describe in comment 5.
(Assignee)

Comment 13

5 years ago
Created attachment 695536 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp
(Assignee)

Comment 14

5 years ago
(In reply to Christoph Diehl [:cdiehl] from comment #9)
> Please let me know your username on Github and I will give you access to the
> fuzzer.

I'm "adamroach" on github. Thanks.
(Assignee)

Updated

5 years ago
Attachment #695536 - Flags: review?(rjesup)

Updated

5 years ago
Attachment #695536 - Flags: review?(rjesup) → review+
Adam,

I agree that this is a problem, but my preferred solution would be to add a
return code and have the caller exit early. I'm fine with initializing the out
pointers (though my usual practice is not to do so)
but we still need to detect errors and that should be done
explicitly.
(Assignee)

Comment 16

5 years ago
Created attachment 695666 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp
(Assignee)

Updated

5 years ago
Attachment #695536 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #695666 - Flags: review?(ekr)
(Assignee)

Comment 17

5 years ago
Patch updated to return status value for vcmRxAllocICE and vcmGetIceParams (and check their values in the calling functions).
Comment on attachment 695666 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp

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

Should we assume here that cpr_malloc() is infallible? I believe it is.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +368,5 @@
>                      cc_call_handle_t  call_handle,
>                      cc_uint16_t port_requested,
>                      int *port_allocated)
>  {
> +    *port_allocated = 0;

Shouldn't this be -1?

@@ +811,5 @@
>   *  @param[in]  level - the m-line
>   *  @param[in]  ufrag - the ufrag
>   *  @param[in]  pwd - the pwd
>   *  @param[in]  candidates - the candidates
> + *  @param[in]   candidate_ct - the number of candidates

Whitespace

@@ +908,5 @@
>    int *pc_stream_id) {
>    uint32_t hints = 0;
>    nsresult res;
>  
> +  *pc_stream_id = 0;

Does this make sense? is 0 a valid #?

@@ +1114,5 @@
>  {
>      char fname[] = "vcmRxOpen";
>  
>      char dottedIP[20] = "";
> +    *port_allocated = 0;

-1?

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +647,5 @@
>              &default_addr, &port_allocated,
>              &candidates, &candidate_ct);
>  
>            // Check that we got a valid address and port
> +          if (!status && default_addr && (strlen(default_addr) > 0)) {

How could these by invalid
(Assignee)

Comment 19

5 years ago
Created attachment 696032 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp
(Assignee)

Updated

5 years ago
Attachment #695666 - Attachment is obsolete: true
Attachment #695666 - Flags: review?(ekr)
(Assignee)

Updated

5 years ago
Attachment #696032 - Flags: review?(ekr)
Comment on attachment 696032 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp

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

with nits.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +415,3 @@
>   *
>   */
> +static short vcmRxAllocICE_m(cc_mcapid_t mcap_id,

Indentation.

@@ +540,2 @@
>   */
> +static short vcmGetIceParams_m(const char *peerconnection,

Indentation

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +2913,1 @@
>      if (!ufrag || !ice_pwd) {

Maybe merge this with the tests above? Or make it a MOZ_ASSERT, since thse should be valid.

@@ +3033,1 @@
>      if (!ufrag || !ice_pwd) {

Comment as above.
Attachment #696032 - Flags: review?(ekr) → review+
(Assignee)

Comment 21

5 years ago
Created attachment 696752 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp,
(Assignee)

Updated

5 years ago
Attachment #696032 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #696752 - Flags: checkin?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a15f30fdfb3
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla20
Comment on attachment 696752 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp,

carry forward r=ekr
Attachment #696752 - Flags: review+
Attachment #696752 - Flags: checkin?(rjesup)
Attachment #696752 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/6a15f30fdfb3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+] [qa-]

Updated

5 years ago
Duplicate of this bug: 824960

Updated

5 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.