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 559ad09f06e83a482f10495c1973db21ddf419d9..1793441aef55022ee960429ea63e78b8aaa998e8 100644 (file)
--- 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 (file)
index 0000000..b94da82
--- /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