[Nice] R: patch for TURN proxy and more

Mon Feb 13 12:01:26 PST 2012

On 02/13/2012 08:40 AM, Madaro Livio wrote:
> Hi Youness,
> thanks for reviewing the patches.
> 
> Here are my comments:
> 
> 1) OK
> 
> 2) I think your fix is better than mine. I tested it for http proxy and it works.
Ok thanks, I will commit a fix using that solution. Thanks for testing.

> 
> 3) I have a use case where len < 0 but the socket is still working. It is when the stun server is not listening and the client receives ICMP messages. I think it is a Windows specific problem. Calling WSAGetLastError() returns WSAECONNRESET.
> The MSDN documentation sais: "On a UDP-datagram socket, this error would indicate that a previous send operation resulted in an ICMP "Port Unreachable" message." ( http://msdn.microsoft.com/en-us/library/windows/desktop/ms740121(v=vs.85).aspx ).
> Thinking again about the patch, I think that the problem would be fixed by adding a check in the function socket_recv in udp-bsd.c
> 
>   if (recvd < 0) {
> #ifdef G_OS_WIN32
>     if (WSAGetLastError () == WSAEWOULDBLOCK || WSAGetLastError () == WSAECONNRESET)
> #else
>     if (errno == EAGAIN || errno == EWOULDBLOCK)
> #endif
>       return 0;
>   }
> 
Humm ok, I haven't thought of this use case because I always assumed UDP sockets
could never get an error. And you're right, if udp gets unreachable, it
shouldn't close it because of the whole peer-reflexive stuff.
I will fix it with this new diff you gave for WSAECONNRESET and hope it's the
only issue this raises. Otherwise I may just make it never return -1 in
socket/udp-bsd.c

> 
> 4) I agree. I will try the git version.
Thanks, let me know if you can reproduce that bug, and if yes, provide logs so
we can figure out why it happens.

> 
> 
> Regards,
> Livio
> 
> 
> -----Messaggio originale-----
> Da: nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org [mailto:nice-bounces+livio.madaro=telecomitalia.it at lists.freedesktop.org] Per conto di Youness Alaoui
> Inviato: sabato 11 febbraio 2012 00:49
> A: nice at lists.freedesktop.org
> Oggetto: Re: [Nice] patch for TURN proxy and more
> 
> 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
> 
> 
> 
> 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/20120213/dc745288/attachment.pgp>