786236 - Add per-PeerConnection STUN configuration

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

Currently, nICEr expects to have a single system global STUN/TURN configuration. We need to make that per-ctx and expose that to the API.

Comment 1

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

Exposing it to the JS level might end up being a separate bug, or will need consultation from myself and/or ehugg and/or ekr.

Comment 2

[Eric Rescorla (:ekr)]

This might be a good one for Jan-Ivar.

Comment 3

[Eric Rescorla (:ekr)]

This needs the following work, from bottom to top:

1. Add a call nr_ice_set_stun_servers() [and for bonus points nr_ice_set_turn_servers)
    to media/mtransport/third_party/nICEr/src/ice/ice_ctx.c. Model it on nr_ice_fetch_{stun,turn}_servers().
    If you really ewant to get fancy, have it extend the list.

2. Suppress the global STUN configuration setting in:
    NrIceCtx::Create() [look for ice.stun.server.0.*].

   I would comment this out. If we provide a moz stun server, we may want to revisit it.

3. Add NrIceCtx::SetStunServers() and wire up below in media/mtransport/nricectx.cpp
4. Add code to the PC to read the configuration variables and parse out the STUN/TURN URLs
5. Wire the PC code up to the JS

3The change you need to n

Comment 4

[Eric Rescorla (:ekr)]

jib: are you planning to do this soonish or do you have more important things? I think it may be in the way of my work on ICE in a bit...

Comment 5

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

I'll work on this one today.

Comment 6

[Eric Rescorla (:ekr)]

jib: have you worked on this at all? If not, I'm going to do it...

Comment 7

[Maire Reavy [:mreavy]]

ekr -- can you do this?  jib is working on a blocker gUM bug after he lands his patches that are currently in flight.

Comment 8

[Eric Rescorla (:ekr)]

No problem. I just didn't want to start on it if he was already mostly done.

Comment 9

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

Thanks, I haven't worked on it.

Comment 10

[Eric Rescorla (:ekr)]

Attachment: Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS

Comment 11

[Eric Rescorla (:ekr)]

Attachment: Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS

Comment 12

[Adam Roach [:abr]]

Comment on attachment 696730 [details] [diff] [review]
Per-ctx configurable STUN servers

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

Looks fundamentally good. I've added some strong recommendations for additional safeguards, and a couple of nits, but nothing that changes the bones of the patch, so I'm r+'ing it.

::: media/mtransport/nricectx.cpp
@@ -275,5 @@
>        NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
>      }
>  
> -    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");
> -    NR_reg_set_uint2((char *)"ice.stun.server.0.port",3478);

With the removal of a default server, the code in nr_ice_fetch_stun_servers will have a ct of 0, which means RCALLOC(sizeof(nr_ice_stun_server)*ct)) might return NULL (see http://c-faq.com/ansi/malloc0.html). The current code will abort context initialization if that happens.

Basically, I think we need to add a check at the top of nr_ice_fetch_stun_servers that checks ct against 0, and returns success if such is the case. This might not be an issue for the Mozilla tree and its PR implementation, but it will certainly help with portability when we push this patch upstream.

Alternately, you could simply avoid calling nr_ice_fetch_stun_servers from nr_ice_ctx_create if stun_server_ct == 0.

::: media/mtransport/nricectx.h
@@ +78,5 @@
>  
> +
> +struct NrIceStunServer {
> + public:
> +  static NrIceStunServer* Create(const std::string& addr, uint16_t port) {

If we're expecting the layer above us to do name -> address resolution, isn't passing the IP address as a string going to be a bit cumbersome?

In normal operation, we'll have a DNS name coming in from the javascript, which is then resolved into an IP address, serialized as a human-readable string, and then re-parsed into an IP address. Wouldn't it make more sense to have the upper layer pass this down as a PRNetAddr in the first place? At the very least, I would propose having a Create() that takes PRNetAddr as its parameters rather than the string.

::: media/mtransport/test/ice_unittest.cpp
@@ +79,5 @@
> +    ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> +        std::string((char *)"216.93.246.14"), 3478));
> +    stun_servers.push_back(*server);
> +    ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->SetStunServers(stun_servers)));
> +    

