Premature end of paircompare when using !* op on non-present attribute
Hi all, yesterday I've noticed the following when using the '!*' op (T_OP_CMP_FALSE) in the check items. When the attribute to be compared is not found in the list of request attributes, '!*' evaluates to true, which is correct, but further processing of the remaining check items will be skipped. The code responsible for this behaviour comes from paircompare() in src/main/valuepair.c:
/* * Not found, it's not a match. */ if (auth_item == NULL) { /* * Didn't find it. If we were *trying* * to not find it, then we succeeded. */ if (check_item->operator == T_OP_CMP_FALSE) return 0; else return -1; }
I believe the `return 0' to be incorrect. I think it should be `continue' instead to evaluate the remaining check items. Or is it correct like it is? Enrik
Here try this. Enrik Berkhan wrote:
Hi all,
yesterday I've noticed the following when using the '!*' op (T_OP_CMP_FALSE) in the check items. When the attribute to be compared is not found in the list of request attributes, '!*' evaluates to true, which is correct, but further processing of the remaining check items will be skipped. The code responsible for this behaviour comes from paircompare() in src/main/valuepair.c:
/* * Not found, it's not a match. */ if (auth_item == NULL) { /* * Didn't find it. If we were *trying* * to not find it, then we succeeded. */ if (check_item->operator == T_OP_CMP_FALSE) return 0; else return -1; }
I believe the `return 0' to be incorrect. I think it should be `continue' instead to evaluate the remaining check items. Or is it correct like it is?
Enrik - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
#! /bin/sh /usr/share/dpatch/dpatch-run ## 200-cmp-operators-fix.dpatch by <joe@nameserver3.ttec.com> ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: No description. @DPATCH@ diff -urNad freeradius-1.1.0~/src/main/valuepair.c freeradius-1.1.0/src/main/valuepair.c --- freeradius-1.1.0~/src/main/valuepair.c 2005-11-06 00:40:42.000000000 -0500 +++ freeradius-1.1.0/src/main/valuepair.c 2005-11-06 00:40:46.000000000 -0500 @@ -298,8 +298,13 @@ * Didn't find it. If we were *trying* * to not find it, then we succeeded. */ - if (check_item->operator == T_OP_CMP_FALSE) - return 0; + if (check_item->operator == T_OP_CMP_FALSE || +#ifdef HAVE_REGEX_H + check_item->operator == T_OP_REG_NE || +#endif + check_item->operator == T_OP_NE + ) + continue; else return -1; } @@ -310,7 +315,10 @@ */ if (check_item->operator == T_OP_CMP_FALSE) return -1; - + + /* no comparison needed if it exists */ + if (check_item->operator == T_OP_CMP_TRUE) + continue; /* * We've got to xlat the string before doing @@ -342,14 +350,14 @@ radlog(L_INFO, "Invalid operator for item %s: " "reverting to '=='", check_item->name); /*FALLTHRU*/ - case T_OP_CMP_TRUE: /* compare always == 0 */ - case T_OP_CMP_FALSE: /* compare always == 1 */ case T_OP_CMP_EQ: if (compare != 0) result = -1; break; case T_OP_NE: - if (compare == 0) result = -1; + /* != only succeeds if NO reply values match*/ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; case T_OP_LT: @@ -457,7 +465,9 @@ compare = regexec(®, (char *)auth_item->vp_strvalue, 0, NULL, 0); regfree(®); - if (compare == 0) result = -1; + /* !~ only succeeds if NO matches are found in reply pair */ + if (compare == 0) return -1; + result = -1; /*check the rest for no match */ break; #endif
participants (2)
-
Enrik Berkhan -
Joe Maimon