922245 - SDP longer than 4k in length is truncated, causing errors

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

Currently, our RTCSessionDescription object is defined like this:

> interface mozRTCSessionDescription {
>   attribute RTCSdpType? type;
>   attribute DOMString? sdp;
> 
>   jsonifier;
>};

It would appear that Firefox has a restriction on DOMString that limits its total length to 4k. This is much smaller than SDP can realistically get. When a large SDP is passed through this interface, the SDP is truncated, and becomes syntactically incorrect (causing parse problems further down in the WebRTC stack).

Comment 1

[Adam Roach [:abr]]

Attachment: Demonstration of SDP Truncation

Comment 2

[Adam Roach [:abr]]

A little poking around with unit tests, and the problem appears to be lower than the webidl -- I can get the SDP to truncate at ~4k even without any browser involved. The problem is likely in the SIPCC code; still digging....

Comment 3

[Adam Roach [:abr]]

And there it is. ccapi.h defines SDP_SIZE to be 4096, and cc_feature_t allocates a "char sdp[SDP_SIZE]". The quick and easy fix is to bump the constant up. I think the more correct fix would be to dynamically allocate the buffer. I'm going to investigate how much work dynamic allocation would take.

Comment 4

[Timothy B. Terriberry (:derf)]

See bug 767380 and bug 798873.

Comment 5

[Adam Roach [:abr]]

Attachment: Make SDP buffer allocation dynamic in feature message

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 812272 [details] [diff] [review]
Make SDP buffer allocation dynamic in feature message

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

Looks good.  Build and worked for me.

::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c
@@ +1221,1 @@
>      }

I would be inclined to put an else { pmsg->sdp = NULL; } here.  I know that cc_get_msg_buf currently zeros out the struct, but I think there might be value in making it explicit.  The cpr_free of this above relies on it being NULL when not malloced here.  Your call.

Comment 7

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0cd8c4e0a4

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b0cd8c4e0a4