Whitespace

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c
@@ +113,5 @@
>    }
>  
> +int nr_ice_ctx_set_stun_servers(nr_ice_ctx *ctx,nr_ice_stun_server *servers,int ct)
> +  {
> +    int r,_status;

Variable 'r' is unused.

@@ +135,5 @@
> +  }
> +
> +int nr_ice_ctx_set_turn_servers(nr_ice_ctx *ctx,nr_ice_turn_server *servers,int ct)
> +  {
> +    int r,_status;

Variable 'r' is unused.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +83,5 @@
> +  // settings.
> +  std::vector<NrIceStunServer> stun_servers;
> +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> +      std::string((char *)"216.93.246.14"), 3478));
> +  stun_servers.push_back(*server);

I know we're hardcoding the input here, so the chance of a parsing error when creating the stun server object is approximately zero -- but I'm a bit antsy about the prospect of calling into code that has a contract allowing it to return null and then passing that returned pointer into a function that doesn't check for null. I'm worried that leaving this pattern here (without a null check) may lead a future implementor to copy the pattern (say, when implementing the final solution).

tl;dr: can we check for null before calling SetStunServers (or add code to SetStunServers that checks for null when traversing the vector)?

Comment 13

[Eric Rescorla (:ekr)]

> Looks fundamentally good. I've added some strong recommendations for
> additional safeguards, and a couple of nits, but nothing that changes the
> bones of the patch, so I'm r+'ing it.
> 
> ::: media/mtransport/nricectx.cpp
> @@ -275,5 @@
> >        NR_reg_set_uchar((char *)"ice.pref.interface.wlan0", 232);
> >      }
> >  
> > -    NR_reg_set_string((char *)"ice.stun.server.0.addr", (char *)"216.93.246.14");
> > -    NR_reg_set_uint2((char *)"ice.stun.server.0.port",3478);
> 
> With the removal of a default server, the code in nr_ice_fetch_stun_servers
> will have a ct of 0, which means RCALLOC(sizeof(nr_ice_stun_server)*ct))
> might return NULL (see http://c-faq.com/ansi/malloc0.html). The current code
> will abort context initialization if that happens.
> 
> Basically, I think we need to add a check at the top of
> nr_ice_fetch_stun_servers that checks ct against 0, and returns success if
> such is the case. This might not be an issue for the Mozilla tree and its PR
> implementation, but it will certainly help with portability when we push
> this patch upstream.
> 
> Alternately, you could simply avoid calling nr_ice_fetch_stun_servers from
> nr_ice_ctx_create if stun_server_ct == 0.

This seems like good advice.



> ::: media/mtransport/nricectx.h
> @@ +78,5 @@
> >  
> > +
> > +struct NrIceStunServer {
> > + public:
> > +  static NrIceStunServer* Create(const std::string& addr, uint16_t port) {
> 
> If we're expecting the layer above us to do name -> address resolution,
> isn't passing the IP address as a string going to be a bit cumbersome?
> 
> In normal operation, we'll have a DNS name coming in from the javascript,
> which is then resolved into an IP address, serialized as a human-readable
> string, and then re-parsed into an IP address. Wouldn't it make more sense
> to have the upper layer pass this down as a PRNetAddr in the first place? At
> the very least, I would propose having a Create() that takes PRNetAddr as
> its parameters rather than the string.

I think this is a good argument. I will do so.



> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +83,5 @@
> > +  // settings.
> > +  std::vector<NrIceStunServer> stun_servers;
> > +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(
> > +      std::string((char *)"216.93.246.14"), 3478));
> > +  stun_servers.push_back(*server);
> 
> I know we're hardcoding the input here, so the chance of a parsing error
> when creating the stun server object is approximately zero -- but I'm a bit
> antsy about the prospect of calling into code that has a contract allowing
> it to return null and then passing that returned pointer into a function
> that doesn't check for null. I'm worried that leaving this pattern here
> (without a null check) may lead a future implementor to copy the pattern
> (say, when implementing the final solution).
> 
> tl;dr: can we check for null before calling SetStunServers (or add code to
> SetStunServers that checks for null when traversing the vector)?

Willdo.

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Per-ctx configurable STUN servers

* * *
Configure STUN servers from JS

Comment 15

[Eric Rescorla (:ekr)]

Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

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

I revised this. Can you re-review?

Comment 16

[Adam Roach [:abr]]

Comment on attachment 698334 [details] [diff] [review]
Per-ctx configurable STUN servers

lgtm

Comment 17

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

Attachment: Per-ctx configurable STUN servers. r=adam

Patch got stale because of Bug 807494 landing. Refreshed.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eda7bbdfedf

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0eda7bbdfedf