Please comment: fix updating dialog nexthop #183
Conversation
|
Sample of the issue manifesting and logged: |
AlexAT
changed the title
|
@dmitry-sinina Sorry, I'm just a very minor contributor actually :) Not part of the base team. |
|
I'm closing this PR then, will try building with the #181 reverted and post there. |
|
Actually... I'm reopening this as actually the dialog update code is still broken, be #181 reverted or not. |
|
In order to avoid storing both, is it not possible to convert local/remote_ip to local/remote_ip_sip when the latter is needed? |
|
No, as msg->local_ip / msg->remote_ip connection information related is lost and not stored in AmSipMsg. As said, we can store addr->ss_family as additional field instead and replicate am_inet_ntop_sip() encapsulation where it's needed, but that will semantically go against am_inet_ntop_sip() / get_addr_str_sip() abstraction, meaning we make part of their behavior cloned and fixed to the place it's cloned to. |
|
If such loss of abstraction / introduction of hardcoded dependency is okay for the place, I'll rewrite the fix storing and reusing addr->ss_family instead though |
|
Another option is naming remote_ip_sip (and local_ip_sip) something like remote_ip_sip_opt, and storing it ONLY if it differs from remote_ip_sip, storing empty string otherwise. But this looks too hacky, and requires a comparison. |
|
AlexAT writes:
No, as msg->local_ip / msg->remote_ip connection information related
is lost and not stored in AmSipMsg.
I meant if is is possible to convert local_ip to local_ip_sip and the
same for remote_ip when sip version is needed?
|
|
Technically yes. It's simply escaping with '[' and ']'. |
|
AlexAT writes:
If such loss of abstraction / introduction of hardcoded dependency is
okay for the place, I'll rewrite the fix using addr->ss_family instead
though
Is this only a problem with IPv6? If so, then whatever the fix is it
should have minimal effect on sems users that do not need IPv6
functionality.
|
|
I suggest storing local_ip_is_ipv6 and remote_ip_is_ipv6 booleans then. |
|
I will rewrite this PR adjustments using such flags in a little while. |
|
A trivial rewrite, please review. |
|
Latest version looks fine. It clearly is backwards compatible when addresses are IPv4. Do you still want to make changes or should I merge the request? |
|
Tested in practice and found working fine, can be merged. |
This one PR is request for comments because the way it proposes to fix the described issue maybe is a bit overkill.
Issue description:
When i.e. first reply in the sequence with pre-defined next hop (like with SBC module) is received, SEMS updates dialog next hop to be used. If some 181/182 reply needing PRACK but not setting remote_tag (so not flagging connection being beyond first request) is received, PRACK is being sent with the pre-defined (and thus updated) next hop.
This should work normally, but there is one exception: the next hop is updated with unframed remote IP. This works for IPv4 but totally fails for IPv6 because IPv6 address needs to be [] framed in SIP. Thus, SEMS tries to resolve first IPv6 address portion before first : as domain then, and the port part is decoded totally wrong.
Of course it works wrong on any other similar dialog update (both request/reply based), just is not noticeable under most conditions except specific ones where this dialog next hop update is crucial.
Amendment:
The PR amends that by also storing SIP variant of remote IP (and local IP for complementary reasons) in the AmSipMsg object, and utilizes SIP variant to update dialogs next hop.
Doubts:
This fix is a bit overkill because it increases memory consumption for storing remote/local IP twice and does double work on each address to string conversion.
An alternative way of doing this may be storing some 'remote_ip_is_ipv6' flag and reusing it in the places remote_ip needs to be injected in SIP escaped way. But this would not be semantically correct as get_addr_str_sip() may potentially begin to do something beyond just escaping IPv6 addresses with [], and so the flag will not be enough.