Problem with setting substring match variables (%{1} etc.)
Hi, there seems to be a subtle problem with setting the substring match variables like %{1} etc. when an optionally matching subexpression does not match, but the expression as a whole does. Example: The expression ^(a|b)?(.)$ will match for example 'ax', 'bx', and 'x'. The first two cases will work like expected setting %{1} = 'a', %{2} = 'x' and so on. In the last case, the first substring is actually empty with rxmatch[1].rm_so == -1, but the second substring contains 'x' (rxmatch[2].rm_so == 0, rxmatch[2].rm_eo == 1). The code setting the substring match variables stops on the first rxmatch[].rm_so == -1 unless the variable has an old value(?), so %{2} might be never set in the last example. Is this expected behaviour? If not, how can this be fixed? Just leave out the 'break' in the for loop? (Code from valuepair.c)
/* * Add %{0}, %{1}, etc. */ for (i = 0; i <= REQUEST_MAX_REGEX; i++) { char *p; char buffer[sizeof(check->vp_strvalue)];
/* * Didn't match: delete old * match, if it existed. */ if ((compare != 0) || (rxmatch[i].rm_so == -1)) { p = request_data_get(request, request, REQUEST_DATA_REGEX | i); if (p) { free(p); continue; }
/* * No previous match * to delete, stop. */ break; } ...; }
Enrik
Enrik Berkhan wrote:
Hi,
there seems to be a subtle problem with setting the substring match variables like %{1} etc. when an optionally matching subexpression does not match, but the expression as a whole does.
Example:
The expression ^(a|b)?(.)$ will match for example 'ax', 'bx', and 'x'. The first two cases will work like expected setting %{1} = 'a', %{2} = 'x' and so on. In the last case, the first substring is actually empty with rxmatch[1].rm_so == -1, but the second substring contains 'x' (rxmatch[2].rm_so == 0, rxmatch[2].rm_eo == 1).
The code setting the substring match variables stops on the first rxmatch[].rm_so == -1 unless the variable has an old value(?), so %{2} might be never set in the last example.
Is this expected behaviour? If not, how can this be fixed? Just leave out the 'break' in the for loop? (Code from valuepair.c)
/* * Add %{0}, %{1}, etc. */ for (i = 0; i <= REQUEST_MAX_REGEX; i++) { char *p; char buffer[sizeof(check->vp_strvalue)];
/* * Didn't match: delete old * match, if it existed. */ if ((compare != 0) || (rxmatch[i].rm_so == -1)) { p = request_data_get(request, request, REQUEST_DATA_REGEX | i); if (p) { free(p); continue; }
/* * No previous match * to delete, stop. */ break; } ...; }
Enrik - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
Yep I noticed this too, though I worked around it at the time. Also gets very uggly when you multiple instances of a subcapture group e.g : ^(a|b){3}(.)$ So you have to write them out in full : ^(a|b)(a|b)(a|b)(.)^ Particularly annoying if your using regular expressions to break apart mac addresses and ip addresses .
Enrik Berkhan wrote:
The expression ^(a|b)?(.)$ will match for example 'ax', 'bx', and 'x'. The first two cases will work like expected setting %{1} = 'a', %{2} = 'x' and so on. In the last case, the first substring is actually empty with rxmatch[1].rm_so == -1, but the second substring contains 'x' (rxmatch[2].rm_so == 0, rxmatch[2].rm_eo == 1).
I wasn't aware of that feature of the regex API.
The code setting the substring match variables stops on the first rxmatch[].rm_so == -1 unless the variable has an old value(?), so %{2} might be never set in the last example.
Yup. It looks like the code should just check *all* of the rxmatch[] array.
Is this expected behaviour? If not, how can this be fixed? Just leave out the 'break' in the for loop? (Code from valuepair.c)
Likely, yes. If that works, please say so, and I'll commit a patch. I'm a little overloaded right now, and can't look at it myself. Alan DeKok.
Hi, Alan DeKok wrote:
Enrik Berkhan wrote:
Is this expected behaviour? If not, how can this be fixed? Just leave out the 'break' in the for loop? (Code from valuepair.c)
Likely, yes.
If that works, please say so, and I'll commit a patch. I'm a little overloaded right now, and can't look at it myself.
For testing, I've changed the inner 'no match or no subexpression match' condition of the four for-loops that are currently used to update %{0}... in the following way: old:
if (r) { free(r); continue; } break;
new:
if (r) { free(r); } continue;
This seems to work, variables are unset if they have an old value and no new value (no match at all or empty) or they are set/updated if they have a new value (match implied). I've only tested it using unlang:
if ("%{User-Name}" =~ /^(a)(.)(.)/) { update reply { Reply-Message := "first match: 0:%{0} 1:%{1} 2:%{2} 3:%{3} " } } else { update reply { Reply-Message := "no match on first regex" } } if ("%{User-Name}" =~ /^(a|b)?(.)/) { update reply { # += segfaults?! #Reply-Message += "second match: 0:%{0} 1:%{1} 2:%{2} 3:%{3} " Reply-Message := "%{reply:Reply-Message}; second match: 0:%{0} 1:%{1} 2:%{2} 3:%{3} " } } else { update reply { #Reply-Message += "no match on second regex" Reply-Message := "%{reply:Reply-Message}; no match on second regex" } }
The for-loops to update %{0}... are currently explicitly found in - src/main/evaluate.c - src/main/valuepair.c - src/modules/rlm_attr_rewrite/rlm_attr_rewrite.c (slight variation: doesn't touch the variables on no match) - src/modules/rlm_policy/evaluate.c Shouldn't this be moved to a single place like a function in src/main/util.c or main/xlat.c called xlat_update_regex or so? Further, I've noticed that in all places where regex-no-match operators are implemented but in src/modules/rlm_policy/evalute.c, the subexpressions are evaluated but never used later. May this could be fixed, too. Of course I could provide patches for all of this or parts thereof if it's the right direction. Just tell me what you'd prefer. Now I'm gonna try to find out why the += segfaults in the above unlang example ... Enrik
Enrik Berkhan wrote:
Now I'm gonna try to find out why the += segfaults in the above unlang example ...
I think it's a simple typo: Index: src/main/evaluate.c =================================================================== RCS file: /source/radiusd/src/main/evaluate.c,v retrieving revision 1.24 diff -u -r1.24 evaluate.c --- src/main/evaluate.c 1 Aug 2007 15:45:34 -0000 1.24 +++ src/main/evaluate.c 9 Aug 2007 08:25:36 -0000 @@ -912,7 +907,7 @@ * file and debug output, where we don't want to * see the operators. */ - to_list[j]->operator = T_OP_EQ; + to_list[i]->operator = T_OP_EQ; *last = to_list[i]; last = &(*last)->next; Enrik
Enrik Berkhan wrote:
For testing, I've changed the inner 'no match or no subexpression match' condition of the four for-loops that are currently used to update %{0}... in the following way:
OK.
This seems to work, variables are unset if they have an old value and no new value (no match at all or empty) or they are set/updated if they have a new value (match implied).
Hmm... on a regex match, all previous match information should be deleted. This prevents old matches from polluting the variables for the current match.
I've only tested it using unlang:
if ("%{User-Name}" =~ /^(a)(.)(.)/) { update reply { Reply-Message := "first match: 0:%{0} 1:%{1} 2:%{2} 3:%{3} "
Isn't that just incredibly easy to use? :)
The for-loops to update %{0}... are currently explicitly found in
- src/main/evaluate.c - src/main/valuepair.c - src/modules/rlm_attr_rewrite/rlm_attr_rewrite.c (slight variation: doesn't touch the variables on no match) - src/modules/rlm_policy/evaluate.c
Shouldn't this be moved to a single place like a function in src/main/util.c or main/xlat.c called xlat_update_regex or so?
Yes, probably.
Further, I've noticed that in all places where regex-no-match operators are implemented but in src/modules/rlm_policy/evalute.c, the subexpressions are evaluated but never used later. May this could be fixed, too.
I seem to recall that in Perl, at least, !~ doesn't update $1, etc. If that's useful, we can add that.
Of course I could provide patches for all of this or parts thereof if it's the right direction. Just tell me what you'd prefer.
Just a patch for the existing code would be useful. Maybe after that, factor out the regex code into one place. Alan DeKok.
Alan DeKok wrote:
Enrik Berkhan wrote:
This seems to work, variables are unset if they have an old value and no new value (no match at all or empty) or they are set/updated if they have a new value (match implied).
Hmm... on a regex match, all previous match information should be deleted. This prevents old matches from polluting the variables for the current match.
The current change does this.
I've only tested it using unlang: Isn't that just incredibly easy to use? :)
Yes :)
Just a patch for the existing code would be useful. Maybe after that, factor out the regex code into one place.
I've attached the patch that I used for testing. Enrik
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
Enrik Berkhan