[asterisk-dev] Filter() function broken ?

[Mark Michelson]

Tilghman Lesher wrote:
> On Wednesday 10 February 2010 00:05:23 Mark Michelson wrote:
>> Pavel Troller wrote:
>>> Hi!
>>>   I was experimenting with the Filter() function to filter some
>>> characters from the dial string and it behaves strangely. I've created
>>> the following filter as a part of my Dial() command:
>>>
>>> ${FILTER("A-Za-z0-9_+\-",${fnum})}
>>>
>>> One of characters I wanted to pass is the "-" one, so I prepended it with
>>> a backlash, as stated in the Filter() function help text.
>>> However, it looks that I triggered a bug. I was surprised, that by
>>> supplying the dialstring "0042133333&DAHDI/g1/2212", the slashes were not
>>> filtered out, even this character was not between the allowed ones.
>>> To find a problem, I've made a debug, and the surprise is here:
>>>
>>> [Feb  9 19:50:11] DEBUG[21285] func_strings.c: c1=65, c2=90
>>> [Feb  9 19:50:11] DEBUG[21285] func_strings.c: c1=97, c2=122
>>> [Feb  9 19:50:11] DEBUG[21285] func_strings.c: c1=48, c2=57
>>> [Feb  9 19:50:11] DEBUG[21285] func_strings.c: c1=43, c2=-1
>>>
>>> [Feb  9 19:50:11] DEBUG[21285] func_strings.c: Allowed:
>>> ABCDEFGHIJKLMNOPQRSTUVWXYZZabcdefghijklmnopqrstuvwxyzz01234567899_+,-./01
>>> 23456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{
>>> |}~^?~@~A~B~C~D~E~F~G~H~I~J~K~L~M~N~O~P~Q~R~S~T~U~V~W~X~Y~Z~[~\~]~^~_| Ą
>>> [Feb  9 19:50:11] DEBUG[21285] pbx.c: Function result is
>>> '0042133333DAHDI/g1/2212'
>>>
>>> It looks that the backslash before the - is not properly recognized, and
>>> that a missing upper limit after the - causes that the filter is filled
>>> up with all possible characters following, in our case, "+". Is it a bug,
>>> or I just improperly understood the help text ? BTW, I was testing it
>>> with quotes (as shown here) as well as without them and there is no
>>> difference.
>>>
>>> With regards, Pavel
>>>
>>> P.S. Asterisk-1.6.1.14.
>> I looked into this and found the problem. I added a debug line to the
>> filter() function in func_strings.c to see what the exact value of
>> args.allowed is at the beginning of the function. When I ran your
>> example, this is what it showed:
>>
>> [Feb  9 23:41:50] NOTICE[26446]: func_strings.c:445 filter: args.allowed
>> is A-Za-z0-9_+-
>>
>> Notice how there is no '\' between the '+' and '-'. The backslash is
>> being removed from the string during the AST_STANDARD_APP_ARGS macro.
>> The function should probably instead use AST_STANDARD_RAW_ARGS so that
>> the backslash will not be removed. The downside to that change is that
>> you would also have to remove the quotation marks from the first
>> argument to FILTER, because otherwise the " character would be allowed.
>> A temporary dialplan fix is to add a second backslash to the string,
>> resulting in "A-Za-z0-9_+\\-".
>>
>> I find the handling of the '-' character in the filter() function to be
>> haphazard in general. There are two cases I can think of that are not
>> handled especially gracefully. One is if the character after the '-'
>> comes before the character before the '-' on the ASCII chart. The other
>> case is where the '-' is unescaped and ends the string. Both result in a
>> much larger range of characters allowed than would be expected. I would
>> expect the behavior to be as follows:
> 
> Actually, those behaviors are intentional and could be expected based on
> other parts of Asterisk that allow range wrapping.

This is something I find a bit hard to swallow in this particular case. I know 
that range wrapping is allowed for such constructs as expressing a time which 
starts in the evening and extends into the morning or a day range which starts 
toward the end of the week and wraps back to the beginning. However, when 
talking about a range of characters that includes the entire ASCII character 
set, including non-printable characters, I think that it's a bit ridiculous to 
think that a person really intended to include such a range.

Perhaps my interpretations of how things should behave are not on par with what 
others think, but I at least think that everyone can agree that a warning 
message should be issued so that if something does behave unexpectedly, the 
dialplan writer can see the big red message that points out what he has likely 
done incorrectly.

> 
>> If someone does something like "b-a" I would expect a warning to be
>> emitted and for the resulting filter to allow the same thing as if the
>> dialplan writer had written "a-b"
>> If a '-' is not followed by any character, then a warning will be
>> emitted and the range of characters up for filtering would be discarded
>> from the allowed string altogether.
> 
> One should always test their dialplan to ensure that their intent matches what
> they wrote.

Oh most definitely.

> 
>> Also, I think it's a bit ridiculous to ever allow non-printable
>> characters through the filter() function.
>>
>> I think func_strings.c is an excellent target for writing some unit
>> tests for anyone looking to add some more to Asterisk, by the way.
> 
> Unit tests were written yesterday and committed today (after testing them).
>

Thanks very much for that, Tilghman.

Mark!