[asterisk-dev] [Code Review] ast_uri_encode() behavior change

[David Vossel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/451/
-----------------------------------------------------------

(Updated 2009-12-23 15:28:15.580958)


Review request for Asterisk Developers.


Changes
-------

Update to reflect changes requested by wdoekes in issue #16299.  The major functionality changes here are that 0x25 ('%') and 0x7f are now always encoded regardless if do_special_char is enabled.  

wdoekes wrote:
"My issues.asterisk login does not seem to work on the reviewboard, so I'll post my feedback here:
(1) I explicitly encoded 0x25 and 0x7f. You left the comment about 0x7f but removed the functionality of both. Let me reiteratie: you *must* encode % to %25. And if the 0x7f stays in the comments, it should stay in the code as well.
(2) You check for (*ptr < 32). If you mean to check both <32 and >127, you should explicitly cast it to a signed char. """It is up to the compiler to decide whether "plain" char or default char is signed or unsigned."""
(3) You've added lots of backslashes before the % in the "expected" variable in your test. "\%" is reduced to "%" as it has no special meaning. But for all you know, it might in the future. Don't do that. (I suspect you're using an editor that highlights the %'s as printf and friends conversion specifiers.)
(4) If we're going to poke around in the ast_uri_encode function, I'd say that upper case hex chars are more common and would suggest replacing the "%%02x" with "%%02X". This might trigger breakage for users of bad code however ;-)"


Summary
-------

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.

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


I have viewed several discussions involving this code lately.  My hopes are that this patch will prompt continued discussion and help bring us to a solution we can all agree upon. 

This patch contains logic written by both wdoekes and myself.  Discussions pertaining to this issue may either be discussed here or directed to (issue #16299).


This addresses bug 16299.
    https://issues.asterisk.org/view.php?id=16299


Diffs (updated)
-----

  /trunk/main/utils.c 236309 
  /trunk/include/asterisk/utils.h 236309 
  /trunk/channels/chan_sip.c 236309 

Diff: https://reviewboard.asterisk.org/r/451/diff


Testing
-------

I wrote a Unit Test to verify ast_uri_encode's output.  The results are below.

START  main/utils/ - uri_encode_test 

Input before executing ast_uri_encode:
abcdefghijklmnopurstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 ~`!@#$%^&*()_-+={[}]|\:;"'<,>.?/
Output expected for ast_uri_encode with enabling do_special_char:
abcdefghijklmnopurstuvwxyz%20ABCDEFGHIJKLMNOPQRSTUVWXYZ%201234567890%20~%60!%40%23%24%25%5e%26*()_-%2b%3d%7b%5b%7d%5d%7c%5c%3a%3b%22'%3c%2c%3e.%3f%2f

Output after enabling do_special_char:
abcdefghijklmnopurstuvwxyz%20ABCDEFGHIJKLMNOPQRSTUVWXYZ%201234567890%20~%60!%40%23%24%25%5e%26*()_-%2b%3d%7b%5b%7d%5d%7c%5c%3a%3b%22'%3c%2c%3e.%3f%2f
Decoded string matched original input

Output after disabling do_special_char:
abcdefghijklmnopurstuvwxyz ABCDEFGHIJKLMNOPQRSTUVWXYZ 1234567890 ~`!@#$%^&*()_-+={[}]|\:;"'<,>.?/
Decoded string matched original input

END    main/utils/ - uri_encode_test Time: 0ms Result: PASS 


Thanks,

David