Updated with helper methods and better SIP URI parsing

[kylekurz]

There was some duplicated code that I was adjusting and you weren't handling URIs that contained a user (sip:user@host), so I pulled the address start and finish detection out into separate functions that could be called from both pj_nat64_get_hostname_from_proxy_string and pj_nat64_resolve_and_replace_hostname_with_ip_if_possible. I also changed the end lookup to not require an explicit port. It still fails to find the end if you don't have either a port OR a transport listed explicitly. I'll be working on that next probably.

[johanlantz]

Hi Kyle

I am currently abroad on vacation. I will have a look at the PR during the
second half of August when I am back in the office.

Best Regard
Johan

On Wednesday, 20 July 2016, Kyle Kurz [email protected] wrote:

There was some duplicated code that I was adjusting and you weren't
handling URIs that contained a user (sip:user@host), so I pulled the
address start and finish detection out into separate functions that could
be called from both pj_nat64_get_hostname_from_proxy_string and
pj_nat64_resolve_and_replace_hostname_with_ip_if_possible. I also changed
the end lookup to not require an explicit port. It still fails to find the
end if you don't have either a port OR a transport listed explicitly. I'll

be working on that next probably.

You can view, comment on, or merge this pull request online at:

#2
Commit Summary

• Updated with helper methods and better SIP URI parsing

File Changes

• M pj-nat64.c
  https://github.com/johanlantz/pj-nat64/pull/2/files#diff-0 (41)

Patch Links:

• https://github.com/johanlantz/pj-nat64/pull/2.patch
• https://github.com/johanlantz/pj-nat64/pull/2.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2, or mute the thread
https://github.com/notifications/unsubscribe-auth/AHHbF1Q10qi8e1RIZfj8ISuYbwgtXJqEks5qXlGWgaJpZM4JQ-yw
.

[kylekurz]

No worries. I have my own fork that I'm using for now. Just figured I'd share my adjustments.

[johanlantz]

That is perfect. I am really happy someone else is looking at the code.

Johan

On Thursday, 21 July 2016, Kyle Kurz [email protected] wrote:

No worries. I have my own fork that I'm using for now. Just figured I'd
share my adjustments.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHHbF6AfQE4xIc7u6LCJCbdN0i8sLt-iks5qX-Y-gaJpZM4JQ-yw
.

[johanlantz]

Sorry for the delay on this one. I am leaning towards not merging it for the following reasons:

• The find_addr_start. I have never encountered a proxy address with a @ user part. Not sure that is legal. For the find_addr_end sure its used twice so it is a little nicer to wrap it in a function. Lets see.

• For the set_resolved_host. This will not work in most systems since when you use rtp proxies which is the most common scenario in a larger system, the IP4 address in the sdp will not be the same as for the registrar. I will try to see if there is another way to handle android synthesizing.

Thanks for the submission nevertheless, I will go through the details and see if there is something I can cherry pick.

[kylekurz]

@johanlantz You're welcome to pull in any/none of my changes. We're using my fork in our production stuff. This latest set of changes comes off the fact that when you replace IPv4 -> IPv6, you're doing a pj_memcpy to a buffer that may not be as long as the data you want to print, eventually smashing the stack. I went ahead and modified the IPv6 -> IPv4 direction too, as we were having trouble with the exceptions as a way to know when the scanner found the requested item.

For your comments on find_addr_start, it's not just proxies. You need to replace IPv6 on any SIP URI (if not going through a proxy, at least), and certainly a registration attempt will include a user field.

I've not been able to find a way to synthesize an address on Android yet. This works for our environment and your module will just fail to replace on Android right now, so adding the option to set it seemed like the right way to handle it. Like I said, feel free to not merge in anything you want, I'm maintaining what works for us in my fork.

[johanlantz]

Thanks for the feedback, I will keep that in mind. This is the first time I revise the code in almost a year so I am a bit rusty.

Android is complicated. I thought it would have improved over time but it seems there is no clear solution. It is good that your workaround is functional and for a system which uses the same IP for media as signalling I think it is a correct solution.

For systems having more than one media endpoint, the only obvious solution I see is for that system to inject a fqdm in the sdp and assign a domain name to each media endpoint. In theory that should work as well.

Leaving this thread open for others to reference for the moment since everyone seems to have different challenges depending on their system.