[asterisk-dev] Filter() function broken ?

[Tilghman Lesher]

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.

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

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

-- 
Tilghman Lesher
Digium, Inc. | Senior Software Developer
twitter: Corydon76 | IRC: Corydon76-dig (Freenode)
Check us out at: www.digium.com & www.asterisk.org