Closed Bug 843644 Opened 7 years ago Closed 7 years ago

Allow STUN servers to be specified by DNS Names

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ekr, Assigned: ekr)

References

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch DNS resolution for STUN servers (obsolete) — Splinter Review
Assignee: nobody → 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?
Flags: needinfo?(gerv)
Attached patch DNS resolution for STUN servers (obsolete) — Splinter Review
Attachment #716589 - Attachment is obsolete: true
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.*
Attachment #716593 - Flags: feedback?(jib)
Attachment #716593 - Flags: feedback?(adam)
Note: this does pass unit testing on my machine, though I haven't tested end-to-end
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++?
Attachment #716593 - Flags: feedback?(jib)
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.
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
Flags: needinfo?(gerv)
Priority: -- → P2
Whiteboard: [WebRTC], [blocking-webrtc+]
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
Attachment #716593 - Flags: feedback?(adam) → feedback+
Attached patch DNS resolution for STUN servers (obsolete) — Splinter Review
Attachment #716593 - Attachment is obsolete: true
Attachment #719749 - Flags: review?(adam)
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 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 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
Attachment #719749 - Flags: review?(adam) → review+
Updated with nits. Carrying forward r+ from adam.
- Compiles in opt builds and on fedora
Attachment #719749 - Attachment is obsolete: true
Attachment #720990 - Flags: review+
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.
Attachment #720990 - Flags: checkin?(ekr)
Whoops. Had the nullptr fix in the wrong patch. Now included.
Attachment #720990 - Attachment is obsolete: true
Attachment #720990 - Flags: checkin?(ekr)
Attachment #721000 - Flags: review+
Attachment #721000 - Flags: checkin?(ekr)
Attachment #721000 - Flags: checkin?(ekr)
https://hg.mozilla.org/mozilla-central/rev/c7abfd72b8e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Keywords: verifyme
Keywords: verifyme
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in before you can comment on or make changes to this bug.