821071 - WebRTC crash [@lsm_open_rx]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: 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)) {

Comment 1

[Christoph Diehl [:posidron]]

Attachment: SDP

Comment 2

[Christoph Diehl [:posidron]]

@rforbes
SEED: 1355341643.91
Fault was detected on test 256

Comment 3

[Christoph Diehl [:posidron]]

Tested with m-i changeset: 115815:ec65853affc8

Comment 4

[Adam Roach [:abr]]

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.

Comment 5

[Adam Roach [:abr]]

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.

Comment 6

[Christoph Diehl [:posidron]]

Attachment: 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.

Comment 7

[Christoph Diehl [:posidron]]

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.

Comment 8

[Adam Roach [:abr]]

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.

Comment 9

[Christoph Diehl [:posidron]]

Please let me know your username on Github and I will give you access to the fuzzer.

Comment 10

[Christoph Diehl [:posidron]]

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.

Comment 11

[Eric Rescorla (:ekr)]

That's pretty distressing. It's hard to see how 822158 could have fixed htis.

Comment 12

[Adam Roach [:abr]]

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.

Comment 13

[Adam Roach [:abr]]

Attachment: Initialize all out parameters in VcmSIPCCBinding.cpp

Comment 14

[Adam Roach [:abr]]

(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.

Comment 15

[Eric Rescorla (:ekr)]

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.

Comment 16

[Adam Roach [:abr]]

Attachment: Initialize all out parameters in VcmSIPCCBinding.cpp

Comment 17

[Adam Roach [:abr]]

Patch updated to return status value for vcmRxAllocICE and vcmGetIceParams (and check their values in the calling functions).

Comment 18

[Eric Rescorla (:ekr)]

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

Comment 19

[Adam Roach [:abr]]

Attachment: Initialize all out parameters in VcmSIPCCBinding.cpp

Comment 20

[Eric Rescorla (: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.

Comment 21

[Adam Roach [:abr]]

Attachment: Initialize all out parameters in VcmSIPCCBinding.cpp,

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a15f30fdfb3

Comment 23

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

Comment on attachment 696752 [details] [diff] [review]
Initialize all out parameters in VcmSIPCCBinding.cpp,

carry forward r=ekr

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a15f30fdfb3