WebRTC crash [@sipsdp_write_to_buf]

RESOLVED FIXED in Firefox 18

Status

()

--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: ehugg)

Tracking

(Blocks: 1 bug, {crash, sec-critical, testcase})

Trunk
mozilla19
x86_64
Mac OS X
crash, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox16 unaffected, firefox17 unaffected, firefox18+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

(Whiteboard: [WebRTC][asan][blocking-webrtc+][qa-][adv-main18-])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 661802 [details]
callstack

changeset:   108232:d9c3bd6436a5
(Reporter)

Comment 1

6 years ago
Created attachment 661804 [details]
testcase
(Assignee)

Comment 2

6 years ago
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?
Assignee: nobody → ethanhugg

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Yes, I can reproduce it every time.

Comment 5

6 years ago
Do you see something wrong in this code?
(Assignee)

Comment 6

6 years ago
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) {
(Reporter)

Updated

6 years ago
Blocks: 792125

Updated

6 years ago
Whiteboard: [WebRTC][asan] → [WebRTC][asan][blocking-webrtc+]
(Assignee)

Comment 7

6 years ago
Created attachment 662659 [details] [diff] [review]
Trying byte-byte copy instead of memcpy
status-firefox-esr10: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → unaffected
status-firefox18: --- → affected
tracking-firefox18: --- → +
(Assignee)

Comment 8

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

Comment 9

6 years ago
Fixed by patch pushed from Bug 798873 - this code path removed now that SDP generation uses a flexible-length string.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Keywords: verifyme

Updated

6 years ago
Keywords: verifyme
Whiteboard: [WebRTC][asan][blocking-webrtc+] → [WebRTC][asan][blocking-webrtc+][qa-]
Ethan, I assume that we no longer need a crashtest given that the code path has been removed?
(Assignee)

Comment 11

6 years ago
(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.
Depends on: 798873
Flags: in-testsuite-
Target Milestone: --- → mozilla19

Comment 12

6 years ago
Bug 798873 has been fixed in 18, is this one also fixed through that?
(Assignee)

Comment 13

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

Comment 14

6 years ago
Then let's mark it as such so it comes off the radar queries. ;-)
status-firefox18: affected → fixed
status-firefox-esr17: --- → unaffected
Whiteboard: [WebRTC][asan][blocking-webrtc+][qa-] → [WebRTC][asan][blocking-webrtc+][qa-][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.