[Nice] patch for TURN proxy and more

[Youness Alaoui]

On 02/09/2012 06:18 AM, Madaro Livio wrote:
> Hi,
> I submit another patch for review. It addresses these issues in libnice0.1.1:

Hi again,
Thanks for the patches. Like I just suggested to John Selbie (I forgot to tell
you yesterday). It would probably be better if you committed those changes into
your local git, then send me the patch files generated by "git format-patch".

> 
> 1) If TURN server is not listening then nice_tcp_bsd_socket_new (in priv_add_new_candidate_discovery_turn) returns socket=NULL and libnice crashes. The patch check for socket==NULL
Good catch! thank you!
I've slightly modified it and committed it under your name :
http://cgit.collabora.com/git/libnice.git/commit/?id=0828f92e42e2279b511e218440358907f1e06090

> 
> 2) In the functions _nice_agent_recv and nice_turn_socket_parse_recv the receiving address is compared to the TURN server address. The test fails if the client is using a proxy.
> The patch adds specific test for proxy.
Humm.. you're right, but your patch isn't correct I believe.
You may get the same issue with socks proxy, not just http, also, you may get an
assertion if the proxy type is http but proxy_ip is NULL.
Also, this isn't forward-compatible if we add ice-tcp support, as it would be
non-turn that uses proxy socket too.
I think the proper fix for this would be to fix the underlying socket object to
report the original source ip/port rather than the proxy one.
Just like socket/tcp-bsd.c does :
  if (from)
    *from = priv->server_addr;
You should try doing :
  if (from)
    *from = priv->addr;
in both http.c and socks5.c. Try it and let me know if it works as expected.


> 
> 3) In function nice_agent_g_source_cb the glib source is destroyed if reading len < 0. Windows platform sometimes invoke the callback even if there are no data to read.  It happens for example when the STUN server is not listening. I think it is related to receiving ICMP.
> The patch check for G_IO_ERR before destroying the glib source
I'm not sure about that, if there is no data to read, it would return len == 0,
but if it returns len < 0,it means that there was an actual error with the
socket, and that would mean we need to close it and stop listening to it. Do you
have a use case where the socket is still valid and working but len is < 0?
Also, wouldn't it be better to do something like this :
if (condition == G_IO_ERR)
  len = -1;
else
  len = _nice_agent_recv (agent, stream, component, ctx->socket,
			  MAX_BUFFER_SIZE, buf);

Especially considering that you may not receive a G_IO_ERR but during the recv,
an error occurs.

> 
> 4) Using TURN server, the permission refresh is sent when the client sends data to the remote peer. The TURN permission refresh is not sent if the client is receiving data but is not transmitting (this happen if libnice (ICE) finds an asymmetrical path using TURN server for receiving and direct connection for transmitting).
> The patch adds the sending of TURN permission refresh in priv_permission_timeout. The side effect is that the permission is refreshed for every remote candidates even if not in use.
Humm... technically an asymetric path should not be happening. If we can use a
direct path for transmitting, then it means we have received a conncheck reply
on it, which means we can also be receiving data from that pair, without using TURN.
If you can reproduce that behavior and get me a log of that, I'll have a look
and see what fails to make the direct pair the selected one.
there was a bug that I fixed in the git version which didn't nominate a pair
even if it was successful, it was a rare use case that would happen if the
conncheck was sent before the peer received the remote candidates (thus, stores
the pending check in an early-icheck list) so when it tried to process it after
it received the remote candidates, it wasn't setting it to nominated if the
agent was controlled and the use-candidate flag was set in the conncheck. If you
also hit this same use case somehow, then this might be one reason why the peer
was not considering that pair as nominated and was using the TURN pair for
sending data.
I believe it's better to fix this bug than to do the permissions workaround that
you did. Especially considering that it means we'd be using a TURN relay even if
we don't need to, and it's best to keep TURN use as a fallback only.
(Note that even if we're not sending, the selected pair should be sending
keepalive connchecks every 30 seconds, which would ensure the permission is
always refreshed).

Thank you,
Youness.

> 
> Regards,
> Livio
> 
> 
> 
> 
> Questo messaggio e i suoi allegati sono indirizzati esclusivamente alle persone indicate. La diffusione, copia o qualsiasi altra azione derivante dalla conoscenza di queste informazioni sono rigorosamente vietate. Qualora abbiate ricevuto questo documento per errore siete cortesemente pregati di darne immediata comunicazione al mittente e di provvedere alla sua distruzione, Grazie.
> 
> This e-mail and any attachments is confidential and may contain privileged information intended for the addressee(s) only. Dissemination, copying, printing or use by anybody else is unauthorised. If you are not the intended recipient, please delete this message and any attachments and advise the sender by return e-mail, Thanks.
> 
> 
> 
> 
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20120210/41c754a7/attachment.pgp>