[libnice] [RFC PATCH] Handling multiple candidates with same foundation in a component.

[Philip Withnall]

Hi Simone,

Thanks for the patch — from a quick glance, it looks like a reasonable
explanation and fix for your problem.

Can you please attach it to Phabricator? We prefer the patch review
interface there. :-)  CC me to the report (pwithnall).

Philip

On Mon, 2015-12-14 at 13:44 +0100, Simone Gotti wrote:
> Hi,
> 
> I debugged some strange webrtc failed connections between browsers
> (firefox
> and chrome) and janus gateway and noticed that they were caused by
> libnice
> sending data to the wrong remote candidate udp port. libnice debug
> reported the
> right pair candidates addresses but then the wrong remote candidates
> were used.
> 
> I noticed that the cause was related to having multiple remote
> candidates with
> identical foundation in the same component These remote candidates
> were of
> two kind:
> 
> *) peer reflexive remote candidates discovered by libnice (more than
> one
> candidate with foundation named "remote-1").
> 
> *) remote candidates added by janus (in this case they were wrongly
> added and
> I have a fix for janus, but adding multiple remote candidates with
> same
> foundation can probably happen).
> 
> in agent/component.c:nice_component_find_pair, the candidate's
> foundation is
> used to find the related candidate in the component. When multiple
> candidates
> have the same foundation the first one is returned but it cannot be
> the right
> one.
> 
> If I read the ICE rfc correctly, multiple candidate with the same
> foundation in
> a component should be handled.
> 
> I made this RFC patch and it fixed the problem. Perhaps there can be
> better ways to handle this but I think it can be a good start for a
> discussion.
> If I need to use phabricator please let me know (I feel more
> comfortable
> with emails).
> 
> Thanks!
> 
> Simone
> 
> 
> 
> From 0376dd3373c1740fc5ef86254669a7d153ae2201 Mon Sep 17 00:00:00
> 2001
> From: Simone Gotti <simone.gotti at gmail.com>
> Date: Sun, 13 Dec 2015 17:28:53 +0100
> Subject: [PATCH] agent: handle multiple candidates with same
> foundation per
>  component.
> 
> nice_component_find_pair finds the local and remote candidates using
> the
> provided foundations, since there can be multiple candidates with the
> same
> foundation, the wrong candidate can be chosen.
> 
> This patch changes nice_component_find_pair to also check the
> candidate
> address.
> 
> The previous version of nice_component_find_pair has been renamed to
> nice_component_find_pair_by_foundation since it's used by the
> exported function
> nice_agent_set_selected_pair.
> 
> Signed-off-by: Simone Gotti <simone.gotti at gmail.com>
> ---
>  agent/agent.c     |  2 +-
>  agent/component.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  agent/component.h |  7 ++++++-
>  agent/conncheck.c |  4 ++--
>  4 files changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/agent/agent.c b/agent/agent.c
> index 279e7cd..1dc6ef2 100644
> --- a/agent/agent.c
> +++ b/agent/agent.c
> @@ -5048,7 +5048,7 @@ nice_agent_set_selected_pair (
>      goto done;
>    }
>  
> -  if (!nice_component_find_pair (component, agent, lfoundation,
> rfoundation, &pair)){
> +  if (!nice_component_find_pair_by_foundation (component, agent,
> lfoundation, rfoundation, &pair)){
>      goto done;
>    }
>  
> diff --git a/agent/component.c b/agent/component.c
> index d6712f4..1c795be 100644
> --- a/agent/component.c
> +++ b/agent/component.c
> @@ -286,18 +286,57 @@ nice_component_close (NiceComponent *cmp)
>  }
>  
>  /*
> - * Finds a candidate pair that has matching foundation ids.
> + * Finds a candidate pair that has matching local and remote
> candidates.
>   *
>   * @return TRUE if pair found, pointer to pair stored at 'pair'
>   */
>  gboolean
> -nice_component_find_pair (NiceComponent *cmp, NiceAgent *agent,
> const gchar *lfoundation, const gchar *rfoundation, CandidatePair
> *pair)
> +nice_component_find_pair (NiceComponent *cmp, NiceAgent *agent,
> NiceCandidate *lcandidate, NiceCandidate *rcandidate, CandidatePair
> *pair)
>  {
>    GSList *i;
>    CandidatePair result = { 0, };
>  
>    for (i = cmp->local_candidates; i; i = i->next) {
>      NiceCandidate *candidate = i->data;
> +    if (strncmp (candidate->foundation, lcandidate->foundation,
> NICE_CANDIDATE_MAX_FOUNDATION) == 0 &&
> +        nice_address_equal(&candidate->addr, &lcandidate->addr)) {
> +      result.local = candidate;
> +      break;
> +    }
> +  }
> +
> +  for (i = cmp->remote_candidates; i; i = i->next) {
> +    NiceCandidate *candidate = i->data;
> +    if (strncmp (candidate->foundation, rcandidate->foundation,
> NICE_CANDIDATE_MAX_FOUNDATION) == 0 &&
> +        nice_address_equal(&candidate->addr, &rcandidate->addr)) {
> +      result.remote = candidate;
> +      break;
> +    }
> +  }
> +
> +  if (result.local && result.remote) {
> +    result.priority = agent_candidate_pair_priority (agent,
> result.local, result.remote);
> +    if (pair)
> +      *pair = result;
> +    return TRUE;
> +  }
> +
> +  return FALSE;
> +}
> +
> +/*
> + * Finds a candidate pair that has matching foundation ids.
> + *
> + * @return TRUE if pair found, pointer to pair stored at 'pair'
> + */
> +gboolean
> +nice_component_find_pair_by_foundation (NiceComponent *cmp,
> NiceAgent *agent, const gchar *lfoundation, const gchar *rfoundation,
> CandidatePair *pair)
> +{
> +  GSList *i;
> +  CandidatePair result = { 0, };
> +
> + for (i = cmp->local_candidates; i; i = i->next) {
> +    NiceCandidate *candidate = i->data;
>      if (strncmp (candidate->foundation, lfoundation,
> NICE_CANDIDATE_MAX_FOUNDATION) == 0) {
>        result.local = candidate;
>        break;
> @@ -322,6 +361,7 @@ nice_component_find_pair (NiceComponent *cmp,
> NiceAgent *agent, const gchar *lfo
>    return FALSE;
>  }
>  
> +
>  /*
>   * Resets the component state to that of a ICE restarted
>   * session.
> diff --git a/agent/component.h b/agent/component.h
> index bc5d0c7..e4e0dab 100644
> --- a/agent/component.h
> +++ b/agent/component.h
> @@ -238,9 +238,14 @@ void
>  nice_component_close (NiceComponent *component);
>  
>  gboolean
> -nice_component_find_pair (NiceComponent *component, NiceAgent
> *agent,
> +nice_component_find_pair (NiceComponent *cmp, NiceAgent *agent,
> +    NiceCandidate *lcandidate, NiceCandidate *rcandidate,
> CandidatePair *pair);
> +
> +gboolean
> +nice_component_find_pair_by_foundation (NiceComponent *component,
> NiceAgent *agent,
>      const gchar *lfoundation, const gchar *rfoundation,
> CandidatePair *pair);
>  
> +
>  void
>  nice_component_restart (NiceComponent *component);
>  
> diff --git a/agent/conncheck.c b/agent/conncheck.c
> index 60d2fbf..3fce9dd 100644
> --- a/agent/conncheck.c
> +++ b/agent/conncheck.c
> @@ -1212,8 +1212,8 @@ static gboolean priv_update_selected_pair
> (NiceAgent *agent, NiceComponent *comp
>    g_assert (component);
>    g_assert (pair);
>    if (pair->priority > component->selected_pair.priority &&
> -      nice_component_find_pair (component, agent, pair->local-
> >foundation,
> -          pair->remote->foundation, &cpair)) {
> +      nice_component_find_pair (component, agent, pair->local,
> +          pair->remote, &cpair)) {
>      nice_debug ("Agent %p : changing SELECTED PAIR for component %u:
> %s:%s "
>          "(prio:%" G_GUINT64_FORMAT ").", agent, component->id,
>          pair->local->foundation, pair->remote->foundation, pair-
> >priority);
-------------- 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/20151216/e4ec3b5b/attachment.sig>