791702 - WebRTC crash [@sipsdp_write_to_buf]

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

changeset:   108232:d9c3bd6436a5

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Ethan Hugg [:ehugg]]

I am inclined to take out this whole block that does this realloc to save a few bytes of space.  I don't think this kind of optimization is useful on our tagret platforms.

However, I can't see how we got a WRITE error on the memcpy on line 463 when the buffer is being allocated two lines before.  Anyone see the problem here?

Comment 3

[Eric Rescorla (:ekr)]

No, I don't.

Sometimes you see false positives with memcpy and valgrind b/c of the way memcpy is implemented. Though usually they are over-reads not over-writes. cdiehl: can you make this happen all the time? If so, maybe we replace the memcpy with a byte-by-byte copy and see if it still happens.

Comment 4

[Christoph Diehl [:posidron]]

Yes, I can reproduce it every time.

Comment 5

[Eric Rescorla (:ekr)]

Do you see something wrong in this code?

Comment 6

[Ethan Hugg [:ehugg]]

If it's repeatable, then it may also be interesting to see if a different error pops up if you remove the entire block in ccsip_sdp.c that starts with:

if ((CCSIP_SDP_BUF_SIZE - sdp_len) > 64) {

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: Trying byte-byte copy instead of memcpy

Comment 8

[Ethan Hugg [:ehugg]]

This chunk of code is removed in the patch to 798873 (not yet pushed).  SDP string building has been changed.

Comment 9

[Ethan Hugg [:ehugg]]

Fixed by patch pushed from Bug 798873 - this code path removed now that SDP generation uses a flexible-length string.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ethan, I assume that we no longer need a crashtest given that the code path has been removed?

Comment 11

[Ethan Hugg [:ehugg]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Ethan, I assume that we no longer need a crashtest given that the code path
> has been removed?

Henrik, that is correct.  We no longer re-size the SDP buffer because we're constructing it with a reallocating string in the patch to Bug 798873.  We should focus our testing efforts on that bug.

Comment 12

[Robert Kaiser]

Bug 798873 has been fixed in 18, is this one also fixed through that?

Comment 13

[Ethan Hugg [:ehugg]]

Yes, this one is fixed through Bug 798873 which was just pushed up for 18.

Comment 14

[Robert Kaiser]

Then let's mark it as such so it comes off the radar queries. ;-)