843644 - Allow STUN servers to be specified by DNS Names

Status: RESOLVED FIXED

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

Comment 1

[Eric Rescorla (:ekr)]

Attachment: DNS resolution for STUN servers

Comment 2

[Eric Rescorla (:ekr)]

Gerv,

The files nr_resolver.[ch] are going to be contributed back as part of nICEr, which, as you can see, is BSD licensed.
The files also include some modest cut-and-paste from nICEr

If I understand the licensing policy, we should be using the same license. Do we just take their license and
add the name Mozilla where it says Adobe? Or add a new Mozilla-only license BSD license?

Comment 3

[Eric Rescorla (:ekr)]

Attachment: DNS resolution for STUN servers

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 716593 [details] [diff] [review]
DNS resolution for STUN servers

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

This is a first cut.

JIB: you can see an example of what we will have to do to wireup AsyncResolve to NrIceCtx in nriceresolverfake.*

Comment 5

[Eric Rescorla (:ekr)]

Note: this does pass unit testing on my machine, though I haven't tested end-to-end

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 716593 [details] [diff] [review]
DNS resolution for STUN servers

Works! I glean from test-run output that TGSSHostname produces two candidates like TGSSIpAddress, and not one like TGSSBogusHostname:

[----------] 4 tests from IceGatherTest
[ RUN      ] IceGatherTest.TestGatherStunServerIpAddress
Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 59417 typ host
Got candidate candidate:1 1 UDP 1692424959 76.99.57.74 59417 typ srflx raddr 192.168.1.103 rport 59417
[       OK ] IceGatherTest.TestGatherStunServerIpAddress (1207 ms)
[ RUN      ] IceGatherTest.TestGatherStunServerHostname
Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 52544 typ host
Got candidate candidate:1 1 UDP 1692424959 76.99.57.74 52544 typ srflx raddr 192.168.1.103 rport 52544
[       OK ] IceGatherTest.TestGatherStunServerHostname (1203 ms)
[ RUN      ] IceGatherTest.TestGatherStunBogusHostname
Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 49769 typ host
[       OK ] IceGatherTest.TestGatherStunBogusHostname (1202 ms)
[ RUN      ] IceGatherTest.TestGatherStunServerHostnameNoResolver
Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 62346 typ host
[       OK ] IceGatherTest.TestGatherStunServerHostnameNoResolver (1203 ms)
[----------] 4 tests from IceGatherTest (4815 ms total)

But there's nothing in the test itself to catch that (that the resolver in fact did something). Is that something we'd like?

I'm modeling bug 837919 on nriceresolverfake.* and putting it in mtransport (cp nr*fake.* nr*.*). Let me know if that's not what you had in mind. No code comments at the moment, except can I use c++?

Comment 7

[Eric Rescorla (:ekr)]

Y(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> Comment on attachment 716593 [details] [diff] [review]
> DNS resolution for STUN servers
> 
> Works! I glean from test-run output that TGSSHostname produces two
> candidates like TGSSIpAddress, and not one like TGSSBogusHostname:
> 
> [----------] 4 tests from IceGatherTest
> [ RUN      ] IceGatherTest.TestGatherStunServerIpAddress
> Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 59417 typ host
> Got candidate candidate:1 1 UDP 1692424959 76.99.57.74 59417 typ srflx raddr
> 192.168.1.103 rport 59417
> [       OK ] IceGatherTest.TestGatherStunServerIpAddress (1207 ms)
> [ RUN      ] IceGatherTest.TestGatherStunServerHostname
> Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 52544 typ host
> Got candidate candidate:1 1 UDP 1692424959 76.99.57.74 52544 typ srflx raddr
> 192.168.1.103 rport 52544
> [       OK ] IceGatherTest.TestGatherStunServerHostname (1203 ms)
> [ RUN      ] IceGatherTest.TestGatherStunBogusHostname
> Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 49769 typ host
> [       OK ] IceGatherTest.TestGatherStunBogusHostname (1202 ms)
> [ RUN      ] IceGatherTest.TestGatherStunServerHostnameNoResolver
> Got candidate candidate:0 1 UDP 2111832319 192.168.1.103 62346 typ host
> [       OK ] IceGatherTest.TestGatherStunServerHostnameNoResolver (1203 ms)
> [----------] 4 tests from IceGatherTest (4815 ms total)
> 
> But there's nothing in the test itself to catch that (that the resolver in
> fact did something). Is that something we'd like?

