#### Type Of Change - [X] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [X ] PR should be backported to stable branches - [ ] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
This fixes mtree to be able to contain strings with character values larger than 0x80. Previously this would not work because the str.s[i] is 'char' and when cast to 'unsigned int' it would be first sign extended for maximum range (on systems where 'char' defaults to signed). E.g. on x86_64 system: (unsigned int)(char)0x80 = 4294967168
The >=MT_CHAR_TABLE_SIZE test would filter any string containing these characters out as invalid.
Code is fixed to cast _mt_char_table index to 'unsigned char' always so index is within range always. The new redundant checks against MT_CHAR_TABLE_SIZE are removed, and the result is stored to local variable (and used from it) to improve code readability.
The error message in mt_add_to_tree() is harmonized with the other messages to show the full string with a problem.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1343
-- Commit Summary --
* mtree: fix handling of character values >= 0x80
-- File Changes --
M src/modules/mtree/mtree.c (132)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1343.patch https://github.com/kamailio/kamailio/pull/1343.diff
Closed #1343.
Thanks! I applied the patch manually to keep printing the character and its hexa code in the error log message but also to have the commit message reflect that it is not a fix to an issue, the existing IF conditions were protecting against the case you described as a problem.
@miconda The patch really fixes the bug described in the commit message. The if() condition protects against crash. But the side-effect was that all characters >=0x80 that got sign extended were rejected by the guard. In real life this caused the problem not to be able match utf-8 strings with non-ascii characters.
Hmm, I see now -- initially after the IRC chat I was left with the impression it was about array out of bounds, then I haven't thought of non-ascii char, because theoretically it is what the cfg interpreter was designed to support when reading from kamailio.cfg file, and the respective table is mapping the char list content to a 256 array for fast matching.
Yes, I originally complained about crash due to experiencing the bug on old Kamailio without the fix that added the if() guards. However, that fix introduced this side-effect. While it's not critical and probably not affecting many, it's still user visible issue that gets fixed.
-
Daniel-Constantin Mierla -
Timo Teräs