From: Arran Cudbard-Bell <a.cudbardb@freeradius.org>
Date: Sun, 17 Mar 2024 01:11:15 +0000 (-0400)
Subject: Fix some behavioural issues with logical or, and logical and
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9b8d655423e8937e16ff3f745d7ae660b7eab077;p=thirdparty%2Ffreeradius-server.git

Fix some behavioural issues with logical or, and logical and

%{0 || 0) was returning NULL, which is NOT correct (should return 0)
---

diff --git a/src/lib/unlang/xlat_expr.c b/src/lib/unlang/xlat_expr.c
index 559ad09f06e..1793441aef5 100644
--- a/src/lib/unlang/xlat_expr.c
+++ b/src/lib/unlang/xlat_expr.c
@@ -1130,14 +1130,19 @@ static xlat_action_t xlat_logical_process_arg(UNUSED TALLOC_CTX *ctx, UNUSED fr_
  *  @param[in]     rctx our ctx
  *  @param[in]     in   list of value-boxes to check
  *  @return
- *	- false on failure
- *	- true for match, with dst updated to contain the relevant box.
+ *	- false if there are no truthy values. The last box is copied to the rctx.
+ *	  This is to allow us to return default values which may not be truthy,
+ *	  e.g. %{&Counter || 0} or %{&Framed-IP-Address || 0.0.0.0}.
+ *	  If we don't copy the last box to the rctx, the expression just returns NULL
+ *	  which is never useful...
+ *	- true if we find a truthy value.  The first truthy box is copied to the rctx.
  *
  *  Empty lists are not truthy.
  */
 static bool xlat_logical_or(xlat_logical_rctx_t *rctx, fr_value_box_list_t const *in)
 {
-	fr_value_box_t *found = NULL;
+	fr_value_box_t *last = NULL;
+	bool ret = false;
 
 	/*
 	 *	Empty lists are !truthy.
@@ -1153,32 +1158,27 @@ static bool xlat_logical_or(xlat_logical_rctx_t *rctx, fr_value_box_list_t const
 			continue;
 		}
 
+		last = box;
+
 		/*
 		 *	Remember the last box we found.
 		 *
 		 *	If it's truthy, then we stop immediately.
 		 */
 		if (fr_value_box_is_truthy(box)) {
-			found = box;
+			ret = true;
 			break;
 		}
-
-		/*
-		 *	Stop on the first "false"
-		 */
-		return false;
 	}
 
-	if (!found) return false;
-
 	if (!rctx->box) {
 		MEM(rctx->box = fr_value_box_alloc_null(rctx->ctx));
 	} else {
 		fr_value_box_clear(rctx->box);
 	}
-	fr_value_box_copy(rctx->box, rctx->box, found);
+	if (last) fr_value_box_copy(rctx->box, rctx->box, last);
 
-	return true;
+	return ret;
 }
 
 /*
@@ -1256,7 +1256,7 @@ static bool xlat_logical_and(xlat_logical_rctx_t *rctx, fr_value_box_list_t cons
 	 */
 	fr_value_box_list_foreach(in, box) {
 		if (fr_box_is_group(box)) {
-			if (!xlat_logical_or(rctx, &box->vb_group)) return false;
+			if (!xlat_logical_and(rctx, &box->vb_group)) return false;
 			continue;
 		}
 
diff --git a/src/tests/keywords/expr-alt-missing b/src/tests/keywords/expr-alt-missing
new file mode 100644
index 00000000000..b94da8279e2
--- /dev/null
+++ b/src/tests/keywords/expr-alt-missing
@@ -0,0 +1,31 @@
+&Acct-Input-Octets := 0
+
+# 0 is false according to the truthy rules, should return 5
+if (!(%{&Acct-Input-Octets || 5} == 5)) {
+        test_fail
+}
+
+# Both values are not truthy, but it's still more useful to
+# return one on them instead of NULL, and this is an extremely
+# common use case when setting defaults.
+if ("%{&Acct-Input-Octets || 0}" == '') {
+        test_fail
+}
+
+# Same as above, except 5 is truthy, so we DEFINITELY shouldn't
+# be returning NULL.
+if ("%{&Acct-Input-Octets || 5}" == '') {
+        test_fail
+}
+
+# Completely absent null value is definitely not truthy
+if (!(%{&Acct-Output-Octets || 5} == 5)) {
+        test_fail
+}
+
+# One null should not trigger the expression returning null overall
+if ("%{&Acct-Output-Octets || 5}" == '') {
+        test_fail
+}
+
+success