From: Alan T. DeKok Date: Mon, 29 Mar 2021 13:22:21 +0000 (-0400) Subject: don't hoist things too far X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=31c516c46d362fdc1783a4bf798c4df5ee64ca3a;p=thirdparty%2Ffreeradius-server.git don't hoist things too far do less hoisting during normalization. Do one last set of hoisting just before we return. --- diff --git a/src/lib/server/cond_tokenize.c b/src/lib/server/cond_tokenize.c index e4add0fccad..3a82525e1f7 100644 --- a/src/lib/server/cond_tokenize.c +++ b/src/lib/server/cond_tokenize.c @@ -528,10 +528,9 @@ static int cond_normalise(TALLOC_CTX *ctx, fr_token_t lhs_type, fr_cond_t **c_ou /* * Normalize the condition before returning. * - * We collapse multiple levels of braces to one. Then - * convert maps to literals. Then literals to true/false - * statements. Then true/false ||/&& followed by other - * conditions to just conditions. + * We convert maps to literals. Then literals to + * true/false statements. Then true/false ||/&& followed + * by other conditions to just conditions. * * Order is important. The more complex cases are * converted to simpler ones, from the most complex cases @@ -539,59 +538,33 @@ static int cond_normalise(TALLOC_CTX *ctx, fr_token_t lhs_type, fr_cond_t **c_ou */ /* - * Loop because we might hoist a child to a child. + * Do some limited normalizations here. */ - while (c->type == COND_TYPE_CHILD) { - fr_cond_t *child; - - child = c->data.child; - - /* - * (FOO) --> FOO - * (FOO) ... --> FOO ... - */ - if (!child->next) { - (void) talloc_steal(ctx, child); - child->parent = c->parent; - child->next = talloc_steal(child, c->next); - - /* - * !(!FOO) --> FOO, etc. - */ - child->negate = (c->negate != child->negate); - - talloc_free(c); - c = child; - continue; - } + if (c->type == COND_TYPE_CHILD) { + fr_cond_t *child = c->data.child; /* - * (FOO ...) --> FOO ... - * - * But don't do !(FOO || BAR) --> !FOO || BAR - * Because that's different. + * ((FOO)) --> (FOO) + * !(!(FOO)) --> FOO */ - if (!c->next && !c->negate) { - (void) talloc_steal(ctx, child); - child->parent = c->parent; - - talloc_free(c); - c = child; - continue; + if (!c->next && !child->next) { + if ((child->type == COND_TYPE_CHILD) || + (child->type == COND_TYPE_TRUE) || + (child->type == COND_TYPE_FALSE)) { + (void) talloc_steal(ctx, child); + child->parent = c->parent; + child->negate = (c->negate != child->negate); + + talloc_free(c); + c = child; + } } /* - * Can't do anything else, stop looping. + * No further optimizations are possible, so we + * just check for and/or short-circuit. */ - break; - } - - /* - * No further optimizations are possible, so we just - * return. - */ - if (c->type == COND_TYPE_CHILD) { - goto check_short_circuit; + goto check_short_circuit; } /* @@ -1610,7 +1583,27 @@ ssize_t fr_cond_tokenize(CONF_SECTION *cs, fr_cond_t **head, tmpl_rules_t const /* * Now that everything has been normalized, reparent the children. */ - if (*head) cond_reparent(*head, NULL); + if (*head) { + fr_cond_t *c = *head; + + /* + * (FOO) -> FOO + * !(!(FOO)) -> FOO + */ + if ((c->type == COND_TYPE_CHILD) && !c->next) { + if (!c->negate || !c->data.child->next) { + fr_cond_t *child = c->data.child; + (void) talloc_steal(cs, child); + child->parent = c->parent; + child->negate = (c->negate != child->negate); + + talloc_free(c); + *head = child; + } + } + + cond_reparent(*head, NULL); + } return slen + diff; } diff --git a/src/tests/unit/condition/base.txt b/src/tests/unit/condition/base.txt index 23981183bca..99027016951 100644 --- a/src/tests/unit/condition/base.txt +++ b/src/tests/unit/condition/base.txt @@ -1,4 +1,3 @@ - # # Tests for parsing conditional expressions. # @@ -52,7 +51,7 @@ match ERROR offset 4: Invalid operator # And now we have a bunch of VALID conditions we want to parse. -# sillyness is OK +# sillyness is OK, but cleaned up. condition ((((((ok)))))) match ok @@ -81,23 +80,23 @@ condition (&User-Name == &User-Password || &Filter-Id == &Reply-Message) match &User-Name == &User-Password || &Filter-Id == &Reply-Message condition ((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)) -match &User-Name == &User-Password || &Filter-Id == &Reply-Message +match (&User-Name == &User-Password) || (&Filter-Id == &Reply-Message) condition (!(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)) -match !&User-Name == &User-Password || &Filter-Id == &Reply-Message +match !(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message) # different from the previous ones. condition (!((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message))) -match !(&User-Name == &User-Password || &Filter-Id == &Reply-Message) +match !((&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)) condition (!(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message)) -match !&User-Name == &User-Password || &Filter-Id == &Reply-Message +match !(&User-Name == &User-Password) || (&Filter-Id == &Reply-Message) condition ((a == b) || (c == d))) match ERROR offset 22: Unexpected closing brace condition (handled && (&Packet-Type == Access-Challenge)) -match handled && &Packet-Type == Access-Challenge +match handled && (&Packet-Type == Access-Challenge) # This is OK, without the braces condition handled && &Packet-Type == Access-Challenge @@ -655,5 +654,14 @@ match false condition (&User-Name == "bob") || (true) match true +# +# A && (B || C) is not the same as (A && B) || C, for 0/1/1 +# +# 0 && (1 || 1) = 0 && 1 == 0 +# (0 && 1) || 1 = 0 || 1 == 1 +# +condition (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message) +match (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message) + count -match 317 +match 319