Closed Bug 860143 Opened 8 years ago Closed 8 years ago

WebRTC assertion failure: sizeof(server->u.dnsname.host) > host_.size() and crash [@mozilla::NrIceStunServer::ToNicerStunStruct]

Categories

(Core :: WebRTC: Networking, defect, P1)

x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox22 + fixed

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)

Attached file testcase
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
Attached file callstack
Test case is using a TURN server, so this won't block unless this can be reproduced in a non-TURN case.
Flags: in-testsuite?
Keywords: assertion
Whiteboard: [WebRTC][blocking-webrtc-][turn]
Assignee: nobody → adam
(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.
(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]
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)
Attachment #735789 - Attachment is obsolete: true
Attachment #735789 - Flags: review?(ekr)
Marking as blocked on Bug 856848, since this test case will encounter that crash once my patch is applied.
Depends on: 856848
Attachment #735809 - Flags: review?(ekr)
Whiteboard: [WebRTC][blocking-webrtc+][turn] → [WebRTC][blocking-webrtc+][turn][webrtc-uplift]
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/75acdef6ee50
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Whiteboard: [WebRTC][blocking-webrtc+][turn][webrtc-uplift] → [WebRTC][blocking-webrtc+][turn][webrtc-uplift][qa-]
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?
Attachment #735809 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #744435 - Flags: review?(adam)
Comment on attachment 744435 [details] [diff] [review]
Crashtest v1

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

Looks good.
Attachment #744435 - Flags: review?(adam) → review+
You need to log in before you can comment on or make changes to this bug.