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

[Simone Gotti]

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);
-- 
2.5.0