From: Alan T. DeKok <aland@freeradius.org>
Date: Fri, 6 Oct 2023 17:21:28 +0000 (-0400)
Subject: don't do too much optimization for logical or
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cf831e21d802cf632ef20a86fa017e99ad41b028;p=thirdparty%2Ffreeradius-server.git

don't do too much optimization for logical or
---

diff --git a/src/lib/unlang/xlat_purify.c b/src/lib/unlang/xlat_purify.c
index c4b14a40a3..82f242cf24 100644
--- a/src/lib/unlang/xlat_purify.c
+++ b/src/lib/unlang/xlat_purify.c
@@ -241,41 +241,75 @@ static bool is_truthy(xlat_exp_t *node, bool *out)
  *	Do some optimizations.
  *
  */
-static xlat_exp_t *logical_peephole_optimize(xlat_exp_t *lhs, fr_token_t op, xlat_exp_t *rhs)
+static xlat_exp_t *peephole_optimize_lor(xlat_exp_t *lhs,  xlat_exp_t *rhs)
 {
 	bool value;
 
 	/*
-	 *	@todo - check for tail of LHS
-	 *		&& tail is truthy, then remove tail, and call ourselves recursively
-	 *		if there's a new node, it becomes the new tail.  Otherwise
-	 *		we append the rhs to the lhs args.
+	 *	LHS isn't truthy, we can't do anything.  If the LHS
+	 *	passes, we return the value of the LHS.
 	 *
-	 *	lhs->call.args->flags.can_purify |= rhs->flags.can_purify | rhs->flags.pure;
-	 *	lhs->flags.can_purify = lhs->call.args->flags.can_purify;
+	 *	FOO || ... --> FOO || ...
 	 */
 	if (!is_truthy(lhs, &value)) {
-		xlat_exp_t *tmp;
-
-		if (!is_truthy(rhs, &value)) return NULL;
+		/*
+		 *	FOO || 0 --> FOO much of the time
+		 *	FOO || 1 --> FOO much of the time
+		 *
+		 *	@todo - if the LHS is a function AND the LHS returns a boolean, THEN we can optimized
+		 *	the "FOO || 1" case to just "1".
+		 */
+		return NULL;
+	}
 
-		tmp = lhs;
-		lhs = rhs;
-		rhs = tmp;
+	/*
+	 *	1 || FOO   --> 1
+	 *	0 || FOO   --> FOO
+	 */
+	if (value) {
+		talloc_free(rhs);
+		return lhs;
 	}
 
+	talloc_free(lhs);
+	return rhs;
+}
+
+
+/*
+ *	Do some optimizations.
+ *
+ */
+static xlat_exp_t *peephole_optimize_land(xlat_exp_t *lhs, xlat_exp_t *rhs)
+{
+	bool value;
+
 	/*
-	 *	1 && FOO   --> FOO
-	 *	0 && FOO   --> 0
-	 *	FOO && BAR --> FOO && BAR
+	 *	LHS isn't truthy
+	 *
+	 *	FOO && ... --> FOO && ...
 	 */
+	if (!is_truthy(lhs, &value)) {
+		/*
+		 *	FOO && 0 --> 0
+		 *	FOO && 1 --> FOO
+		 */
+		if (!is_truthy(rhs, &value)) return NULL;
+
+		if (!value) {
+			talloc_free(lhs);
+			return rhs;
+		}
+
+		talloc_free(rhs);
+		return lhs;
+	}
 
 	/*
-	 *	1 || FOO   --> 1
-	 *	0 || FOO   --> FOO
-	 *	FOO || BAR --> FOO || BAR
+	 *	0 && FOO   --> 0
+	 *	1 && FOO   --> FOO
 	 */
-	if (value == (op != T_LAND)) {
+	if (!value) {
 		talloc_free(rhs);
 		return lhs;
 	}
@@ -284,7 +318,6 @@ static xlat_exp_t *logical_peephole_optimize(xlat_exp_t *lhs, fr_token_t op, xla
 	return rhs;
 }
 
-
 /*
  *	Do peephole optimizations.
  */
@@ -350,7 +383,6 @@ static int binary_peephole_optimize(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_
 	/*
 	 *	The tmpl_tokenize code takes care of resolving the data if there's a cast.
 	 */
-
 	lhs_box = xlat_value_box(lhs);
 	if (!lhs_box) return 0;
 
@@ -374,10 +406,20 @@ static int binary_peephole_optimize(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_
 
 int xlat_purify_op(TALLOC_CTX *ctx, xlat_exp_t **out, xlat_exp_t *lhs, fr_token_t op, xlat_exp_t *rhs)
 {
-	if ((op == T_LAND) || (op == T_LOR)) {
+	if (op == T_LOR) {
+		xlat_exp_t *node;
+
+		node = peephole_optimize_lor(lhs, rhs);
+		if (!node) return 0;
+
+		*out = node;
+		return 1;
+	}
+
+	if (op == T_LAND) {
 		xlat_exp_t *node;
 
-		node = logical_peephole_optimize(lhs, op, rhs);
+		node = peephole_optimize_land(lhs, rhs);
 		if (!node) return 0;
 
 		*out = node;
diff --git a/src/tests/keywords/logical-or-assign b/src/tests/keywords/logical-or-assign
new file mode 100644
index 0000000000..3c0fc43d2a
--- /dev/null
+++ b/src/tests/keywords/logical-or-assign
@@ -0,0 +1,13 @@
+string foo
+
+&Acct-Session-Time = 30
+
+#
+#  Test for short-circuit logical optimizations
+#
+&foo = %{&Acct-Session-Time || 'NULL'}
+
+if !(&foo == "30") {
+	test_fail
+}
+success
diff --git a/src/tests/unit/condition/base.txt b/src/tests/unit/condition/base.txt
index 13d657fbe3..afe19da234 100644
--- a/src/tests/unit/condition/base.txt
+++ b/src/tests/unit/condition/base.txt
@@ -657,8 +657,18 @@ match false
 condition (&User-Name == "bob") && (false)
 match false
 
+#
+#  This could return a string of the User-Name, *or* a boolean true.
+#
+condition &User-Name || true
+match (&User-Name || true)
+
+#
+#  The evaluator does not know that the LHS returns a boolean, so it
+#  can't optimize it.
+#
 condition (&User-Name == "bob") || (true)
-match true
+match ((&User-Name == "bob") || true)
 
 #
 #  A && (B || C) is not the same as (A && B) || C, for 0/1/1
@@ -670,4 +680,4 @@ condition (&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message)
 match ((&User-Name == "bob") && ((&User-Password == "bob") || &EAP-Message))
 
 count
-match 305
+match 307