798873 - Crash in SDP formatted with too many ICE candidates - 'Assertion failure: endbuf_p - *ptr > 0' [@ sdp_build_attribute]

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

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)]

The right thing to happen is  to generate an error.

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: Signaling SDP construction handles overflow

Comment 3

[Ethan Hugg [:ehugg]]

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]]

Attachment: Signaling SDP construction handles overflow

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: Signaling - increase max SDP size.

Comment 6

[Ethan Hugg [:ehugg]]

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][⌚️UTC+2] (away 07/21 - 08/08)]

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

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 [:bzbarsky]]

dom/media/test/craashtests seems like the obvious place for the test (and would involve creating said dirs).

Comment 10

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

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]]

Attachment: Signaling SDP construction uses flex_string

Comment 12

[Ethan Hugg [:ehugg]]

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]]

Attachment: Signaling SDP construction uses flex_string

Comment 14

[Ethan Hugg [:ehugg]]

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]

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)]

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]]

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] (needinfo me)]

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]]

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]]

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]]

Attachment: Signaling SDP construction uses flex_string

Comment 22

[Ethan Hugg [:ehugg]]

Attachment: Signaling SDP construction uses flex_string

Comment 23

[Ethan Hugg [:ehugg]]

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] (needinfo me)]

(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] (needinfo me)]

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]]

Attachment: Signaling SDP construction uses flex_string

Comment 27

[Ethan Hugg [:ehugg]]

Attachment: Signaling SDP construction uses flex_string

Comment 28

[Ethan Hugg [:ehugg]]

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] (needinfo me)]

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 30

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b02035b70698

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b02035b70698

Comment 32

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

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][⌚️UTC+2] (away 07/21 - 08/08)]

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][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: 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] (needinfo me)]

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][⌚️UTC+2] (away 07/21 - 08/08)]

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 38

[Robert Kaiser]

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]]

Attachment: Patch 3 - flex_string fix for Windows vsnprintf

Comment 40

[Ethan Hugg [:ehugg]]

Reopening.  Bug found in flex_string_sprintf on Windows builds.  Windows version of vsnprintf is different than Linux/OSX.

Comment 41

[Ethan Hugg [:ehugg]]

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] (needinfo me)]

https://hg.mozilla.org/mozilla-central/rev/cb573b9307e5

Comment 43

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

Once we're verified with the Windows fix, I plan to ask for uplift to Aurora.

Comment 44

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

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] (needinfo me)]

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]]

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 .

Comment 47

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/56608426e56c
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f3b48aa0187

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=00c95d70fbbf