[libnice] Selecting server reflexive candidates

[Philip Withnall]

On Tue, 2015-06-02 at 11:32 +0200, Jakub Adam wrote:
> I would like to ask you for a review of the following libnice patch that tries to
> solve an issue with candidate selection that I've encountered when a server-reflexive
> IP address appeared in XOR-MAPPED-ADDRESS of an incoming connectivity check response.
> 
>   https://github.com/tieto/libnice/commit/b3d9430bb1c0fd54e468f676930e31cc1c4f205a

As Ilya says, please do open a bug report about this:

https://bugs.freedesktop.org/enter_bug.cgi?product=nice

Anyway, some review comments. Firstly, a description of a simple network
configuration which triggers this would be appreciated, and would make
the review a lot easier to reason about.

I _think_ the patch is correct with reference to the specification, but
I would want to see an example network configuration before accepting
it.

Effectively, it allows priv_add_peer_reflexive_pair() to be called with
the (mapped address of the) local candidate if local_cand_matches, which
wasn’t allowed before.

It limits priv_conn_check_unfreeze_related() to now only be called if a
new_pair is found, which seems to be more correct wrt §7.1.3.2.2 of the
specification[1] than the current implementation, which considers
‘adding to VALID LIST’ if only a local candidate matches, rather than
the whole pair.

However, since this patch changes
priv_process_response_check_for_peer_reflexive() to also check for
server reflexive candidates, I suggest renaming it to
priv_process_response_check_for_reflexive(). Its documentation and call
sites also need to be updated to indicate that it can no longer return
NULL in any case.

Finally, I would like some review from someone else who knows the ICE
specification better than me (Olivier or Youness).

Philip

[1]: https://tools.ietf.org/html/rfc5245#section-7.1.3.2.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/nice/attachments/20150605/75e73a2b/attachment.sig>