From 2118cbae32a3bdd6a03c692d5188437b4e53890e Mon Sep 17 00:00:00 2001 From: Fabrice Bellet Date: Tue, 25 Jun 2019 09:19:05 +0200 Subject: 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). --- agent/conncheck.c | 48 ++++++++++++++++++++++++++++++++++++++++++------ agent/discovery.c | 24 ++++++++---------------- agent/discovery.h | 1 + 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, -- cgit v1.2.1