evaluate.c & strings with "\" in them
If I do this: echo User-Name='foo\bar' | radclient ... ...and try to match this in unlang: if (User-Name =~ /^(.+)\\\\(.+)$/) { ...which corresponds to a regex: ^(.+)\\(.+)$ ...it doesn't work. I think this is because evaluate.c:radius_do_cmp calls vp_print_value(..., 0) for T_OP_REG_*. That final arugment "0" means "escape the string" so the value compared against gets turned into: foo\\bar ...which needs a regex: ^(.+)\\\\(.+)$ ...which needs a condition: if (User-Name =~ /^(.+)\\\\\\\\(.+)$/) { This seems a bit crazy, but is also inconsistent with the "users"-file stuff in valuepair.c - this uses radius_xlat (or vp_prints_value directly, now) which finishes up in "vp_prints_value(..., -1) which means "value verbatim", so doesn't need quad-escaping. The fix is pretty simple: https://github.com/philmayers/freeradius-server/commit/2695a37464560dbd42b10... ...but is sadly not a backwards-compatible change. The other thing is, do we actually need to handle "\" inside // at all, with the exception of escaping the regex delimiter? Or could we just pass "\" straight through to to the regex engine? To give you an idea of what I mean (with a bit of extra flags magic too): https://github.com/philmayers/freeradius-server/commit/7c322b36d603d0189c729... if (User-Name =~ !^(.+)\\(.+)$!ims8) { ... } The choice of "!" is arbitrary, could be anything, indeed, could be selectable sed-style by inspecting the 1st character: if (User-Name =~ @regex@flags) ...but letting it be something other than "/" helps avoid leaning toothpick syndrome. Thoughts welcome. Just idly playing here really.
On Wed, Oct 10, 2012 at 06:51:17PM +0100, Phil Mayers wrote:
The other thing is, do we actually need to handle "\" inside // at all, with the exception of escaping the regex delimiter?
I think even then, you would also need to treat backslash backslash as a single backslash. Reason: imagine you wanted to match a a backslash followed by a forward slash. The RE couldn't be \/ because that would be just the escaped forward slash, so it would have to be \\/. Hence every instance of a single backslash needs to be escaped. But backslash followed by any other character could be passed straight through, e.g. \n is "\" and "n". The other question is, do we want escape sequences like \n for newline and \xHH for an arbitrary byte?
On 11/10/12 11:29, Brian Candler wrote:
On Wed, Oct 10, 2012 at 06:51:17PM +0100, Phil Mayers wrote:
The other thing is, do we actually need to handle "\" inside // at all, with the exception of escaping the regex delimiter?
I think even then, you would also need to treat backslash backslash as a single backslash.
Sure.
Reason: imagine you wanted to match a a backslash followed by a forward slash. The RE couldn't be \/ because that would be just the escaped forward slash, so it would have to be \\/. Hence every instance of a single backslash needs to be escaped.
But backslash followed by any other character could be passed straight through, e.g. \n is "\" and "n".
The other question is, do we want escape sequences like \n for newline and \xHH for an arbitrary byte?
Isn't that the same question as the previous one?
Phil Mayers wrote:
This seems a bit crazy,
Yes.
but is also inconsistent with the "users"-file stuff in valuepair.c - this uses radius_xlat (or vp_prints_value directly, now) which finishes up in "vp_prints_value(..., -1) which means "value verbatim", so doesn't need quad-escaping.
Yes.
The fix is pretty simple:
https://github.com/philmayers/freeradius-server/commit/2695a37464560dbd42b10...
Good.
...but is sadly not a backwards-compatible change.
That's fine for 3.0.
The other thing is, do we actually need to handle "\" inside // at all, with the exception of escaping the regex delimiter? Or could we just pass "\" straight through to to the regex engine? To give you an idea of what I mean (with a bit of extra flags magic too):
https://github.com/philmayers/freeradius-server/commit/7c322b36d603d0189c729...
Just do: if (*p != '/') return getregex2(...); And have it do basic checks for sanity.
The choice of "!" is arbitrary, could be anything, indeed, could be selectable sed-style by inspecting the 1st character:
Yeah.
...but letting it be something other than "/" helps avoid leaning toothpick syndrome.
That's nice.
Thoughts welcome. Just idly playing here really.
I'ts OK. I'll do the pull. :) Alan DeKok.
participants (3)
-
Alan DeKok -
Brian Candler -
Phil Mayers