[asterisk-dev] [Code Review] RFC compliant uri and display-name encode/decode
Nick Lewis
Nick.Lewis at atltelecom.com
Fri Jan 22 08:25:11 CST 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/469/#review1390
-----------------------------------------------------------
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3143>
I think parse_uri function should be used here
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3144>
I think it is best to use parse_uri etc in this function. (This may also allow sips: to be supported)
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3145>
Would parse_uri be suitable here?
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3146>
Would parse_uri be suitable here?
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3141>
Consider creating a complete from-header parsing function similar to the read_to_parts function used on the to-header. The read_from_parts function would contain the calls to get_header, get_calleridname, get_in_brackets, parse_uri and SIP_PEDANTIC_DECODE
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3140>
I think parse_uri and SIP_PEDANTIC_DECODE should be used to get exten and our_contact e.g.
if (asz(p->exten)) {
parse_uri(uri2,"sip:,sips:", &t);
SIP_PEDANTIC_DECODE(t);
asfs(p,exten,t);
if (asz(p->our_contact))
build_contact(p);
}}
Also all this request-uri stuff would be better moved up to top with terminate_uri(uri2) rather than mixed in with from-header stuff
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3142>
This function makes mistakes with escaped uris and token based display names. It needs to use the parse_uri, get_calleridname and SIP_PEDANTIC_DECODE functions.
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3147>
It needs get_calleridname here to correctly read token based names
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3148>
I think parse_uri function should be used here to get transport, contact_number etc. Also contact_number may need SIP_PEDANTIC_DECODE
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3150>
Should these two uris be deconstructed with parse_uri and the parts be uri decoded before making a comparison
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/469/#comment3149>
Should this be parse_uri? Also should it be uri decoded and uri recoded before adding to transfer header so that any illegal chars are screened out
- Nick
On 2010-01-21 18:36:47, David Vossel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/469/
> -----------------------------------------------------------
>
> (Updated 2010-01-21 18:36:47)
>
>
> Review request for Asterisk Developers.
>
>
> Summary
> -------
>
> Parts of this patch were posted in separate reviews a few weeks ago. During the discussion of those patches I took down the reviews as I felt the code was not complete. This review is a combination of the two uri encode/decode patches, a complete rewrite of the get_calleridname() function, and the addition of two new unit tests. These changes are in response to (issue #16299) and are a compilation of code written by both wdoekes and myself.
>
> ------Changes------
>
> 1. URI Encoding
>
> This patch changes ast_uri_encode()'s behavior when doreserved is enabled. Previously when doreserved was enabled only a small set of reserved characters were encoded. This set was comprised primarily of the reserved characters defined in RFC3261 section 25.1, but contained other characters as well. Rather than only escaping the reserved set, doreserved now escapes all characters not within the unreserved set as defined by RFC 3261 and RFC 2396. Also, the 'doreserved' variable has been renamed to 'do_special_char' in attempts to avoid confusion.
>
> When doreserve is not enabled, the previous logic of only encoding the characters <= 0X1F and > 0X7f remains, except for the '%' character, which must always be encoded as it signifies a HEX escaped character during the decode process.
>
> In RFC 3261 and RFC 2396 the unreserved character set is defined by all alphanumeric characters and a small number of characters defined in the mark set.
> mark = "-" / "_" / "." / "!" / "~" / "*" / "'" / "(" / ")"
> unreserved = alphanum / mark
>
> 2. URI Decoding: Break up URI before decode.
>
> In chan_sip.c ast_uri_decode is called on the entire URI instead of it's individual parts after it is parsed. This is not good as ast_uri_decode can introduce special characters back into the URI which can mess up parsing. This patch resolves this by not decoding a URI until parsing is completely done. There are many instances where we check to see if pedantic checking is enabled before we decode a URI. In these cases a new macro, SIP_PEDANTIC_DECODE, is used on the individual parsed segments of the URI rather than constantly putting if (pedantic) { decode() } checks everywhere in the code. In the areas where ast_uri_decode is not dependent upon pedantic checking this macro is not used, but decoding is still moved to each individual part of the URI. The only behavior that should change from this patch is the time at which decoding occurs.
>
> Since I had to look over every place URI parsing occurs to create this patch, I found several places where we use duplicate code for parsing. To consolidate the code, those areas have updated to use the parse_uri() function where possible.
>
> 3. SIP display-name decoding according to RFC3261 section 25.
>
> To properly decode the display-name portion of a FROM header, chan_sip's get_calleridname() function required a complete re-write. More information about this change can be found in the comments at the beginning of this function.
>
> 4. Unit Tests.
>
> Unit tests for ast_uri_encode, ast_uri_decode, and get_calleridname() have been written. This involved the addition of the test_utils.c file for testing the utils api.
>
>
> Diffs
> -----
>
> /trunk/include/asterisk/utils.h 242134
> /trunk/main/test.c 242134
> /trunk/main/test_utils.c PRE-CREATION
> /trunk/channels/chan_sip.c 242134
> /trunk/main/utils.c 242134
>
> Diff: https://reviewboard.asterisk.org/r/469/diff
>
>
> Testing
> -------
>
> - new unit tests pass
> - verified SIP registrations, calls, and transfers work correctly within my test environment
>
>
> Thanks,
>
> David
>
>
More information about the asterisk-dev
mailing list