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/08de5a57f202aebabdbd0...
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?