yes, but.. Currently our tests don't depend on having a working STUN server,
so this is potentially going to create a problem in the test harness. We need
to deal with getting a reasonable C++ unit test set up before we get more
aggressive.



> I'm modeling bug 837919 on nriceresolverfake.* and putting it in mtransport
> (cp nr*fake.* nr*.*). Let me know if that's not what you had in mind. No
> code comments at the moment, except can I use c++?

You can use C++ internally, but the external interface must of course be C.

Comment 8

[Gervase Markham [:gerv]]

Please use the license from Adobe, with an additional copyright line underneath the existing one:

Copyright (C) 2013 The Mozilla Foundation

This means we don't lie outright and say Adobe owns the whole copyright, but we also don't multiply license blocks or license variants.

Gerv

Comment 9

[Adam Roach [:abr]]

Comment on attachment 716593 [details] [diff] [review]
DNS resolution for STUN servers

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

#include <std_trailing_ws_comment>

I'm not including any comments on the resolve() function signature changes we discussed on IRC.

This all seems to hang together just fine, although I think there is some unnecessary complexity that we might want to slightly refactor (see my comments below about resolver creation).

::: media/mtransport/build/Makefile.in
@@ +30,5 @@
>  EXPORTS_mtransport = \
>    ../dtlsidentity.h \
>    ../nricectx.h \
>    ../nricemediastream.h \
> +  ../nriceresolverfake.h \

Isn't the real resolver class going to need access to nr_resolver.h? I think you want to add an entry here for it. Admittedly, I don't fully understand the purpose of the EXPORTS_mtransport variable, so I might be off in the weeds here...

::: media/mtransport/nricectx.h
@@ +82,1 @@
>  struct NrIceStunServer {

Not related to this set of changes, but: why is this a struct rather than a class?

@@ +85,4 @@
>      memcpy(&addr_, &addr, sizeof(addr));
>    }
>  
>    // Convenience function to allow you to pass an IP addr as a string

This comment is no longer correct (or, at least, no longer complete) -- this is now also the API used to pass DNS names in.

::: media/mtransport/standalone/Makefile.in
@@ +26,5 @@
>  EXPORTS_mtransport = \
>    ../dtlsidentity.h \
>    ../nricectx.h \
>    ../nricemediastream.h \
> +  ../nriceresolverfake.h \

I don't understand the purpose of this file, but my comment from the other Makefile.in may apply here as well.

