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

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Jason Smith [:jsmith]]

Test case is using a TURN server, so this won't block unless this can be reproduced in a non-TURN case.

Comment 3

[Adam Roach [:abr]]

(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.

Comment 4

[Adam Roach [:abr]]

Attachment: Check correct variable when setting host

Comment 5

[Maire Reavy [:mreavy]]

(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

Comment 6

[Adam Roach [:abr]]

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.

Comment 7

[Adam Roach [:abr]]

Attachment: Check correct variable when setting host

Comment 8

[Adam Roach [:abr]]

Marking as blocked on Bug 856848, since this test case will encounter that crash once my patch is applied.

Comment 9

[Eric Rescorla (:ekr)]

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.

Comment 10

[Adam Roach [:abr]]

(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.

Comment 11

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75acdef6ee50

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75acdef6ee50

Comment 13

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

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

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4e7e2769de8

Comment 15

[Jason Smith [:jsmith]]

Attachment: Crashtest v1

Comment 16

[Adam Roach [:abr]]

Comment on attachment 744435 [details] [diff] [review]
Crashtest v1

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

Looks good.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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