Closed
Bug 860143
Opened 12 years ago
Closed 12 years ago
WebRTC assertion failure: sizeof(server->u.dnsname.host) > host_.size() and crash [@mozilla::NrIceStunServer::ToNicerStunStruct]
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: posidron, Assigned: abr)
References
Details
(Keywords: assertion, crash, testcase, Whiteboard: [WebRTC][blocking-webrtc+][turn][qa-])
Crash Data
Attachments
(4 files, 1 obsolete file)
539 bytes,
text/html
|
Details | |
5.87 KB,
text/plain
|
Details | |
917 bytes,
patch
|
ekr
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
media/mtransport/build/nricectx.cpp:198 nsresult NrIceStunServer::ToNicerStunStruct(nr_ice_stun_server *server) const { [...] MOZ_ASSERT(sizeof(server->u.dnsname.host) > host_.size()); PL_strncpyz(server->u.dnsname.host, host_.c_str(), sizeof(server->u.dnsname.host)); server->u.dnsname.port = port_; server->type=NR_ICE_STUN_SERVER_TYPE_DNSNAME; } Tested with m-i with rev bbed45f6dbcc
Reporter | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
Test case is using a TURN server, so this won't block unless this can be reproduced in a non-TURN case.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → adam
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #2) > Test case is using a TURN server, so this won't block unless this can be > reproduced in a non-TURN case. FWIW, simply changing "turn:" to "stun:" in this example elicits the same crash.
Assignee | ||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
(In reply to Adam Roach [:abr] from comment #3) > (In reply to Jason Smith [:jsmith] from comment #2) > > Test case is using a TURN server, so this won't block unless this can be > > reproduced in a non-TURN case. > > FWIW, simply changing "turn:" to "stun:" in this example elicits the same > crash. Sounds like this now blocks
Priority: -- → P1
Whiteboard: [WebRTC][blocking-webrtc-][turn] → [WebRTC][blocking-webrtc+][turn]
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 735789 [details] [diff] [review] Check correct variable when setting host ekr: The fix for the reported error is the one-line change in nricectx.h. However, when I made that change, I discovered that a failure early enough in PeerConnectionImpl::Init() prevents PeerConnectionMedia::Init() from being called. called. If this happens, then PeerConnectionMedia::SelfDestruct() will attempt to deref the (null) mMainThread pointer and blow up. So this patch fixes that defect also.
Attachment #735789 -
Flags: review?(ekr)
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #735789 -
Attachment is obsolete: true
Attachment #735789 -
Flags: review?(ekr)
Assignee | ||
Comment 8•12 years ago
|
||
Marking as blocked on Bug 856848, since this test case will encounter that crash once my patch is applied.
Depends on: 856848
Assignee | ||
Updated•12 years ago
|
Attachment #735809 -
Flags: review?(ekr)
Updated•12 years ago
|
Whiteboard: [WebRTC][blocking-webrtc+][turn] → [WebRTC][blocking-webrtc+][turn][webrtc-uplift]
Comment 9•12 years ago
|
||
Comment on attachment 735809 [details] [diff] [review] Check correct variable when setting host Review of attachment 735809 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/nricectx.h @@ +111,5 @@ > port_ = port; > has_addr_ = true; > return NS_OK; > } > + else if (addr.size() < 256) { Can we use sizeof here.
Attachment #735809 -
Flags: review?(ekr) → review+
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #9) > Can we use sizeof here. Not easily. This headerfile only knows these structures by forward declaration. Including ice_ctx.h to get the structure causes myriad other files to get upset, since it's not in their include paths. (I don't really like adding the include anyway, as we're introducing additional dependencies, which can do nothing to help build times). Fixing the include paths seems like a rather extreme bit of swamp clearing for a bug that is fundamentally fixing a typo. It would be possible to move the Init function into the cpp file, where sizeof() would work fine; but that seems like more than should be called for when we're dealing with what is fundamentally a cosmetic defect.
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75acdef6ee50
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/75acdef6ee50
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Whiteboard: [WebRTC][blocking-webrtc+][turn][webrtc-uplift] → [WebRTC][blocking-webrtc+][turn][webrtc-uplift][qa-]
Comment 13•12 years ago
|
||
Comment on attachment 735809 [details] [diff] [review] Check correct variable when setting host [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: possible crash with bad input Testing completed (on m-c, etc.): on m-c, passes testcase Risk to taking this patch (and alternatives if risky): nil - fixes test of incorrect value String or IDL/UUID changes made by this patch: none
Attachment #735809 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
status-firefox22:
--- → affected
tracking-firefox22:
--- → +
Updated•12 years ago
|
Attachment #735809 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4e7e2769de8
Whiteboard: [WebRTC][blocking-webrtc+][turn][webrtc-uplift][qa-] → [WebRTC][blocking-webrtc+][turn][qa-]
Target Milestone: mozilla23 → mozilla22
Comment 15•12 years ago
|
||
Updated•12 years ago
|
Attachment #744435 -
Flags: review?(adam)
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 744435 [details] [diff] [review] Crashtest v1 Review of attachment 744435 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #744435 -
Flags: review?(adam) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a104963ba21d
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•