::: media/mtransport/test/ice_unittest.cpp
@@ +406,5 @@
> +
> +TEST_F(IceGatherTest, TestGatherStunServerHostname) {
> +  peer_->SetStunServer(kDefaultStunServerHostname, kDefaultStunServerPort);
> +  peer_->SetResolver();
> +  Gather();

Shouldn't we do some inspection of the ice_ctx stun servers to ensure that the IP addresses were correctly set to kDefaultStunServerAddress?

@@ +412,5 @@
> +
> +TEST_F(IceGatherTest, TestGatherStunBogusHostname) {
> +  peer_->SetStunServer(kBogusStunServerHostname, kDefaultStunServerPort);
> +  peer_->SetResolver();
> +  Gather();

Similar check here for failure, of course.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +112,5 @@
>    {
>      nr_ice_candidate *cand=0;
>      nr_ice_candidate *tmp=0;
>      int r,_status;
> +    char label[256];

I don't think 256 is quite right.

We're about to fill this buffer with possibly as much as a type (5 bytes), a local IP address (39 bytes for v6) & port (5 bytes), and a server hostname (255 bytes) & port (5 bytes), along with five formatting characters (including two colons to separate ports) and a terminating null.

If I do my math right, I think we want 315 bytes here.

@@ +138,5 @@
> +
> +      case SERVER_REFLEXIVE:
> +        if(r=nr_ice_candidate_format_stun_label(label, sizeof(label),cand))
> +          ABORT(r);
> +	break;

Tab

@@ +143,5 @@
> +
> +      case RELAYED:
> +        if(r=nr_ice_candidate_format_stun_label(label, sizeof(label),cand))
> +          ABORT(r);
> +	break;

Tab

@@ +183,5 @@
>  
>      _status=0;
>    abort:
>      if (_status){
>        nr_ice_candidate_destroy(&cand);

We used to emit a log message regardless of whether the candidate creation succeeded. It might be useful to match that behavior by emitting a failure log message here.

@@ +484,5 @@
> +      ABORT(r);
> +
> +    /* Fix the port */
> +    if(r=nr_transport_addr_set_port(&cand->stun_server_addr,
> +				    cand->stun_server->dnsname_port))

Tabs

@@ +485,5 @@
> +
> +    /* Fix the port */
> +    if(r=nr_transport_addr_set_port(&cand->stun_server_addr,
> +				    cand->stun_server->dnsname_port))
> +	ABORT(r);

Tab

@@ +488,5 @@
> +				    cand->stun_server->dnsname_port))
> +	ABORT(r);
> +    /* Fill in the address string */
> +    if(r=nr_transport_addr_fmt_addr_string(&cand->stun_server_addr))
> +	ABORT(r);

Tab

@@ +509,5 @@
> +    int r,_status;
> +
> +    switch(cand->type){
> +      case HOST:
> +        assert(0); /* Can't happen */

ABORT(R_INTERNAL)

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +238,3 @@
>      }
>  
>      /* Now initialize all the candidates */

https://www.dropbox.com/s/uf2o1a5s4xgokxr/35440558.jpg

@@ +242,5 @@
>      while(cand){
>        if(cand->state!=NR_ICE_CAND_STATE_INITIALIZING){
>          if(r=nr_ice_candidate_initialize(cand,nr_ice_initialize_finished_cb,ctx)){
>            if(r!=R_WOULDBLOCK){
> +	    ctx->uninitialized_candidates--;

Tabs

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h
@@ +55,2 @@
>    nr_transport_addr addr;
> +  char dnsname_host[256];  /* Limit from RFC 1034, plus a 0 byte */

It seems to me that we want to union addr with dnsname_host/dnsname_port.

@@ +157,5 @@
>  int nr_ice_ctx_remember_id(nr_ice_ctx *ctx, nr_stun_message *msg);
>  int nr_ice_ctx_finalize(nr_ice_ctx *ctx, nr_ice_peer_ctx *pctx);
>  int nr_ice_ctx_set_stun_servers(nr_ice_ctx *ctx,nr_ice_stun_server *servers, int ct);
>  int nr_ice_ctx_set_turn_servers(nr_ice_ctx *ctx,nr_ice_turn_server *servers, int ct);
> +int nr_ice_ctx_set_resolver(nr_ice_ctx *ctx, nr_resolver *resolver);

The whole initialization process here seems unnecessarily intricate -- I'm seeing what looks like superfluous plumbing in both the fake resolver and the unittests just to ferry these resolver objects around after they've been created.

It seems that things could be simplified dramatically by changing the signature here to simply be:

int nr_ice_ctx_set_resolver(nr_ice_ctx *ctx, void *obj, nr_resolver_vtbl *vtbl);

::: media/mtransport/third_party/nICEr/src/net/nr_resolver.c
@@ +40,5 @@
> +#include <nr_api.h>
> +#include "nr_resolver.h"
> +
> +int nr_resolver_create_int(void *obj, nr_resolver_vtbl *vtbl,
> +			   nr_resolver **resolverp)

Tabs

::: media/mtransport/third_party/nICEr/src/net/nr_resolver.h
@@ +55,5 @@
> +typedef struct nr_resolver_ {
> +  void *obj;
> +  nr_resolver_vtbl *vtbl;
> +} nr_resolver;
> +

I think we want a bit more documentation in here about the responsibilities of the resolver implementation, such as a description of how one interacts with the nr_resolver_create_int function, and the contract for the vtbl ("resolve()" in particular). At the very least, a pointer to NrIceResolverFake would give implementors a good idea of how this hangs together.

Some discussion of lifetime is probably warranted also (e.g., "destroy" is called when a ctx goes away, and doesn't necessarily signal that the resolver must be destroyed at that time).

@@ +57,5 @@
> +  nr_resolver_vtbl *vtbl;
> +} nr_resolver;
> +
> +int nr_resolver_create_int(void *obj, nr_resolver_vtbl *vtbl,
> +			   nr_resolver **resolverp);

Tabs

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c
@@ +220,5 @@
> +    int _status;
> +
> +    switch(addr->ip_version){
> +      case NR_IPV4:
> +	addr->u.addr4.sin_port=htons(port);

Tab

Comment 10

[Eric Rescorla (:ekr)]

Attachment: DNS resolution for STUN servers

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 719749 [details] [diff] [review]
DNS resolution for STUN servers

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

::: media/mtransport/nriceresolverfake.cpp
@@ +172,5 @@
> +}
> +
> +int NrIceResolverFake::cancel(void *obj, void *handle) {
> +  MOZ_ASSERT(obj);
> +  NrIceResolverFake *fake = static_cast<NrIceResolverFake *>(obj);

nriceresolverfake.cpp:176:22: error: unused variable 'fake' [-Werror,-Wunused-variable]
  NrIceResolverFake *fake = static_cast<NrIceResolverFake *>(obj);
                     ^
1 error generated.
make[6]: *** [nriceresolverfake.o] Error 1

https://tbpl.mozilla.org/?tree=Try&rev=d801e203dfbd

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 719749 [details] [diff] [review]
DNS resolution for STUN servers

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

::: media/mtransport/nriceresolverfake.h
@@ +84,5 @@
> +
> +  // Get an address.
> +  const PRNetAddr *Resolve(const std::string& hostname) {
> +    if (!addrs_.count(hostname))
> +      return nullptr;

Fedora only:

In file included from nriceresolverfake.cpp:56:0:
../../../../media/mtransport/nriceresolverfake.h: In member function 'const PRNetAddr* mozilla::NrIceResolverFake::Resolve(const std::string&)':
../../../../media/mtransport/nriceresolverfake.h:88:14: error: 'nullptr' was not declared in this scope
make[5]: *** [nriceresolverfake.o] Error 1

Not sure what's going on.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Forgot the link: https://tbpl.mozilla.org/?tree=Try&rev=06f6a1fc1d1e

Comment 14

[Adam Roach [:abr]]

Comment on attachment 719749 [details] [diff] [review]
DNS resolution for STUN servers

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

Looks good to me. One minor comment inline (and a response to jib's discovery); the rest are whitespace nits.

::: media/mtransport/nriceresolverfake.cpp
@@ +116,5 @@
> +  MOZ_ASSERT(fake->allocated_resolvers_ > 0);
> +
> +  PendingResolution *pending = new PendingResolution(fake,
> +                                                     resource->domain_name,
> +                                                     resource->port ? 

WS

@@ +158,5 @@
> +    if (r)
> +      goto abort;
> +
> +    pending->cb_(pending->cb_arg_, &transport_addr);
> +      

WS

::: media/mtransport/nriceresolverfake.h
@@ +84,5 @@
> +
> +  // Get an address.
> +  const PRNetAddr *Resolve(const std::string& hostname) {
> +    if (!addrs_.count(hostname))
> +      return nullptr;

Yeah, this is a little wack. It appears that the Linux builds are using gcc-4.3.3, while gcc didn't add "nullptr" support until 4.6.0. I find a few instances of '#include "mozilla/NullPtr.h"' in the code, but it's not obvious which of those you're expected to include (or transitively include) to make "nullptr" work.

Given that this is test driver code, you might get away with including NullPtr.h directly -- it's not without precedent.

@@ +93,5 @@
> +
> +  struct PendingResolution {
> +    PendingResolution(NrIceResolverFake *resolver,
> +                      const std::string& hostname,
> +                      uint16_t port,

*Eventually* -- when we support S-NAPTR looksup for TURN servers -- part of the output of this operation can be transport to use. It would make the API more future-proof if we included that as a parameter now.

::: media/mtransport/test/ice_unittest.cpp
@@ +100,5 @@
> +
> +  void AddAddressToResolver(const std::string hostname,
> +                            const std::string address) {
> +    PRNetAddr addr;
> +    

WS

@@ +214,5 @@
>  
>    void Shutdown() {
>      ice_ctx_ = nullptr;
>    }
> +  

WS

@@ +291,5 @@
> +    peer_->Gather();
> +
> +    ASSERT_TRUE_WAIT(peer_->gathering_complete(), 10000);
> +  }
> +  

WS

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +138,5 @@
> +
> +      case SERVER_REFLEXIVE:
> +        if(r=nr_ice_candidate_format_stun_label(label, sizeof(label),cand))
> +          ABORT(r);
> +	break;

Tab

@@ +143,5 @@
> +
> +      case RELAYED:
> +        if(r=nr_ice_candidate_format_stun_label(label, sizeof(label),cand))
> +          ABORT(r);
> +	break;

Tab

@@ +346,5 @@
>        case PEER_REFLEXIVE:
>          if(r=NR_reg_get_uchar(NR_ICE_REG_PREF_TYPE_PEER_RFLX,&type_preference))
>            ABORT(r);
>          stun_priority=0;
> +	break;

Tab

@@ +510,5 @@
> +
> +    switch(cand->type){
> +      case HOST:
> +        assert(0); /* Can't happen */
> +	ABORT(R_INTERNAL);

Tab

@@ +570,3 @@
>    {
>      int r,_status;
> +	

WS. A tab, in fact.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +242,5 @@
>      while(cand){
>        if(cand->state!=NR_ICE_CAND_STATE_INITIALIZING){
>          if(r=nr_ice_candidate_initialize(cand,nr_ice_initialize_finished_cb,ctx)){
>            if(r!=R_WOULDBLOCK){
> +	    ctx->uninitialized_candidates--;

Tab

::: media/mtransport/third_party/nICEr/src/net/nr_resolver.h
@@ +50,5 @@
> +
> +typedef struct nr_resolver_vtbl_ {
> +  int (*destroy)(void **obj);
> +  int (*resolve)(void *obj,
> +		 nr_resolver_resource *resource,

Tabs

@@ +79,5 @@
> +  the resources associated with obj. No other function will
> +  be called afterwards.
> +*/
> +int nr_resolver_create_int(void *obj, nr_resolver_vtbl *vtbl,
> +			   nr_resolver **resolverp);

Tabs

@@ +84,5 @@
> +int nr_resolver_destroy(nr_resolver **resolverp);
> +
> +/* Request resolution of a domain */
> +int nr_resolver_resolve(nr_resolver *resolver,
> +			nr_resolver_resource *resource,

Tabs

::: media/mtransport/third_party/nICEr/src/net/transport_addr.c
@@ +220,5 @@
> +    int _status;
> +
> +    switch(addr->ip_version){
> +      case NR_IPV4:
> +	addr->u.addr4.sin_port=htons(port);

Tab

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: DNS resolution for STUN servers. r=abr

Updated with nits. Carrying forward r+ from adam.
- Compiles in opt builds and on fedora

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 720990 [details] [diff] [review]
DNS resolution for STUN servers. r=abr

crashtests appear fine with this patch alone, so the problem must be in the patch in bug 837919.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: DNS resolution for STUN servers. r=abr

Whoops. Had the nullptr fix in the wrong patch. Now included.

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

See Bug 837919, Comment 23.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=5b0b7232db64

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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