[Asterisk-code-review] res_rtp_asterisk: iterate all local addresses looking to populate ICE. (asterisk[13])

[Jaco Kroon]

Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13362 )

Change subject: res_rtp_asterisk: iterate all local addresses looking to populate ICE.
......................................................................


Patch Set 6:

(3 comments)

Some additional comments.  I personally think this should suffice but there are still one or two issues on which I need some guidance that I'm aware of.  There may be more.

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c 
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3157 
PS5, Line 3157: 				ast_sockaddr_to_pj_sockaddr(addr, &base);
> Not as far as I can determine no. […]
Correction:  Yes.  According to the way I understand RFC 5245 the base address (raddr) should be the address from whence we sent the STUN request, and rport the associated port.  So this enforces that in the case where addr for the socket is known (not ANY).  Otherwise the old behaviour is followed (which I suspect is wrong, we should use something similar to ast_ouraddrfor towards stunserver (might be good enough) since raddr should be the address we used to send to the STUN server.  This is how I understand it, could be wrong, and due to bug mentioned I also understand that the address used here should also be a candidate.

Happy to provide:

1.  Exact old behaviour (replace these lines with baseset = 0).
2.  As is currently (my recommendation).
3.  ast_outaddrfor (might be faster since we don't iterate the candidate set, but involves multiple system calls).

Your call and I'll abide by what you decide here, since I don't actually use STUN.

Currently, as is, this will be different from current if, and only if, RTC?P is bound to a specific address to begin with (As provided in addr), and that address isn't the first IPv4 address discovered.

There is a further possible change of ordering of ICE candidates in case of use of [ice_host_candidates].  Which does not actually influence base address selection.

The old code got a list of up to 32 addresses from PJPROJECT.  Let's say:

1.1.1.1; 1.1.1.2; 1.1.1.3

If we then override 1.1.1.1 to 1.1.1.4 *and* have include_local, the *old* code would have:

1.1.1.4; 1.1.1.2; 1.1.1.3; 1.1.1.1

New code will have:

1.1.1.4; 1.1.1.1; 1.1.1.2; 1.1.1.3

Since both iterate to find the first address both variants should still select 1.1.1.4 as the base addr (in case addr is ANY).

Also, if we end up hitting the 32 limit, the old code would discard early on found addresses, whereas, as per above ordering, I include earlier found addresses and their aliases.

If we ever hit that 32 limit on any system, there is probably something seriously wrong with the system configuration anyway.  Whilst I have many addresses, there are only a handful which should be used for rtp in case of addr being "any".

I do not believe that this should present a problem.


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3165 
PS5, Line 3165: 				if (!baseset && ast_sockaddr_is_ipv4(&candidate->address) && !ast_sockaddr_is_any(&candidate->address)) {
> This is srflx - I did my absolute best to clone this functionality "exactly as is currently". […]
Ok, so original line 3097 isn't related to this.

I think you're right, nor would it make sense.  Removed.


https://gerrit.asterisk.org/c/asterisk/+/13362/5/res/res_rtp_asterisk.c@3179 
PS5, Line 3179: 				ast_sockaddr_to_pj_sockaddr(addr, &base);
This is a bug.  This sets base back to ANY for PJSIP, resulting in raddr being :: in SDP.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13362
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I109eaffc3e2b432f00bf958e3caa0f38cacb4edb
Gerrit-Change-Number: 13362
Gerrit-PatchSet: 6
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 17:07:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: Jaco Kroon <jaco at uls.co.za>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200327/05129ded/attachment.html>