Module: sip-router Branch: master Commit: 3ff9594a30eff2d18e6ea0595c5f82f878eaca1c URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=3ff9594a...
Author: Juha Heinanen jh@tutpro.com Committer: Juha Heinanen jh@tutpro.com Date: Fri Nov 26 10:37:30 2010 +0200
modules_k/nathelper: add_contact_alias adds <>s around contact URI
Make sure that Contact URI is surrounded by <> when adding ;alias parameter. Otherwise, ;alias parameter may be interpreted as header parameter, because the syntax seems to be ambiguous. (cherry picked from commit f5ef15fee0c3bf09adf2506effa0895f72af9034)
---
modules_k/nathelper/nathelper.c | 69 +++++++++++++++++++++++++++++++------- 1 files changed, 56 insertions(+), 13 deletions(-)
diff --git a/modules_k/nathelper/nathelper.c b/modules_k/nathelper/nathelper.c index 51c1086..52c5e5f 100644 --- a/modules_k/nathelper/nathelper.c +++ b/modules_k/nathelper/nathelper.c @@ -786,7 +786,7 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) struct lump *anchor; struct sip_uri uri; struct ip_addr *ip; - char *param, *at, *port, *start; + char *lt, *gt, *param, *at, *port, *start;
/* Do nothing if Contact header does not exist */ if (!msg->contact) { @@ -816,25 +816,63 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) return 2; } - /* Add alias param */ + /* Check if function has been called already */ if ((c->uri.s < msg->buf) || (c->uri.s > (msg->buf + msg->len))) { LM_ERR("you can't call alias_contact twice, check your config!\n"); return -1; } + + /* Check if Contact URI needs to be enclosed in <>s */ + lt = gt = param = NULL; + at = memchr(msg->contact->body.s, '<', msg->contact->body.len); + if (at == NULL) { + lt = (char*)pkg_malloc(1); + if (!lt) { + LM_ERR("no pkg memory left for lt sign\n"); + goto err; + } + *lt = '<'; + anchor = anchor_lump(msg, msg->contact->body.s - msg->buf, 0, 0); + if (anchor == NULL) { + LM_ERR("anchor_lump for beginning of contact body failed\n"); + goto err; + } + if (insert_new_lump_before(anchor, lt, 1, 0) == 0) { + LM_ERR("insert_new_lump_before for "<" failed\n"); + goto err; + } + gt = (char*)pkg_malloc(1); + if (!gt) { + LM_ERR("no pkg memory left for gt sign\n"); + goto err; + } + *gt = '>'; + anchor = anchor_lump(msg, msg->contact->body.s + + msg->contact->body.len - msg->buf, 0, 0); + if (anchor == NULL) { + LM_ERR("anchor_lump for end of contact body failed\n"); + goto err; + } + if (insert_new_lump_before(anchor, gt, 1, 0) == 0) { + LM_ERR("insert_new_lump_before for ">" failed\n"); + goto err; + } + } + + /* Create ;alias param */ param_len = SALIAS_LEN + IP6_MAX_STR_SIZE + 1 /* ~ */ + 5 /* port */ + 1 /* ~ */ + 1 /* proto */; param = (char*)pkg_malloc(param_len); if (!param) { LM_ERR("no pkg memory left for alias param\n"); - return -1; + goto err; } at = param; append_str(at, SALIAS, SALIAS_LEN); ip_len = ip_addr2sbuf(&(msg->rcv.src_ip), at, param_len - SALIAS_LEN); if (ip_len <= 0) { - pkg_free(param); LM_ERR("failed to copy source ip\n"); - return -1; + goto err; } at = at + ip_len; append_chr(at, '~'); @@ -842,9 +880,8 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) append_str(at, port, len); append_chr(at, '~'); if ((msg->rcv.proto < PROTO_UDP) || (msg->rcv.proto > PROTO_SCTP)) { - pkg_free(param); LM_ERR("invalid transport protocol\n"); - return -1; + goto err; } append_chr(at, msg->rcv.proto + '0'); param_len = at - param; @@ -854,18 +891,24 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) } else { start = uri.host.s + uri.host.len; } + + /* Add ;alias param */ anchor = anchor_lump(msg, start - msg->buf, 0, 0); if (anchor == NULL) { - pkg_free(param); - LM_ERR("anchor_lump failed\n"); - return -1; + LM_ERR("anchor_lump for ;alias param failed\n"); + goto err; } if (insert_new_lump_after(anchor, param, param_len, 0) == 0) { - LM_ERR("insert_new_lump_after failed\n"); - pkg_free(param); - return -1; + LM_ERR("insert_new_lump_after for ;alias param failed\n"); + goto err; } return 1; + + err: + if (lt) pkg_free(lt); + if (gt) pkg_free(gt); + if (param) pkg_free(param); + return -1; }
Juha,
this commit seems to suffer from an off-by-one bug, placing the alias parameter outside the just added <>. Attached commit should fix this. If you agree, i'll commit is asap.
Alex.
On Friday 26 November 2010, Juha Heinanen wrote:
Module: sip-router Branch: master Commit: 3ff9594a30eff2d18e6ea0595c5f82f878eaca1c URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=3ff9 594a30eff2d18e6ea0595c5f82f878eaca1c
Author: Juha Heinanen jh@tutpro.com Committer: Juha Heinanen jh@tutpro.com Date: Fri Nov 26 10:37:30 2010 +0200
modules_k/nathelper: add_contact_alias adds <>s around contact URI
Make sure that Contact URI is surrounded by <> when adding ;alias parameter. Otherwise, ;alias parameter may be interpreted as header parameter, because the syntax seems to be ambiguous. (cherry picked from commit f5ef15fee0c3bf09adf2506effa0895f72af9034)
modules_k/nathelper/nathelper.c | 69 +++++++++++++++++++++++++++++++------- 1 files changed, 56 insertions(+), 13 deletions(-)
diff --git a/modules_k/nathelper/nathelper.c b/modules_k/nathelper/nathelper.c index 51c1086..52c5e5f 100644 --- a/modules_k/nathelper/nathelper.c +++ b/modules_k/nathelper/nathelper.c @@ -786,7 +786,7 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) struct lump *anchor; struct sip_uri uri; struct ip_addr *ip;
- char *param, *at, *port, *start;
char *lt, *gt, *param, *at, *port, *start;
/* Do nothing if Contact header does not exist */ if (!msg->contact) {
@@ -816,25 +816,63 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) return 2; }
- /* Add alias param */
- /* Check if function has been called already */ if ((c->uri.s < msg->buf) || (c->uri.s > (msg->buf + msg->len))) { LM_ERR("you can't call alias_contact twice, check your config!\n"); return -1; }
- /* Check if Contact URI needs to be enclosed in <>s */
- lt = gt = param = NULL;
- at = memchr(msg->contact->body.s, '<', msg->contact->body.len);
- if (at == NULL) {
- lt = (char*)pkg_malloc(1);
- if (!lt) {
LM_ERR("no pkg memory left for lt sign\n");
goto err;
- }
- *lt = '<';
- anchor = anchor_lump(msg, msg->contact->body.s - msg->buf, 0, 0);
- if (anchor == NULL) {
LM_ERR("anchor_lump for beginning of contact body failed\n");
goto err;
- }
- if (insert_new_lump_before(anchor, lt, 1, 0) == 0) {
LM_ERR("insert_new_lump_before for \"<\" failed\n");
goto err;
- }
- gt = (char*)pkg_malloc(1);
- if (!gt) {
LM_ERR("no pkg memory left for gt sign\n");
goto err;
- }
- *gt = '>';
- anchor = anchor_lump(msg, msg->contact->body.s +
msg->contact->body.len - msg->buf, 0, 0);
- if (anchor == NULL) {
LM_ERR("anchor_lump for end of contact body failed\n");
goto err;
- }
- if (insert_new_lump_before(anchor, gt, 1, 0) == 0) {
LM_ERR("insert_new_lump_before for \">\" failed\n");
goto err;
- }
- }
- /* Create ;alias param */ param_len = SALIAS_LEN + IP6_MAX_STR_SIZE + 1 /* ~ */ + 5 /* port */
- 1 /* ~ */ + 1 /* proto */; param = (char*)pkg_malloc(param_len); if (!param) { LM_ERR("no pkg memory left for alias param\n");
- return -1;
- goto err; } at = param; append_str(at, SALIAS, SALIAS_LEN); ip_len = ip_addr2sbuf(&(msg->rcv.src_ip), at, param_len -
SALIAS_LEN); if (ip_len <= 0) {
- pkg_free(param); LM_ERR("failed to copy source ip\n");
- return -1;
- goto err; } at = at + ip_len; append_chr(at, '~');
@@ -842,9 +880,8 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) append_str(at, port, len); append_chr(at, '~'); if ((msg->rcv.proto < PROTO_UDP) || (msg->rcv.proto > PROTO_SCTP)) {
- pkg_free(param); LM_ERR("invalid transport protocol\n");
- return -1;
- goto err; } append_chr(at, msg->rcv.proto + '0'); param_len = at - param;
@@ -854,18 +891,24 @@ add_contact_alias_f(struct sip_msg* msg, char* str1, char* str2) } else { start = uri.host.s + uri.host.len; }
- /* Add ;alias param */ anchor = anchor_lump(msg, start - msg->buf, 0, 0); if (anchor == NULL) {
- pkg_free(param);
- LM_ERR("anchor_lump failed\n");
- return -1;
- LM_ERR("anchor_lump for ;alias param failed\n");
- goto err; } if (insert_new_lump_after(anchor, param, param_len, 0) == 0) {
- LM_ERR("insert_new_lump_after failed\n");
- pkg_free(param);
- return -1;
- LM_ERR("insert_new_lump_after for ;alias param failed\n");
- goto err; } return 1;
- err:
- if (lt) pkg_free(lt);
- if (gt) pkg_free(gt);
- if (param) pkg_free(param);
- return -1;
}
Alex Hermann writes:
this commit seems to suffer from an off-by-one bug, placing the alias parameter outside the just added <>. Attached commit should fix this. If you agree, i'll commit is asap.
i have to go out this morning and cannot check your patch until evening. i have been using add_contact_alias in production setups for many months and have not heard that something would be wrong. can you give an example on what it currently does and what you think it should do?
-- juha
Alex Hermann writes:
this commit seems to suffer from an off-by-one bug, placing the alias parameter outside the just added <>. Attached commit should fix this. If you agree, i'll commit is asap.
i was able to make one test and didn't see any problems:
incoming contact:
Contact: sip:jh_test_fi@192.98.102.10:5074;transport=tcp
outgoing after calling add_contact_alias():
Contact: sip:jh_test_fi@192.98.102.10:5074;alias=192.98.102.10~46952~2;transport=tcp
-- juha
On Tuesday 06 September 2011, you wrote:
Alex Hermann writes:
this commit seems to suffer from an off-by-one bug, placing the alias parameter outside the just added <>. Attached commit should fix this. If you agree, i'll commit is asap.
i was able to make one test and didn't see any problems:
incoming contact:
The problem is with contacts that have no <> surrounding them, which is the situation your patch was handling.
Contact: sip:192.168.1.10:5678
became
Contact: sip:192.168.1.10:5678;alias=123.123.123.123~1234~18
alias has become a header parameter instead of of a uri parameter. In my patch i try to fix it and the result is now:
Contact: sip:192.168.1.10:5678;alias=123.123.123.123~1234~18
There seems to be another issue, sorry for the noise.
On Tuesday 06 September 2011, Alex Hermann wrote:
There seems to be another issue, sorry for the noise.
Or not...
When the contact has no <>, these are added by the add_contact_alias() function, but as header parameters instead of uri paramater. My attempted fix doesn't work either.
I think the problem is with the lumps. When the <> are missing, 3 lumps are added to the contact: "<", ">" and ";alias=X" parameter, which seems to mess up the position of the alias parameter. This looks like the infamous problem that one can't manipulate the same part of the message multiple times.
Contact: sip:192.168.1.10:5678
becomes
Contact: sip:192.168.1.10:5678;alias=123.123.123.123~1234~1
When the <> are already present only one lump is added and that does not give any problems.
Am 06.09.2011 12:19, schrieb Alex Hermann:
On Tuesday 06 September 2011, Alex Hermann wrote:
There seems to be another issue, sorry for the noise.
Or not...
When the contact has no <>, these are added by the add_contact_alias() function, but as header parameters instead of uri paramater. My attempted fix doesn't work either.
I think the problem is with the lumps. When the <> are missing, 3 lumps are added to the contact: "<", ">" and ";alias=X" parameter, which seems to mess up the position of the alias parameter. This looks like the infamous problem that one can't manipulate the same part of the message multiple times.
Maybe you can workaround it by adding just 2 lumps? E.g. "<" and ";alias=X>".
regards Klaus
On Tuesday 06 September 2011, Klaus Darilion wrote:
Am 06.09.2011 12:19, schrieb Alex Hermann:
On Tuesday 06 September 2011, Alex Hermann wrote:
There seems to be another issue, sorry for the noise.
Or not...
When the contact has no <>, these are added by the add_contact_alias() function, but as header parameters instead of uri paramater. My attempted fix doesn't work either.
I think the problem is with the lumps. When the <> are missing, 3 lumps are added to the contact: "<", ">" and ";alias=X" parameter, which seems to mess up the position of the alias parameter. This looks like the infamous problem that one can't manipulate the same part of the message multiple times.
Maybe you can workaround it by adding just 2 lumps? E.g. "<" and ";alias=X>".
That might work. Just have to make sure that existing parameters in the contact without <> have to be moved outside the > because they are header parameters (according to rfc3261 section 20, last paragraph).
If i understood correctly the following cases must be supported:
These go wrong at the moment:
Contact: sip:192.168.1.10:5678 should become Contact: sip:192.168.1.10:5678;alias=X
Contact: sip:192.168.1.10:5678;header-param should become Contact: sip:192.168.1.10:5678;alias=Xheader-param
These are already correctly supported:
Contact: sip:192.168.1.10:5678 should become Contact: sip:192.168.1.10:5678;alias=X
Contact: sip:192.168.1.10:5678;uri-param should become Contact: sip:192.168.1.10:5678;alias=X;uri-param
Contact: sip:192.168.1.10:5678;header-param should become Contact: sip:192.168.1.10:5678;alias=Xheader-param
Contact: sip:192.168.1.10:5678;uri-param;header-param should become Contact: sip:192.168.1.10:5678;alias=X;uri-paramheader-param
Pressed send too soon, here is the patch missing from previous mail. Comments welcome.
Alex.
On Tuesday 06 September 2011, Alex Hermann wrote:
On Tuesday 06 September 2011, Klaus Darilion wrote:
Am 06.09.2011 12:19, schrieb Alex Hermann:
On Tuesday 06 September 2011, Alex Hermann wrote:
There seems to be another issue, sorry for the noise.
Or not...
When the contact has no <>, these are added by the add_contact_alias() function, but as header parameters instead of uri paramater. My attempted fix doesn't work either.
I think the problem is with the lumps. When the <> are missing, 3 lumps are added to the contact: "<", ">" and ";alias=X" parameter, which seems to mess up the position of the alias parameter. This looks like the infamous problem that one can't manipulate the same part of the message multiple times.
Maybe you can workaround it by adding just 2 lumps? E.g. "<" and ";alias=X>".
That might work. Just have to make sure that existing parameters in the contact without <> have to be moved outside the > because they are header parameters (according to rfc3261 section 20, last paragraph).
If i understood correctly the following cases must be supported:
These go wrong at the moment:
Contact: sip:192.168.1.10:5678 should become Contact: sip:192.168.1.10:5678;alias=X
Contact: sip:192.168.1.10:5678;header-param should become Contact: sip:192.168.1.10:5678;alias=Xheader-param
These are already correctly supported:
Contact: sip:192.168.1.10:5678 should become Contact: sip:192.168.1.10:5678;alias=X
Contact: sip:192.168.1.10:5678;uri-param should become Contact: sip:192.168.1.10:5678;alias=X;uri-param
Contact: sip:192.168.1.10:5678;header-param should become Contact: sip:192.168.1.10:5678;alias=Xheader-param
Contact: sip:192.168.1.10:5678;uri-param;header-param should become Contact: sip:192.168.1.10:5678;alias=X;uri-paramheader-param
alex,
i reviewed your patch and it looked ok to me except i added one line to initialize lt and param variebles:
/* Check if Contact URI needs to be enclosed in <>s */ lt = param = NULL;
i also tested the case when brackets exist already in incoming request and it worked ok as before. so far i have not had opportunity to test the case where brackets are missing in incoming request.
-- juha
On Wednesday 07 September 2011, Juha Heinanen wrote:
i reviewed your patch and it looked ok to me except i added one line to initialize lt and param variebles:
/* Check if Contact URI needs to be enclosed in <>s */ lt = param = NULL;
Thanks, that disappeared from the original code by accident.
i also tested the case when brackets exist already in incoming request and it worked ok as before. so far i have not had opportunity to test the case where brackets are missing in incoming request.
I'm still wondering whether you have a case where the original code is working for a Contact without brackets.
Alex Hermann writes:
I'm still wondering whether you have a case where the original code is working for a Contact without brackets.
alex, i have not managed to find any ua that would issue contact uri without brackets. if it works in your tests, go ahead and commit your patch.
-- juha
Alex Hermann writes:
These go wrong at the moment:
Contact: sip:192.168.1.10:5678 should become Contact: sip:192.168.1.10:5678;alias=X
Contact: sip:192.168.1.10:5678;header-param should become Contact: sip:192.168.1.10:5678;alias=Xheader-param
alex,
i was away the whole day, because weather was good.
did you test your patch against the above two cases? if so and if it didn't break existing working cases, then please go ahead and apply it.
-- juha