Last Comment Bug 798873 - Crash in SDP formatted with too many ICE candidates - 'Assertion failure: endbuf_p - *ptr > 0' [@ sdp_build_attribute]
: Crash in SDP formatted with too many ICE candidates - 'Assertion failure: end...
Status: RESOLVED FIXED
[WebRTC], [blocking-webrtc+], [blocki...
: crash
Product: Core
Classification: Components
Component: WebRTC: Signaling (show other bugs)
: unspecified
: x86 Mac OS X
: -- critical (vote)
: mozilla18
Assigned To: Ethan Hugg [:ehugg]
: Jason Smith [:jsmith]
:
Mentors:
: 767380 (view as bug list)
Depends on:
Blocks: 791702 795057 865774
  Show dependency treegraph
 
Reported: 2012-10-07 00:30 PDT by Eric Rescorla (:ekr)
Modified: 2013-04-25 10:44 PDT (History)
8 users (show)
hskupin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Signaling SDP construction handles overflow (9.43 KB, patch)
2012-10-07 10:42 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling SDP construction handles overflow (82.07 KB, patch)
2012-10-07 21:09 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling - increase max SDP size. (5.41 KB, patch)
2012-10-07 22:29 PDT, Ethan Hugg [:ehugg]
ekr: review+
rjesup: review+
Details | Diff | Splinter Review
crashtest (640 bytes, text/html)
2012-10-08 07:03 PDT, Henrik Skupin (:whimboo) [away 09/30 - 10/06]
no flags Details
Signaling SDP construction uses flex_string (145.81 KB, patch)
2012-10-10 09:42 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling SDP construction uses flex_string (145.72 KB, patch)
2012-10-10 11:23 PDT, Ethan Hugg [:ehugg]
ekr: review+
Details | Diff | Splinter Review
Signaling SDP construction uses flex_string (145.88 KB, patch)
2012-10-10 16:45 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling SDP construction uses flex_string (145.87 KB, patch)
2012-10-10 16:48 PDT, Ethan Hugg [:ehugg]
rjesup: review+
Details | Diff | Splinter Review
Signaling SDP construction uses flex_string (143.57 KB, patch)
2012-10-11 11:19 PDT, Ethan Hugg [:ehugg]
no flags Details | Diff | Splinter Review
Signaling SDP construction uses flex_string (143.64 KB, patch)
2012-10-11 11:31 PDT, Ethan Hugg [:ehugg]
rjesup: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Crashtest v1 (2.33 KB, patch)
2012-10-15 09:48 PDT, Henrik Skupin (:whimboo) [away 09/30 - 10/06]
rjesup: review-
Details | Diff | Splinter Review
Patch 3 - flex_string fix for Windows vsnprintf (1.53 KB, patch)
2012-10-17 19:02 PDT, Ethan Hugg [:ehugg]
rjesup: review+
Details | Diff | Splinter Review

Description Eric Rescorla (:ekr) 2012-10-07 00:30:46 PDT
If you have enough interfaces, then the SDP formatter overruns the available SDP, which causes a crash. There are two defects here:

1. The hardcoded SDP length of 2048 is too small, so you get truncation.


2. When you run out of space, you assert, which is a problem.

(gdb) c
Continuing.
Assertion failure: endbuf_p - *ptr > 0, at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c:211

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
sdp_build_attribute (sdp_p=0x129d8e000, level=3, ptr=0x12b8f59f0, len=359) at sdp_attr.c:211
(gdb) bt
#0  sdp_build_attribute (sdp_p=0x129d8e000, level=3, ptr=0x12b8f59f0, len=359) at sdp_attr.c:211
#1  0x00000001044f47cf in sdp_build (sdp_ptr=0x129d8e000, bufp=0x12b8f5a68, len=2048) at sdp_main.c:1198
#2  0x0000000104553ff6 in sipsdp_write_to_buf (sdp_info=0x12654cff0, retbytes=0x12b8f5ab4) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/sipstack/ccsip_sdp.c:429
#3  0x00000001044ac537 in gsmsdp_encode_sdp (sdp_p=0x12654cff0, msg_body=0x12b8f5bf8) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5483
#4  0x00000001044ac6e2 in gsmsdp_encode_sdp_and_update_version (dcb_p=0x129d93000, msg_body=0x12b8f5bf8) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:5539
#5  0x0000000104492007 in fsmdef_ev_createoffer (event=0x12b8f5e28) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c:2958
#6  0x00000001044c004b in sm_process_event (tbl=0x106398e98, event=0x12b8f5e28) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/sm.c:83
#7  0x00000001044824aa in fim_process_event (data=0x128979000, cac_passed=0 '\0') at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/fim.c:671
#8  0x00000001044a3e73 in gsm_process_msg (cmd=158, msg=0x128979000) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/gsm.c:167
#9  0x00000001044a42d4 in GSMTask (arg=0x12992d400) at /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/sipcc/core/gsm/gsm.c:359
#10 0x00007fff89825fd6 in _pthread_start ()
#11 0x00007fff89825e89 in thread_start ()
(gdb)
Comment 1 Eric Rescorla (:ekr) 2012-10-07 00:31:26 PDT
The right thing to happen is  to generate an error.
Comment 2 Ethan Hugg [:ehugg] 2012-10-07 10:42:23 PDT
Created attachment 668937 [details] [diff] [review]
Signaling SDP construction handles overflow
Comment 3 Ethan Hugg [:ehugg] 2012-10-07 10:46:59 PDT
Comment on attachment 668937 [details] [diff] [review]
Signaling SDP construction handles overflow


This is a work in progress.  I'm considering running all of the SDP construction through the new static sdp_build_line so we can handle the snprintf return and pointer arithmetic in one place.  If we want to go with this idea, I can change the rest of the file as well.
Comment 4 Ethan Hugg [:ehugg] 2012-10-07 21:09:50 PDT
Created attachment 669006 [details] [diff] [review]
Signaling SDP construction handles overflow
Comment 5 Ethan Hugg [:ehugg] 2012-10-07 22:29:33 PDT
Created attachment 669019 [details] [diff] [review]
Signaling - increase max SDP size.
Comment 6 Ethan Hugg [:ehugg] 2012-10-07 22:32:35 PDT
Comment on attachment 669019 [details] [diff] [review]
Signaling - increase max SDP size.


SDP max length upped to 4096
MOZ_ASSERT on overflow removed.
SDP creation overflow should now make CreateOffer/CreateAnswer fail.

You can try out the last one on OSX by running a VMWare instance in the background, and recompiling with SDP_MAX_LENGTH set back to 2048.
Comment 7 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-08 07:03:51 PDT
Created attachment 669136 [details]
crashtest

This is a proposed crash test for this fix. Boris, what would be the best location in our tree for it? /dom/tests/crashtests/? It doesn't exist yet so I wonder where to put it into. Thanks.
Comment 8 Eric Rescorla (:ekr) 2012-10-08 09:03:24 PDT
Comment on attachment 669019 [details] [diff] [review]
Signaling - increase max SDP size.

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

looks good as a short-term fix
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-10-08 09:21:14 PDT
dom/media/test/craashtests seems like the obvious place for the test (and would involve creating said dirs).
Comment 10 Randell Jesup [:jesup] 2012-10-08 09:24:21 PDT
https://hg.mozilla.org/mozilla-central/rev/87f3548e638e

Leaving open as this is a temporary patch

Agreed, we absolutely need tests for SDP overflows, etc.  (FYI henrik)
Comment 11 Ethan Hugg [:ehugg] 2012-10-10 09:42:41 PDT
Created attachment 670018 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 12 Ethan Hugg [:ehugg] 2012-10-10 09:45:13 PDT
Comment on attachment 670018 [details] [diff] [review]
Signaling SDP construction uses flex_string


Still work in progress.  Uploading for testing and self-splinter.

SDP max length removed entirely.  Uses new flex_string string handler functions.

Also "Cisco-SIPUA" changed to "Mozilla-SIPUA"
Comment 13 Ethan Hugg [:ehugg] 2012-10-10 11:23:16 PDT
Created attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 14 Ethan Hugg [:ehugg] 2012-10-10 11:25:54 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string


Complete removal of SDP_MAX_STRING in favor of the new flex_string.
Comment 15 Suhas 2012-10-10 12:25:32 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +73,5 @@
> +    semicolon ? ";" : "",
> +    name,
> +    value);
> +}
> +

