conncheck: fix pair priorities uniqueness

This patch fixes the priority assigned to a peer reflexive discovered
local candidate, when the agent has the stun client role and receives an
stun reply. This priority must be the value put in the stun request, ie
the pair->rflx_priority from the parent pair. This ensures two similar
ordered pairs, will generate discovered pairs ordered in the same way
for the stun client, and also for the stun server on the other side.
Without this identical ordered on both sides of the connections, the two
agents may nominate a different pair with the aggresive nomination
scenario, since both are valid.

The other fix concerns the function that ensures local candidates
priority uniqueness, that breaks the assumption that "two local
candidates having the same priority should generate the same
prflx_priority in the pairs they contribute". Respecting this assumption
is important to stay coherent with the behavior of the other agent, that
considers that two stun requests coming from the same peer-reflexive
remote candidate will have the same remote priority (once a remote
candidate is added to the component remote_candidates list, its priority
is not supposed to change).

3 files changed, 51 insertions, 22 deletions

diff --git a/agent/conncheck.c b/agent/conncheck.c
index 5ece935..369dd0a 100644
--- a/agent/conncheck.c
+++ b/agent/conncheck.c
@@ -2071,17 +2071,47 @@ ensure_unique_priority (NiceStream *stream, NiceComponent *component,
     }
   }
 
+  return priority;
+}
+
+static guint32
+ensure_unique_prflx_priority (NiceStream *stream, NiceComponent *component,
+    guint32 local_priority, guint32 prflx_priority)
+{
+  GSList *item;
+
+  /* First, ensure we provide the same value for pairs having
+   * the same local candidate, ie the same local candidate priority
+   * for the sake of coherency with the stun server behaviour that
+   * stores a unique priority value per remote candidate, from the
+   * first stun request it receives (it depends on the kind of NAT
+   * typically, but for NAT that preserves the binding this is required).
+   */
   for (item = stream->conncheck_list; item; item = item->next) {
     CandidateCheckPair *p = item->data;
 
     if (p->component_id == component->id &&
-        p->prflx_priority == priority) {
-      priority--;
+        p->local->priority == local_priority) {
+      return p->prflx_priority;
+    }
+  }
+
+ /* Second, ensure uniqueness across all other prflx_priority values */
+ again:
+  if (prflx_priority == 0)
+    prflx_priority--;
+
+  for (item = stream->conncheck_list; item; item = item->next) {
+    CandidateCheckPair *p = item->data;
+
+    if (p->component_id == component->id &&
+        p->prflx_priority == prflx_priority) {
+      prflx_priority--;
       goto again;
     }
   }
 
-  return priority;
+  return prflx_priority;
 }
 
 
@@ -2128,8 +2158,8 @@ static CandidateCheckPair *priv_add_new_check_pair (NiceAgent *agent,
           tmpbuf1, nice_address_get_port (&pair->local->addr),
           tmpbuf2, nice_address_get_port (&pair->remote->addr));
   }
-  pair->prflx_priority = ensure_unique_priority (stream, component,
-      peer_reflexive_candidate_priority (agent, local));
+  pair->prflx_priority = ensure_unique_prflx_priority (stream, component,
+      local->priority, peer_reflexive_candidate_priority (agent, local));
 
   stream->conncheck_list = g_slist_insert_sorted (stream->conncheck_list, pair,
       (GCompareFunc)conn_check_compare);
@@ -3049,6 +3079,7 @@ static CandidateCheckPair *priv_add_peer_reflexive_pair (NiceAgent *agent, guint
   }
   g_snprintf (pair->foundation, NICE_CANDIDATE_PAIR_MAX_FOUNDATION, "%s:%s",
       local_cand->foundation, parent_pair->remote->foundation);
+
   if (agent->controlling_mode == TRUE)
     pair->priority = nice_candidate_pair_priority (pair->local->priority,
         pair->remote->priority);
@@ -3056,7 +3087,8 @@ static CandidateCheckPair *priv_add_peer_reflexive_pair (NiceAgent *agent, guint
     pair->priority = nice_candidate_pair_priority (pair->remote->priority,
         pair->local->priority);
   pair->nominated = parent_pair->nominated;
-  pair->prflx_priority = ensure_unique_priority (stream, component,
+  pair->prflx_priority = ensure_unique_prflx_priority (stream, component,
+      local_cand->priority,
       peer_reflexive_candidate_priority (agent, local_cand));
   nice_debug ("Agent %p : added a new peer-discovered pair %p with "
       "foundation '%s' and transport %s:%s to stream %u component %u",
@@ -3172,10 +3204,14 @@ static CandidateCheckPair *priv_process_response_check_for_reflexive(NiceAgent *
       if (!agent->force_relay) {
         /* step: find a new local candidate, see RFC 5245 7.1.3.2.1.
          * "Discovering Peer Reflexive Candidates"
+         *
+         * The priority equal to the value of the PRIORITY attribute
+         * in the Binding request is taken from the "parent" pair p
          */
         local_cand = discovery_add_peer_reflexive_candidate (agent,
                                                              stream->id,
                                                              component->id,
+                                                             p->prflx_priority,
                                                             &mapped,
                                                              sockptr,
                                                              local_candidate,
diff --git a/agent/discovery.c b/agent/discovery.c
index 685c5b8..d8c4e9d 100644
--- a/agent/discovery.c
+++ b/agent/discovery.c
@@ -897,6 +897,7 @@ discovery_add_peer_reflexive_candidate (
   NiceAgent *agent,
   guint stream_id,
   guint component_id,
+  guint32 priority,
   NiceAddress *address,
   NiceSocket *base_socket,
   NiceCandidate *local,
@@ -927,22 +928,13 @@ discovery_add_peer_reflexive_candidate (
   candidate->addr = *address;
   candidate->sockptr = base_socket;
   candidate->base_addr = base_socket->addr;
-
-  if (agent->compatibility == NICE_COMPATIBILITY_GOOGLE) {
-    candidate->priority = nice_candidate_jingle_priority (candidate);
-  } else if (agent->compatibility == NICE_COMPATIBILITY_MSN ||
-             agent->compatibility == NICE_COMPATIBILITY_OC2007)  {
-    candidate->priority = nice_candidate_msn_priority (candidate);
-  } else if (agent->compatibility == NICE_COMPATIBILITY_OC2007R2) {
-    candidate->priority =  nice_candidate_ms_ice_priority (candidate,
-        agent->reliable, FALSE);
-  } else {
-    candidate->priority = nice_candidate_ice_priority (candidate,
-        agent->reliable, FALSE);
-  }
-
-  candidate->priority = ensure_unique_priority (stream, component,
-      candidate->priority);
+  /* We don't ensure priority uniqueness in this case, since the
+   * discovered candidate receives the same priority than its
+   * parent pair, by design, RFC 5245, sect 7.1.3.2.1.
+   * Discovering Peer Reflexive Candidates (the priority from the
+   * STUN Request)
+   */
+  candidate->priority = priority;
   priv_assign_foundation (agent, candidate);
 
   if ((agent->compatibility == NICE_COMPATIBILITY_MSN ||
diff --git a/agent/discovery.h b/agent/discovery.h
index 976118b..270c883 100644
--- a/agent/discovery.h
+++ b/agent/discovery.h
@@ -148,6 +148,7 @@ discovery_add_peer_reflexive_candidate (
   NiceAgent *agent,
   guint stream_id,
   guint component_id,
+  guint32 priority,
   NiceAddress *address,
   NiceSocket *base_socket,
   NiceCandidate *local,