[Asterisk-code-review] chan sip: Peers with distinct source ports don't match, rega... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/7430 )

Change subject: chan_sip: Peers with distinct source ports don't match, regardless of transport.
......................................................................


Patch Set 1:

(1 comment)

https://gerrit.asterisk.org/#/c/7430/1/channels/chan_sip.c
File channels/chan_sip.c:

https://gerrit.asterisk.org/#/c/7430/1/channels/chan_sip.c@34132
PS1, Line 34132: 	if (((peer->transports & peer2->transports) &
> For TCP/TLS, the same connection must be re-used. I see no benefit
 > in insecure=port for TCP/TLS.
 > 
 > However, David wrote something like "[For TCP,] the received
 > packet's destination port will not match the one the peer table is
 > built with." No idea what he was about. It is a pity that we cannot
 > ask David anymore.
 > 
 > The code before this change looks like insecure=port is enabled for
 > TCP/TLS always, regardless the settings in sip.conf. However, that
 > might have been not a TCP/TLS issue in general but a specific issue
 > at that time within Asterisk.
 > 
 > By the way, did you see my previous reply in master? When peer2
 > has, and peer1 does not have insecure=port, zero is returned.
 > Instead, it should go through the port test and return zero only
 > when the ports do not match. Or? With other words, only when UDP
 > and both peers have insecure=port, then a match should be returned.
 > Otherwise the ports should be checked.

Ah, I didn't see the comment, sorry.  I think part of the problem is that peer_ipcmp_cb_full gets used in multiple ways.  It's used as the compare function for the peers_by_ip container when you call ao2_find on it.  In that case, there are 2 peers to compare.  It's also used by sip_find_peer when you only know the ip address or name.  In that case, peer2 is just a temporary peer created for the search.  I'm concerned that changing the default behavior of peer_ipcmp_cb_full would cause other problems because of teh multiple ways it's used.  Is there not something in the allowguest logic that could be tweaked instead?



-- 
To view, visit https://gerrit.asterisk.org/7430
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Id190428bf1d931f2dbfd4b293f53ff8f20d98efa
Gerrit-Change-Number: 7430
Gerrit-PatchSet: 1
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Wed, 06 Dec 2017 14:15:02 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171206/e9dce0f9/attachment-0001.html>