Correctly calculate priority

Author: mickel8

lib/ex_ice/priv/candidate.ex

@@ -27,8 +27,25 @@ defmodule ExICE.Priv.Candidate do
   @callback send_data(t(), :inet.ip_address(), :inet.port_number(), binary()) ::
               {:ok, t()} | {:error, term(), t()}
 
-  @spec priority(:inet.ip_address(), type()) :: integer()
-  def priority(address, type) do
+  @spec priority!(%{:inet.ip_address() => non_neg_integer()}, :inet.ip_address(), type()) ::
+          non_neg_integer()
+  def priority!(local_preferences, base_address, type) do
+    local_preference = Map.fetch!(local_preferences, base_address)
+    do_priority(local_preference, type)
+  end
+
+  @spec priority(%{:inet.ip_address() => non_neg_integer()}, :inet.ip_address(), type()) ::
+          {%{:inet.ip_address() => non_neg_integer()}, non_neg_integer()}
+  def priority(local_preferences, base_address, type) do
+    local_preference =
+      Map.get(local_preferences, base_address) || generate_local_preference(local_preferences)
+
+    local_preferences = Map.put(local_preferences, base_address, local_preference)
+
+    {local_preferences, do_priority(local_preference, type)}
+  end
+
+  defp do_priority(local_preference, type) do
     type_preference =
       case type do
         :host -> 126
@@ -37,15 +54,23 @@ defmodule ExICE.Priv.Candidate do
         :relay -> 0
       end
 
-    # That's not fully correct as according to RFC 8445 sec. 5.1.2.1 we should:
-    # * use value of 65535 when there is only one IP address
-    # * use different values when there are multiple IP addresses
-    # local_preference = 65_535
+    2 ** 24 * type_preference + 2 ** 8 * local_preference + 2 ** 0 * (256 - 1)
+  end
 
-    # just make sure that for different ip addresses, we have different local preference
-    local_preference = :erlang.phash2(address, 65535)
+  defp generate_local_preference(local_preferences, attempts \\ 200)
 
-    2 ** 24 * type_preference + 2 ** 8 * local_preference + 2 ** 0 * (256 - 1)
+  defp generate_local_preference(_local_preferences, 0),
+    do: raise("Couldn't generate local preference")
+
+  defp generate_local_preference(local_preferences, attempts) do
+    # this should give us a number from 0 to 2**16-1
+    <<pref::16>> = :crypto.strong_rand_bytes(2)
+
+    if Map.has_key?(local_preferences, pref) do
+      generate_local_preference(local_preferences, attempts - 1)
+    else
+      pref
+    end
   end
 
   @spec foundation(type(), :inet.ip_address() | String.t(), :inet.ip_address() | nil, atom()) ::

lib/ex_ice/priv/candidate_base.ex

@@ -35,22 +35,14 @@ defmodule ExICE.Priv.CandidateBase do
     transport = :udp
     address = Keyword.fetch!(config, :address)
 
-    priority =
-      if config[:priority] do
-        config[:priority]
-      else
-        base_address = Keyword.fetch!(config, :base_address)
-        ExICE.Priv.Candidate.priority(base_address, type)
-      end
-
     %__MODULE__{
       id: Utils.id(),
       address: address,
       base_address: config[:base_address],
       base_port: config[:base_port],
       foundation: Candidate.foundation(type, address, nil, transport),
       port: Keyword.fetch!(config, :port),
-      priority: priority,
+      priority: Keyword.fetch!(config, :priority),
       transport: transport,
       transport_module: Keyword.get(config, :transport_module, ExICE.Priv.Transport.UDP),
       socket: Keyword.fetch!(config, :socket),

lib/ex_ice/priv/gatherer.ex

@@ -84,9 +84,17 @@ defmodule ExICE.Priv.Gatherer do
     end)
   end
 
-  @spec gather_host_candidates(t(), [Transport.socket()]) :: [Candidate.t()]
-  def gather_host_candidates(gatherer, sockets) do
-    Enum.map(sockets, &create_new_host_candidate(gatherer, &1))
+  @spec gather_host_candidates(t(), %{:inet.ip_address() => non_neg_integer()}, [
+          Transport.socket()
+        ]) :: [Candidate.t()]
+  def gather_host_candidates(gatherer, local_preferences, sockets) do
+    {local_preferences, cands} =
+      Enum.reduce(sockets, {local_preferences, []}, fn socket, {local_preferences, cands} ->
+        {local_preferences, cand} = create_new_host_candidate(gatherer, local_preferences, socket)
+        {local_preferences, [cand | cands]}
+      end)
+
+    {local_preferences, Enum.reverse(cands)}
   end
 
   @spec gather_srflx_candidate(t(), integer(), Transport.socket(), ExSTUN.URI.t()) ::
@@ -155,21 +163,24 @@ defmodule ExICE.Priv.Gatherer do
     Keyword.get_values(int, :addr)
   end
 
