search

algesten / str0m

A Sans I/O WebRTC implementation in Rust.
MIT License
335 stars 50 forks source link

fix(ice): match remote candidate of stun request by priority #569

Closed thomaseizinger closed 1 month ago

thomaseizinger commented 1 month ago

Candidates are redundant when their protocol and address are identical. When such a redundant candidate is added, str0m correctly either rejects the newly formed pair or replaces the existing one in case the priority is better.

However, new candidate pairs are still formed as part of looking for the remote candidate of an incoming STUN binding. In the presence of redundant candidates, whether we pick the correct one currently depends on in which order the original candidates have been added.

To make this deterministic and prevent the forming of new, redundant candidate pairs, we now explicitly pick the remote candidate with the highest priority.

algesten commented 1 month ago

Whether it's a server reflexive or host in this scenario doesn't really matter does it?

thomaseizinger commented 1 month ago

Whether it's a server reflexive or host in this scenario doesn't really matter does it?

Correct, it doesn't really matter. Host candidates have a higher priority, so initially when the candidate pairs are formed, the ones with the server-reflexive candidate will be dropped.

Later on, when STUN binding requests are coming in, it may be the case that the server-reflexive candidate is found "first" in the list and then we str0m will form a new pair and constantly test both although they are identical. It is not much traffic but it is kind of unnecessary :man_shrugging:

thomaseizinger commented 1 month ago

Whether it's a server reflexive or host in this scenario doesn't really matter does it?

Correct, it doesn't really matter.

Or do you mean that this also applies if the identical host candidates are added multiple times?

algesten commented 1 month ago

I mean that network connectivity is there either way. it's more about having a sense of order to make this be the exact same on both sides.

thomaseizinger commented 1 month ago

I mean that network connectivity is there either way. it's more about having a sense of order to make this be the exact same on both sides.

I am not sure if we are talking about the same thing here. Yes network connectivity is there. What I am trying to fix is str0m needlessly creating a duplicate candidate pair and thus issuing twice as many STUN bindings as necessary.

I don't think this has anything to do with order on both sides? We cannot influence, in which order the user adds the remote candidates. The second candidate pair gets created because prior to this patch, there was no specified algorithm for how to find the correct candidate for an incoming STUN request. Instead, we just took the first one that matches by address and protocol even though that can just as easily be a candidate that we previously considered redundant because there is another one with a better priority.