unsigned .. should  we be specific on right integral type and its size so that it maps appropriately to format specified ....

@@ +237,5 @@
>                      SDP_PRINT("%s Built a=%s attribute line", sdp_p->debug_str,
>                                sdp_get_attr_name(attr_p->type));
>                  }
> +            } else {
> +              SDP_ERROR("%s error building attribute %d", __FUNCTION__, result);

Should we return on Error ?

@@ +3402,3 @@
>          }
> +
> +        flex_string_append(fs, "\r\n");

line 3657 seems to add only \n from the original code ?

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +144,5 @@
> +void flex_string_init(flex_string *fs) {
> +  fs->buffer_length = FLEX_STRING_CHUNK_SIZE;
> +  fs->string_length = 0;
> +  fs->buffer = cpr_malloc(fs->buffer_length);
> +  fs->buffer[0] = '\0';

cpr_malloc(FLEX_STRING_CHUNK_SIZE)

@@ +181,5 @@
> + * Not thread-safe
> + */
> +void flex_string_append(flex_string *fs, const char *more) {
> +  fs->string_length += strlen(more);
> +

how do we handle non null terminated strings ...

@@ +185,5 @@
> +
> +  flex_string_check_alloc(fs, fs->string_length + 1);
> +
> +  sstrncat(fs->buffer, more, fs->buffer_length - strlen(fs->buffer));
> +}

please update string_length after either check_alloc or sstrncat ...

Also are we truncating the characters that exceed existing chunk size of flex_string ?
Comment 16 Eric Rescorla (:ekr) 2012-10-10 12:46:32 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +65,5 @@
> + * Helper function for adding nv-pair where value is unsigned.
> + */
> +static void sdp_append_name_and_unsigned(flex_string *fs,
> +  const char *name,
> +  unsigned value,

"unsigned int"?

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +144,5 @@
> +void flex_string_init(flex_string *fs) {
> +  fs->buffer_length = FLEX_STRING_CHUNK_SIZE;
> +  fs->string_length = 0;
> +  fs->buffer = cpr_malloc(fs->buffer_length);
> +  fs->buffer[0] = '\0';

I actually prefer the original, since it has less chance of typos

@@ +169,5 @@
> + */
> +void flex_string_check_alloc(flex_string *fs, int new_min_length) {
> +  if (new_min_length > fs->buffer_length) {
> +    /* Oversize, allocate more */
> +    fs->buffer_length = (new_min_length / FLEX_STRING_CHUNK_SIZE + 1) * FLEX_STRING_CHUNK_SIZE;

I assume you want to compute:

CHUNK_SIZE * ((new_min_length/CHUNK_SIZE) + 1)

So that if I pass in new_min_length == chunk_size I get a result of 2 * chunk_size?

If so, I would use parentheses rather than rely on precedence.

Also, check for integer overflow here.

@@ +181,5 @@
> + * Not thread-safe
> + */
> +void flex_string_append(flex_string *fs, const char *more) {
> +  fs->string_length += strlen(more);
> +

non null terminated strings generally don't belong in C code.

@@ +185,5 @@
> +
> +  flex_string_check_alloc(fs, fs->string_length + 1);
> +
> +  sstrncat(fs->buffer, more, fs->buffer_length - strlen(fs->buffer));
> +}

How could that happen? The whole point here is to resize up to the right point.

@@ +209,5 @@
> +    va_start(ap, format);
> +    vsnprintf_result = vsnprintf(fs->buffer + fs->string_length, fs->buffer_length - fs->string_length, format, ap);
> +    PR_ASSERT(vsnprintf_result > 0 && vsnprintf_result < (fs->buffer_length - fs->string_length));
> +    va_end(ap);
> +    fs->string_length += vsnprintf_result;

Can't you move this out of the if/else so you just need it once?

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_string.h
@@ +39,5 @@
>  
>  #ifndef _CPR_STRING_H_
>  #define _CPR_STRING_H_
>  
> +#include <stdarg.h>

I don't believe stdarg.h is required here, since you don't use the va_* macros.

@@ +107,5 @@
> +
> +typedef struct {
> +  char *buffer;
> +  unsigned buffer_length;
> +  unsigned string_length;

I would use size_t. If you want unsigned int, then at least use "unsigned int" not just unsigned.
Comment 17 Ethan Hugg [:ehugg] 2012-10-10 13:38:52 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +3402,3 @@
>          }
> +
> +        flex_string_append(fs, "\r\n");

Good eye,  I'm pretty sure the original one is wrong is this case so I changed it.  Every other line of SDP ends in \r\n.

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +144,5 @@
> +void flex_string_init(flex_string *fs) {
> +  fs->buffer_length = FLEX_STRING_CHUNK_SIZE;
> +  fs->string_length = 0;
> +  fs->buffer = cpr_malloc(fs->buffer_length);
> +  fs->buffer[0] = '\0';

I wanted the minimum chance that someone could edit and set the buffer_length and malloc separately.

@@ +181,5 @@
> + * Not thread-safe
> + */
> +void flex_string_append(flex_string *fs, const char *more) {
> +  fs->string_length += strlen(more);
> +

strlen will fly through available memory until encountering a null by chance or causing an access violation, or did you mean something else by non-null-terminated string?

@@ +185,5 @@
> +
> +  flex_string_check_alloc(fs, fs->string_length + 1);
> +
> +  sstrncat(fs->buffer, more, fs->buffer_length - strlen(fs->buffer));
> +}

setting string_length at the end would require a new uint on the stack like "unsigned int new_length = fs->string_length + strlen(more);"  I just avoided the extra var by doing it this way.  It's not re-entrant so no one is going to try to use the flex_string until after the fcn completes.

@@ +209,5 @@
> +    va_start(ap, format);
> +    vsnprintf_result = vsnprintf(fs->buffer + fs->string_length, fs->buffer_length - fs->string_length, format, ap);
> +    PR_ASSERT(vsnprintf_result > 0 && vsnprintf_result < (fs->buffer_length - fs->string_length));
> +    va_end(ap);
> +    fs->string_length += vsnprintf_result;

Sure.  Hidden here is what we do with a vsnprintf return of -1.  I'm currently doing nothing assuming the format string was bogus.

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_string.h
@@ +39,5 @@
>  
>  #ifndef _CPR_STRING_H_
>  #define _CPR_STRING_H_
>  
> +#include <stdarg.h>

Yes,that should be moved to the .c
Comment 18 Randell Jesup [:jesup] 2012-10-10 14:32:07 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +144,5 @@
> +void flex_string_init(flex_string *fs) {
> +  fs->buffer_length = FLEX_STRING_CHUNK_SIZE;
> +  fs->string_length = 0;
> +  fs->buffer = cpr_malloc(fs->buffer_length);
> +  fs->buffer[0] = '\0';

ekr/ehugg++

@@ +166,5 @@
> + * Allocate enough chunks to hold the new minimum size.
> + *
> + * Not thread-safe
> + */
> +void flex_string_check_alloc(flex_string *fs, int new_min_length) {

size_t for length?

@@ +169,5 @@
> + */
> +void flex_string_check_alloc(flex_string *fs, int new_min_length) {
> +  if (new_min_length > fs->buffer_length) {
> +    /* Oversize, allocate more */
> +    fs->buffer_length = (new_min_length / FLEX_STRING_CHUNK_SIZE + 1) * FLEX_STRING_CHUNK_SIZE;

Which makes sense, since you probably want to allocate more than needed in these cases.  But how do you get a buffer_length  < 1*FLEX_STRING_CHUNK_SIZE?  We only reallocate if new_min_length > fs->buffer_length, so if buffer_length == FLEX_STRING_CHUNK_SIZE the case you mention won't happen.  

And in general, if new_min_length % FLEX_STRING_CHUNK_SIZE == 0, then it's also unlikely (perhaps impossible) that buffer_length isn't a multiple of FLEX_STRING_CHUNK_SIZE.

Agreed on parens

overflow: We only bump the size by FLEX_STRING_CHUNK_SIZE at most above new_min_length.  Passing in values less than FLEX_STRING_CHUNK_SIZE from MAXINT is likely a pretty serious error...  and unlikely to be successful in allocating negative values (if int).  If it's size_t, it would wrap around to a small allocation, but the question is how something could cause such a large allocation.  

However: put some unreasonable limit on the string buffer size.

@@ +181,5 @@
> + * Not thread-safe
> + */
> +void flex_string_append(flex_string *fs, const char *more) {
> +  fs->string_length += strlen(more);
> +

As in, an unterminated string is an API error in the caller, and hard to check here.  If you need to support unterminated strings (buffer+length), use a separate function.

::: media/webrtc/signaling/src/sipcc/cpr/include/cpr_string.h
@@ +107,5 @@
> +
> +typedef struct {
> +  char *buffer;
> +  unsigned buffer_length;
> +  unsigned string_length;

size_t please
Comment 19 Ethan Hugg [:ehugg] 2012-10-10 14:44:03 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +169,5 @@
> + */
> +void flex_string_check_alloc(flex_string *fs, int new_min_length) {
> +  if (new_min_length > fs->buffer_length) {
> +    /* Oversize, allocate more */
> +    fs->buffer_length = (new_min_length / FLEX_STRING_CHUNK_SIZE + 1) * FLEX_STRING_CHUNK_SIZE;

Also one thing I now see is I am doing the +1 for NULL in two places, the check_alloc and the calls to it.  I'll removed the +1 in the check_alloc.  (or maybe put two NULLs for safety ;-))

Jesup do you have a suggestion for an unreasonable limit to the string buffer size?  I've changed it to a size_t, but I'm not sure what would be a good value to use as a limit, and what is the best way to do an abort from here.

Also, I'm considering putting in an overflow check by doing a divide of SIZE_MAX now that it's a size_t.  Like this:

    /* Overflow check */
    MOZ_ASSERT((SIZE_MAX / FLEX_STRING_CHUNK_SIZE) >= ((new_min_length / FLEX_STRING_CHUNK_SIZE) + 1));

Opinions?
Comment 20 Ethan Hugg [:ehugg] 2012-10-10 14:45:29 PDT
Comment on attachment 670043 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

::: media/webrtc/signaling/src/sipcc/cpr/common/cpr_string.c
@@ +169,5 @@
> + */
> +void flex_string_check_alloc(flex_string *fs, int new_min_length) {
> +  if (new_min_length > fs->buffer_length) {
> +    /* Oversize, allocate more */
> +    fs->buffer_length = (new_min_length / FLEX_STRING_CHUNK_SIZE + 1) * FLEX_STRING_CHUNK_SIZE;

Wait, ignore that bit about the +1s - I'm losing my mind.
Comment 21 Ethan Hugg [:ehugg] 2012-10-10 16:45:41 PDT
Created attachment 670183 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 22 Ethan Hugg [:ehugg] 2012-10-10 16:48:26 PDT
Created attachment 670185 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 23 Ethan Hugg [:ehugg] 2012-10-10 16:52:45 PDT
Comment on attachment 670185 [details] [diff] [review]
Signaling SDP construction uses flex_string


Addressed the review comments and a couple more things.  

Changed PR_ASSERTs to MOZ_ASSERTs

flex_string will no longer alloc an extra CHUNK_SIZE when alloc mod CHUNK_SIZE == 0

TRY build here - https://tbpl.mozilla.org/?tree=Try&rev=fe15ec3b29a0
Comment 24 Randell Jesup [:jesup] 2012-10-10 18:54:43 PDT
(In reply to Ethan Hugg [:ehugg] from comment #19)
> Jesup do you have a suggestion for an unreasonable limit to the string
> buffer size?  I've changed it to a size_t, but I'm not sure what would be a
> good value to use as a limit, and what is the best way to do an abort from
> here.

Since it's done only when doing a malloc, I'm ok with a runtime check here.  It also protects us against unreasonable-size allocation attacks (not super likely, and you'd have to get there one malloc at a time, though in theory a single malloc could be very large...)

It has the side-benefit of semi-blocking attempts to DoS the browser using expanding allocations.

Shall we say max of 1MB? 2MB?  Can you see >1MB SDP as ever being reasonable?  I can see 10's of K in some cases, maybe in crazy-extreme cases you could find a use for 100K (though you'd probably need to support CapNeg to do so... ;-)  Hard to see 1MB.  

> Also, I'm considering putting in an overflow check by doing a divide of
> SIZE_MAX now that it's a size_t.  Like this:

Not needed if we put a limit in.  Or we put no limit in, and just do:
   MOZ_ASSERT(new_size > old_size);
Comment 25 Randell Jesup [:jesup] 2012-10-10 20:08:43 PDT
Comment on attachment 670185 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

I really like how this cleans up the code and removes an entire class of otherwise-recurring problems.  If you want to macro-ize parts, that's great, but r+

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +228,5 @@
>                  SDP_WARN("%s Invalid attribute type to build (%u)", 
>                           sdp_p->debug_str, attr_p->type);
>              }
>          } else {
> +

probably remove this blank line - super-minor

@@ +271,5 @@
>          return (SDP_SUCCESS);
>      }
>  }
>  
> +sdp_result_e sdp_build_attr_simple_string (sdp_t *sdp_p, sdp_attr_t *attr_p, flex_string *fs)

wrap

@@ +276,2 @@
>  {
> +  flex_string_sprintf(fs, "a=%s:%s\r\n", sdp_attr[attr_p->type].name, attr_p->attr.string_val);

wrap - not a big deal; I'm not a stickler for line length (and violate it myself), but splinter is happier with < 80 char lines I think.  On the other hand, if a longer length is already typical in the file - forget this comment, be consistent with the file.

@@ +1964,5 @@
> +        semicolon = TRUE;
> +      }
> +
> +      if (fmtp_p->qcif > 0) {
> +        sdp_append_name_and_unsigned(fs, "QCIF", attr_p->attr.fmtp.qcif, semicolon);

Very tempted to make a macro:
#define UNSIGNED_PARAM(cond,name,value) \
  if ((cond)) { \
    sdp_append_name_and_unsigned(fs,(name),(value),semicolon); \
    semicolon = TRUE; \
  }

  UNSIGNED_PARAM(fmtp->qcif > 0, "QCIF", attr_p->attr.fmtp.qcif)
  UNSIGNED_PARAM(fmtp->cif > 0,  "CIF",  attr_p->attr.fmtp.cif)
  ...

But honestly it doesn't save space, just lines of cut-and-paste, so in the end it may not be worth it unless there are a LOT of these.

On the other hand, maybe there are enough to make a few macros like this to make it easier to read, vet and add new value to fmtp/etc lines.  And it's easier to concentrate on the few that don't fit any of the macros.
Comment 26 Ethan Hugg [:ehugg] 2012-10-11 11:19:35 PDT
Created attachment 670465 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 27 Ethan Hugg [:ehugg] 2012-10-11 11:31:29 PDT
Created attachment 670475 [details] [diff] [review]
Signaling SDP construction uses flex_string
Comment 28 Ethan Hugg [:ehugg] 2012-10-11 11:35:19 PDT
Comment on attachment 670475 [details] [diff] [review]
Signaling SDP construction uses flex_string


Added macros for simplifying the repetitive code in sdp_build_attr_fmtp.  Also wrapped a bunch of long lines for those of you still using your VT100s.
Comment 29 Randell Jesup [:jesup] 2012-10-11 13:57:13 PDT
Comment on attachment 670475 [details] [diff] [review]
Signaling SDP construction uses flex_string

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

Our VT100's (and splinter) thank you
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-10-13 04:04:37 PDT
https://hg.mozilla.org/mozilla-central/rev/b02035b70698
Comment 32 Randell Jesup [:jesup] 2012-10-13 07:59:28 PDT
This definitely could use a test; perhaps by using ridiculously long strings where we have some ability to control the SDP generated from JS (otherwise it will need to be added to the C++ unit tests, and might be better there since more of the functionality can be tested).
Comment 33 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-15 08:10:35 PDT
So I put together a crashtest for this crash. But sadly we can't get it landed until the leaks it discovers are getting fixed. I will file this as a follow-up bug.
Comment 34 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-15 09:48:08 PDT
Created attachment 671472 [details] [diff] [review]
Crashtest v1

First crashtest for WebRTC! It will be disabled for now on m-c due to leakage (see bug 801682).

Results without it being disabled you can find here:
https://tbpl.mozilla.org/?tree=Try&rev=afd532de4f16
Comment 35 Randell Jesup [:jesup] 2012-10-15 12:26:11 PDT
Comment on attachment 671472 [details] [diff] [review]
Crashtest v1

Unfortunately this cannot be known to reliably reproduce the crash as it's dependent on the number of interfaces and ICE candidates generated.
Comment 36 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2012-10-15 12:40:26 PDT
As given by Randell on IRC we probably have to wait until appropriate interfaces are available so that it can be done as a crashtest (reftest). For now there should be a way to do it via an unit test.

So I will hold off from further work on this crashtest for now.
Comment 37 Ethan Hugg [:ehugg] 2012-10-17 09:09:53 PDT
*** Bug 767380 has been marked as a duplicate of this bug. ***
Comment 38 Robert Kaiser 2012-10-17 15:32:27 PDT
Is this going to land on Aurora? Bug 791702 is marked tracking for 18, and is fixed by the patch here, apparently.
Comment 39 Ethan Hugg [:ehugg] 2012-10-17 19:02:12 PDT
Created attachment 672636 [details] [diff] [review]
Patch 3 - flex_string fix for Windows vsnprintf
Comment 40 Ethan Hugg [:ehugg] 2012-10-17 19:03:00 PDT
Reopening.  Bug found in flex_string_sprintf on Windows builds.  Windows version of vsnprintf is different than Linux/OSX.
Comment 41 Ethan Hugg [:ehugg] 2012-10-17 19:04:53 PDT
Comment on attachment 672636 [details] [diff] [review]
Patch 3 - flex_string fix for Windows vsnprintf


Using derf's suggestion because PR_vsnprintf does not return formatted size.
Comment 42 Randell Jesup [:jesup] 2012-10-17 21:38:20 PDT
https://hg.mozilla.org/mozilla-central/rev/cb573b9307e5
Comment 43 Randell Jesup [:jesup] 2012-10-17 22:35:59 PDT
Once we're verified with the Windows fix, I plan to ask for uplift to Aurora.
Comment 44 Randell Jesup [:jesup] 2012-10-18 22:30:13 PDT
I'll note that the standard loopback demo at http://people.mozilla.com/~anarayanan/webrtc/pc_test.html provokes the last issue on Windows, and that now appears to work.
Comment 45 Randell Jesup [:jesup] 2012-10-31 06:19:45 PDT
Comment on attachment 670475 [details] [diff] [review]
Signaling SDP construction uses flex_string

[Approval Request Comment]
Bug caused by (feature/regressing bug #): main webrtc signaling landing just before 18 uplift.
User impact if declined: Failure to create useful offers or answers if the user has too many interfaces, possible security impact.
Testing completed (on m-c, etc.): Tested on m-c and baked for several weeks, with one regression fix (on this same bug, patch #3).
Risk to taking this patch (and alternatives if risky): Pretty low risk; patch is large but largely mechanical API change.   Basic code is quite clean and it removes tons of error paths, as the only size-related error left is OOM and we use infallible malloc.
String or UUID changes made by this patch: None
Comment 46 bhavana bajaj [:bajaj] 2012-10-31 14:52:35 PDT
Comment on attachment 670475 [details] [diff] [review]
Signaling SDP construction uses flex_string

Approving because the patch is low risk for a new feature landed in 18, baked on m-c for several weeks and fixes a regression tracking 18 .

Note You need to log in before you can comment on or make changes to this bug.