[sr-dev] [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[sr-dev] [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric

Type Of Change

  • 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

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"mtree: fix handling of character values \u003e= 0x80 (#1343)"}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|

Re: [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric

Closed #1343.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Closed #1343."}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343#event-1364191472"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|

Re: [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric
In reply to this post by mihovilkolaric

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@miconda in #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."}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343#issuecomment-347961781"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|

Re: [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric
In reply to this post by mihovilkolaric

@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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@fabled in #1343: @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 \u003e=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."}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343#issuecomment-347977164"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|

Re: [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric
In reply to this post by mihovilkolaric

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@miconda in #1343: 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."}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343#issuecomment-347982673"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|

Re: [kamailio/kamailio] mtree: fix handling of character values >= 0x80 (#1343)

mihovilkolaric
In reply to this post by mihovilkolaric

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@fabled in #1343: 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."}],"action":{"name":"View Pull Request","url":"https://github.com/kamailio/kamailio/pull/1343#issuecomment-347983929"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev