SQL escaping

[Phil Mayers]

On 25/09/12 20:16, Alan DeKok wrote:
>
>    The only thing for me is that the escaping has to be done recursively.
>     e.g. SQL asking for an expansion of "foo %{Bar}" means that the "foo"
> portion should be copied as-is.  But the %{Bar} portion is untrusted,
> and MUST be escaped.

It should do that - I tested it locally with a pretty big variety of 
xlats on the SQL and LDAP modules, including nested xlats.

However - TBH I find the code in decode_attribute() and 
rad_copy_variable() a bit hairy. It wasn't obvious to me for example 
that this:

%{some %{var}:-other %{text}}

...isn't legal syntax. I can think of circumstances that might be useful.

So, that being the case, if someone could compile and test the xlat 
stuff to destruction to check I haven't missed anything, that would be a 
good thing (tm).

If the general approach is ok, I'll apply this patch to our testing 
radius server, and might work up a branch with SQL-native escaping to 
really give it a torture test.

>
>    From looking at the code, it looks pretty good.  My only $0.02 is:
>
> https://github.com/philmayers/freeradius-server/commit/08de5a57f202aebabdbd0a50a99574aca79a130d
>
>    It does a malloc.  I'd just declare (or re-use) a buffer on the stack.
>   malloc() is slow and painful for temporary storage.

The main reason I used a malloc there was that I didn't want to truncate 
any output, since all the code upward of the stack uses pointer/length 
arguments. And variable-length arrays:

  char buf[freespace];

...are a GCC-ism. But I take your point, and I guess a static buffer is 
OK - we're reading from a "char buf[8192]" at that point anyway.

Do you want me to re-do the patch stream or just modify the commit 
if/when it gets merged?