omit optional arguments entirely, instead of passing the wrong type

if an optional argument is missing (i.e. source list is empty),
then we can pass it to the function ONLY if the function accepts
type VOID for that argument.

Otherwise, we have to omit that argument, and all subsequent ones.
Otherwise, we'll pass the wrong data type to a function which
expects to be able to dereference the type-specific field of that
value box, and who knows what will happen.

As a result, any function which takes an optional argument has to
check for non-existence, not for NULL type.

We will reserve NULL types for "error", not for normal usage.

diff --git a/src/lib/eap_aka_sim/xlat.c b/src/lib/eap_aka_sim/xlat.c
index 85e2db8d26fd4e038e4423f9e8f94b53ffe5998f..b7920403b9bff705bfedb1fb29180d1424003260 100644 (file)
--- a/src/lib/eap_aka_sim/xlat.c
+++ b/src/lib/eap_aka_sim/xlat.c
@@ -245,7 +245,7 @@ static xlat_action_t aka_sim_3gpp_temporary_id_decrypt_xlat(TALLOC_CTX *ctx, fr_
        fr_value_box_t  *vb;
        fr_pair_t       *eap_type;
 
-       if (!fr_type_is_null(tag_vb->type)) include_tag = tag_vb->vb_bool;
+       if (tag_vb) include_tag = tag_vb->vb_bool;
 
        if (id_len != (AKA_SIM_3GPP_PSEUDONYM_LEN)) {
                REDEBUG2("3gpp pseudonym incorrect length, expected %i bytes, got %zu bytes",
@@ -384,7 +384,7 @@ static xlat_action_t aka_sim_3gpp_temporary_id_encrypt_xlat(TALLOC_CTX *ctx, fr_
        /*
         *      Check for the optional type argument
         */
-       if (!fr_type_is_null(type_vb->type)) {
+       if (type_vb) {
                fr_dict_enum_value_t const              *type_enum;
 
                type_enum = fr_dict_enum_by_name(attr_eap_aka_sim_identity_type,

diff --git a/src/lib/unlang/xlat_builtin.c b/src/lib/unlang/xlat_builtin.c
index 0dac2b39fb07bf58602b080f76197ee333de5cfe..0a015ba06708e3f53e452a1608765da256e7e579 100644 (file)
--- a/src/lib/unlang/xlat_builtin.c
+++ b/src/lib/unlang/xlat_builtin.c
@@ -961,7 +961,7 @@ static xlat_action_t xlat_func_debug(TALLOC_CTX *ctx, fr_dcursor_t *out,
        /*
         *  Assume we just want to get the current value and NOT set it to 0
         */
-       if (fr_box_is_null(in_head)) goto done;
+       if (!in_head) goto done;
 
        level = in_head->vb_int8;
        if (level == 0) {

diff --git a/src/lib/unlang/xlat_eval.c b/src/lib/unlang/xlat_eval.c
index dbb4a162b7edc2fab19e498209e4b3b03d49c8c3..8c0f97b4334318ef431eed45b2772a1eef06b9a4 100644 (file)
--- a/src/lib/unlang/xlat_eval.c
+++ b/src/lib/unlang/xlat_eval.c
@@ -410,6 +410,7 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list, requ
                         */
                        if (!vb) {
                                if (arg_p->required) {
+                               missing:
                                        REDEBUG("Missing required argument %u", (unsigned int)((arg_p - args) + 1));
                                        return XLAT_ACTION_FAIL;
                                }
@@ -437,26 +438,60 @@ xlat_action_t xlat_process_args(TALLOC_CTX *ctx, fr_value_box_list_t *list, requ
                        if (xa != XLAT_ACTION_DONE) return xa;
 
                        /*
-                        *      In some cases we replace the current
-                        *      argument with the head of the group.
+                        *      This argument doesn't exist.  That might be OK, or it may be a fatal error.
                         */
-                       if (arg_p->single || arg_p->concat) {
+                       if (fr_dlist_empty(&vb->vb_group)) {
                                /*
-                                *      If the group is empty, convert
-                                *      it to a null box to maintain
-                                *      correct ordering in the argument
-                                *      list.
+                                *      Empty groups for optional arguments are OK, we can just stop processing the list.
                                 */
-                               if (fr_dlist_empty(&vb->vb_group)) {
-                                       fr_value_box_t *prev = fr_dlist_remove(list, vb);
-                                       fr_value_box_init_null(vb);
-                                       fr_dlist_insert_after(list, prev, vb);
-                               } else {
-                                       fr_dlist_replace(list, vb, fr_dlist_pop_head(&vb->vb_group));
+                               if (!arg_p->required) {
+                                       fr_assert(!next);
+
+                                       /*
+                                        *      If the caller doesn't care about the type, then we leave the
+                                        *      empty group there.
+                                        */
+                                       if (arg_p->type == FR_TYPE_VOID) goto do_next;
+
+                                       /*
+                                        *      The caller does care about the type, and we don't have any
+                                        *      matching data.  Omit this argument, and all arguments after it.
+                                        *
+                                        *      i.e. if the caller has 3 optional arguments, all
+                                        *      FR_TYPE_UINT8, and the first one is missing, then we MUST
+                                        *      either supply boxes all of FR_TYPE_UINT8, OR we supply nothing.
+                                        *
+                                        *      We can't supply a box of any other type, because the caller
+                                        *      has declared that it wants FR_TYPE_UINT8, and is naively
+                                        *      accessing the box as vb_uint8, hoping that it's being passed
+                                        *      the right thing.
+                                        */
+                                       fr_dlist_remove(list, vb);
                                        talloc_free(vb);
+                                       break;
                                }
+
+                               /*
+                                *      If the caller is expecting a particular type, then getting nothing is
+                                *      an error.
+                                *
+                                *      If the caller manually checks the input type, then we can leave it as
+                                *      an empty group.
+                                */
+                               if (arg_p->type != FR_TYPE_VOID) goto missing;
+                       }
+
+                       /*
+                        *      In some cases we replace the current argument with the head of the group.
+                        *
+                        *      xlat_process_arg_list() has already done concatenations for us.
+                        */
+                       if (arg_p->single || arg_p->concat) {
+                               fr_dlist_replace(list, vb, fr_dlist_pop_head(&vb->vb_group));
+                               talloc_free(vb);
                        }
 
+               do_next:
                        if (arg_p->variadic) {
                                if (!next) break;
                        } else {

diff --git a/src/modules/rlm_client/rlm_client.c b/src/modules/rlm_client/rlm_client.c
index b195880b0f239fad72adf317e3b8f7f2eb16ceee..7a4b431be629ab17b42d3d34bfbb556395a428ee 100644 (file)
--- a/src/modules/rlm_client/rlm_client.c
+++ b/src/modules/rlm_client/rlm_client.c
@@ -240,7 +240,7 @@ static xlat_action_t xlat_client(TALLOC_CTX *ctx, fr_dcursor_t *out,
        fr_value_box_t  *client_ip = fr_dlist_next(in, field);
        fr_value_box_t  *vb;
 
-       if (!fr_box_is_null(client_ip)) {
+       if (client_ip) {
                if (fr_inet_pton(&ip, client_ip->vb_strvalue, -1, AF_UNSPEC, false, true) < 0) {
                        RDEBUG("Invalid client IP address \"%s\"", client_ip->vb_strvalue);
                        return XLAT_ACTION_FAIL;