-  defp create_new_host_candidate(gatherer, socket) do
+  defp create_new_host_candidate(gatherer, local_preferences, socket) do
     {:ok, {sock_ip, sock_port}} = gatherer.transport_module.sockname(socket)
 
+    {local_preferences, priority} = Candidate.priority(local_preferences, sock_ip, :host)
+
     cand =
       Candidate.Host.new(
         address: sock_ip,
         port: sock_port,
         base_address: sock_ip,
         base_port: sock_port,
+        priority: priority,
         transport_module: gatherer.transport_module,
         socket: socket
       )
 
     Logger.debug("New candidate: #{inspect(cand)}")
 
-    cand
+    {local_preferences, cand}
   end
 end

lib/ex_ice/priv/ice_agent.ex

@@ -86,6 +86,7 @@ defmodule ExICE.Priv.ICEAgent do
     sockets: [],
     local_cands: %{},
     remote_cands: %{},
+    local_preferences: %{},
     stun_servers: [],
     turn_servers: [],
     resolved_turn_servers: [],
@@ -305,7 +306,11 @@ defmodule ExICE.Priv.ICEAgent do
     ice_agent = change_gathering_state(ice_agent, :gathering)
 
     {:ok, sockets} = Gatherer.open_sockets(ice_agent.gatherer)
-    host_cands = Gatherer.gather_host_candidates(ice_agent.gatherer, sockets)
+
+    {local_preferences, host_cands} =
+      Gatherer.gather_host_candidates(ice_agent.gatherer, ice_agent.local_preferences, sockets)
+
+    ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences}
 
     ice_agent =
       Enum.reduce(host_cands, ice_agent, fn host_cand, ice_agent ->
@@ -1074,14 +1079,26 @@ defmodule ExICE.Priv.ICEAgent do
           {client.turn_ip, client.turn_port} | ice_agent.resolved_turn_servers
         ]
 
-        ice_agent = %{ice_agent | resolved_turn_servers: resolved_turn_servers}
+        # Use sock_addr for calculating priority.
+        # In other case, we might duplicate priority.
+        {:ok, {sock_addr, _sock_port}} = ice_agent.transport_module.sockname(tr.socket)
+
+        {local_preferences, priority} =
+          Candidate.priority(ice_agent.local_preferences, sock_addr, :relay)
+
+        ice_agent = %{
+          ice_agent
+          | resolved_turn_servers: resolved_turn_servers,
+            local_preferences: local_preferences
+        }
 
         relay_cand =
           Candidate.Relay.new(
             address: alloc_ip,
             port: alloc_port,
             base_address: alloc_ip,
             base_port: alloc_port,
+            priority: priority,
             transport_module: ice_agent.transport_module,
             socket: tr.socket,
             client: client
@@ -1741,12 +1758,18 @@ defmodule ExICE.Priv.ICEAgent do
       nil ->
         {:ok, {base_addr, base_port}} = ice_agent.transport_module.sockname(tr.socket)
 
+        {local_preferences, priority} =
+          Candidate.priority(ice_agent.local_preferences, base_addr, :srflx)
+
+        ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences}
+
         cand =
           Candidate.Srflx.new(
             address: xor_addr,
             port: xor_port,
             base_address: base_addr,
             base_port: base_port,
+            priority: priority,
             transport_module: ice_agent.transport_module,
             socket: tr.socket
           )
@@ -2058,12 +2081,16 @@ defmodule ExICE.Priv.ICEAgent do
       # TODO calculate correct prio and foundation
       local_cand = Map.fetch!(ice_agent.local_cands, conn_check_pair.local_cand_id)
 
+      priority =
+        Candidate.priority!(ice_agent.local_preferences, local_cand.base.base_address, :prflx)
+
       cand =
         Candidate.Prflx.new(
           address: xor_addr.address,
           port: xor_addr.port,
           base_address: local_cand.base.base_address,
           base_port: local_cand.base.base_port,
+          priority: priority,
           transport_module: ice_agent.transport_module,
           socket: local_cand.base.socket
         )
@@ -2808,7 +2835,19 @@ defmodule ExICE.Priv.ICEAgent do
     # priority sent to the other side has to be
     # computed with the candidate type preference of
     # peer-reflexive; refer to sec 7.1.1
-    priority = Candidate.priority(local_candidate.base.base_address, :prflx)
+    priority =
+      if local_candidate.base.type == :relay do
+        {:ok, {sock_addr, _sock_port}} =
+          ice_agent.transport_module.sockname(local_candidate.base.socket)
+
+        Candidate.priority!(ice_agent.local_preferences, sock_addr, :prflx)
+      else
+        Candidate.priority!(
+          ice_agent.local_preferences,
+          local_candidate.base.base_address,
+          :prflx
+        )
+      end
 
     attrs = [
       %Username{value: "#{ice_agent.remote_ufrag}:#{ice_agent.local_ufrag}"},

test/candidate_test.exs

@@ -8,21 +8,45 @@ defmodule ExICE.CandidateTest do
     port = 12_345
 
     %Candidate{foundation: f1} =
-      Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port)
+      Candidate.new(:host,
+        address: ip,
+        port: port,
+        base_address: ip,
+        base_port: port,
+        priority: 123
+      )
 
     %Candidate{foundation: f2} =
-      Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port)
+      Candidate.new(:host,
+        address: ip,
+        port: port,
+        base_address: ip,
+        base_port: port,
+        priority: 123
+      )
 
     assert f1 == f2
 
     ip2 = {192, 168, 1, 2}
     port2 = 23_456
 
     %Candidate{foundation: f1} =
-      Candidate.new(:host, address: ip, port: port, base_address: ip, base_port: port)
+      Candidate.new(:host,
+        address: ip,
+        port: port,
+        base_address: ip,
+        base_port: port,
+        priority: 123
+      )
 
     %Candidate{foundation: f2} =
-      Candidate.new(:host, address: ip2, port: port2, base_address: ip2, base_port: port2)
+      Candidate.new(:host,
+        address: ip2,
+        port: port2,
+        base_address: ip2,
+        base_port: port2,
+        priority: 122
+      )
 
     assert f1 != f2
   end

test/priv/candidate_pair_test.exs

@@ -13,11 +13,10 @@ defmodule ExICE.Priv.CandidatePairTest do
         port: port1,
         base_address: addr1,
         base_port: port1,
+        priority: 100,
         socket: nil
       )
 
-    c1 = %Candidate.Host{c1 | base: %{c1.base | priority: 100}}
-
     addr2 = {192, 168, 1, 2}
     port2 = 23_456
 
@@ -26,11 +25,10 @@ defmodule ExICE.Priv.CandidatePairTest do
         address: addr2,
         port: port2,
         base_address: addr2,
-        base_port: port2
+        base_port: port2,
+        priority: 200
       )
 
-    c2 = %ExICE.Candidate{c2 | priority: 200}
-
     c1c2 = CandidatePair.new(c1, c2, :controlling, :frozen)
     assert c1c2.priority == 429_496_730_000
 

test/priv/checklist_test.exs

@@ -17,6 +17,7 @@ defmodule ExICE.Priv.ChecklistTest do
         port: local_port,
         base_address: local_addr,
         base_port: local_port,
+        priority: 123,
         socket: nil
       )
 
@@ -25,15 +26,17 @@ defmodule ExICE.Priv.ChecklistTest do
         address: local_addr,
         port: local_port,
         base_address: local_addr,
-        base_port: local_port
+        base_port: local_port,
+        priority: 123
       )
 
     remote_srflx_cand =
       ExICE.Candidate.new(:srflx,
         address: remote_srflx_addr,
         port: remote_srflx_port,
         base_address: remote_addr,
-        base_port: remote_port
+        base_port: remote_port,
+        priority: 122
       )
 
     host_pair =

test/priv/conn_check_handler/controlled_test.exs

@@ -20,8 +20,8 @@ defmodule ExICE.Priv.ConnCheckHandler.ControlledTest do
     end
   end
 
-  @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445)
-  @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
+  @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123)
+  @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 122)
 
   describe "incoming binding request" do
     setup do

test/priv/conn_check_handler/controlling_test.exs

@@ -20,8 +20,8 @@ defmodule ExICE.Priv.ConnCheckHandler.ControllingTest do
     end
   end
 
-  @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445)
-  @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
+  @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445, priority: 123)
+  @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445, priority: 122)
 
   describe "incoming binding request" do
     setup do

test/priv/gatherer_test.exs

@@ -42,7 +42,11 @@ defmodule ExICE.Priv.GathererTest do
     assert {:ok, sockets} = Gatherer.open_sockets(gatherer)
 
     # there should only be one candidate
-    assert [%Candidate.Host{} = c] = Gatherer.gather_host_candidates(gatherer, sockets)
+    assert {local_preferences, [%Candidate.Host{} = c]} =
+             Gatherer.gather_host_candidates(gatherer, %{}, sockets)
+
+    assert Map.has_key?(local_preferences, c.base.address)
+    assert map_size(local_preferences) == 1
     assert c.base.address == {192, 168, 0, 1}
     assert c.base.base_address == {192, 168, 0, 1}
     assert c.base.port == c.base.base_port
@@ -65,7 +69,8 @@ defmodule ExICE.Priv.GathererTest do
 
     {:ok, sockets} = Gatherer.open_sockets(gatherer)
 
-    [%Candidate.Host{} = c] = Gatherer.gather_host_candidates(gatherer, sockets)
+    {_local_preferences, [%Candidate.Host{} = c]} =
+      Gatherer.gather_host_candidates(gatherer, %{}, sockets)
 
     assert :ok = Gatherer.gather_srflx_candidate(gatherer, 1234, c.base.socket, stun_server)
     assert packet = Transport.Mock.recv(c.base.socket)